Received: by 2002:a05:6a10:22f:0:0:0:0 with SMTP id 15csp627866pxk; Thu, 17 Sep 2020 11:43:16 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzpFeTLUwmcAuoWGU1YC1QgL0szqAjO1qC2kp/9bqAeeKb1ti2LZCuzsvKjOtEaISHXFatK X-Received: by 2002:a50:bb65:: with SMTP id y92mr34298015ede.53.1600368196617; Thu, 17 Sep 2020 11:43:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1600368196; cv=none; d=google.com; s=arc-20160816; b=w2u8mXvGZrdUsc8/Ps8snOhMJLXcTvOrEqhLfraF/d5sH8+a/l9AY/+Bv/2vFe9MV9 zFmd7HarXZ+Ca5XLeQPV13OrKPHJLYAmDZrHpVkJrK4lP4MVW4jpi6usw7N3fCVKNa+I +Jl/neYXnPsKonmenlIAf4+Az02zU8jWSOXbBCKbBVwMmJZ2zgEsiK61s6PTmIBlw7Sm NWjJ6+FUprrkqvZeFVC9d3CGQnVQi4TDNSiL6FBiSE0T/Z3t61ShQSLkriX8x/6WF6l6 bkTSJ0nAJFoqgWFqRHuqyn2LUHoPxSMQBqV6EddP3b9Hn+nRBqEmr/bhZIKvu5aa/P0C k2wA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :message-id:subject:cc:to:from:date:dkim-signature; bh=CgjsOf/ewGmUBy2NWdzTp5Rv6s7HlxSecdeA/vMZAdo=; b=Qk3sV+RIMIufWakm8IIF6YYiJgGVXQyJsz5myIS5OI6Rwt28jZWAafQOOaFw/tbs39 dy9lEEWDRlFl2DScACDmSow0kwsT0Mnn/Yddkf+OaqidTDwgCvM6rsKyWs+cZNybfsxn 2w+kMYhaZvLzXMcOp+EILn85URKQIVLznEHmp9kzMNY64DIn3J+xoK+UQDSGKhoB/TZf NVUPMKygXLO95QHBT4b9MLWO5h1mo4+JX3TEqluM5FignAmBG5YD//5mzbBanM1BIzay DHAiWruDrtwB2xTWkwSTyhAJMfQUEzX4kBrGkfT2ojc+RhenxByPsRyWHTztv8ACQ6qj ar2Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=wkTGE77d; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id b43si484988edf.15.2020.09.17.11.42.53; Thu, 17 Sep 2020 11:43:16 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=wkTGE77d; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726301AbgIQSjl (ORCPT + 99 others); Thu, 17 Sep 2020 14:39:41 -0400 Received: from mail.kernel.org ([198.145.29.99]:32932 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726375AbgIQRU0 (ORCPT ); Thu, 17 Sep 2020 13:20:26 -0400 Received: from localhost (52.sub-72-107-123.myvzw.com [72.107.123.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8F92F221E7; Thu, 17 Sep 2020 17:20:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600363211; bh=mEQ2zYGQwN+YFky2JH41RrTh7Hdb3VlvGJsG4dOTm3U=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=wkTGE77d5M5NxO963X691r6qrDnGMAshe/FjCD3VjrdWNtOFuRQK4Wawm2TaF9GR0 xJbobnfdWHK9bTUkSHWi/TKgaZNugKuhAUmBf9flJdtRUclUR6U1p4MevONmNaAEa2 vewm8cpAxMst/jQlk+1NohmsU7mqL9DDnGSUT7oo= Date: Thu, 17 Sep 2020 12:20:10 -0500 From: Bjorn Helgaas To: "Derrick, Jonathan" Cc: "wangxiongfeng2@huawei.com" , "kw@linux.com" , "hkallweit1@gmail.com" , "kai.heng.feng@canonical.com" , "refactormyself@gmail.com" , "linux-kernel@vger.kernel.org" , "mika.westerberg@linux.intel.com" , "Mario.Limonciello@dell.com" , "linux-pci@vger.kernel.org" , "bhelgaas@google.com" , "Wysocki, Rafael J" Subject: Re: [PATCH] PCI/ASPM: Enable ASPM for links under VMD domain Message-ID: <20200917172010.GA1710481@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4db0fbba635cd1ff5a3c1529d3c7fa08d0729756.camel@intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 10, 2020 at 07:51:05PM +0000, Derrick, Jonathan wrote: > On Thu, 2020-09-10 at 14:17 -0500, Bjorn Helgaas wrote: > > On Thu, Sep 10, 2020 at 06:52:48PM +0000, Derrick, Jonathan wrote: > > > On Thu, 2020-09-10 at 12:38 -0500, Bjorn Helgaas wrote: > > > > On Thu, Sep 10, 2020 at 04:33:39PM +0000, Derrick, Jonathan wrote: > > > > > On Wed, 2020-09-09 at 20:55 -0500, Bjorn Helgaas wrote: > > > > > > On Fri, Aug 21, 2020 at 08:32:20PM +0800, Kai-Heng Feng wrote: > > > > > > > New Intel laptops with VMD cannot reach deeper power saving state, > > > > > > > renders very short battery time. > > > > > > > > > > > > > > As BIOS may not be able to program the config space for devices under > > > > > > > VMD domain, ASPM needs to be programmed manually by software. This is > > > > > > > also the case under Windows. > > > > > > > > > > > > > > The VMD controller itself is a root complex integrated endpoint that > > > > > > > doesn't have ASPM capability, so we can't propagate the ASPM settings to > > > > > > > devices under it. Hence, simply apply ASPM_STATE_ALL to the links under > > > > > > > VMD domain, unsupported states will be cleared out anyway. > > > > > > > > > > > > > > Signed-off-by: Kai-Heng Feng > > > > > > > --- > > > > > > > drivers/pci/pcie/aspm.c | 3 ++- > > > > > > > drivers/pci/quirks.c | 11 +++++++++++ > > > > > > > include/linux/pci.h | 2 ++ > > > > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > > > > > > index 253c30cc1967..dcc002dbca19 100644 > > > > > > > --- a/drivers/pci/pcie/aspm.c > > > > > > > +++ b/drivers/pci/pcie/aspm.c > > > > > > > @@ -624,7 +624,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) > > > > > > > aspm_calc_l1ss_info(link, &upreg, &dwreg); > > > > > > > > > > > > > > /* Save default state */ > > > > > > > - link->aspm_default = link->aspm_enabled; > > > > > > > + link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ? > > > > > > > + ASPM_STATE_ALL : link->aspm_enabled; > > > > > > > > > > > > This function is ridiculously complicated already, and I really don't > > > > > > want to make it worse. > > > > > > > > > > > > What exactly is the PCIe topology here? Apparently the VMD controller > > > > > > is a Root Complex Integrated Endpoint, so it's a Type 0 (non-bridge) > > > > > > device. And it has no Link, hence no Link Capabilities or Control and > > > > > > hence no ASPM-related bits. Right? > > > > > > > > > > That's correct. VMD is the Type 0 device providing config/mmio > > > > > apertures to another segment and MSI/X remapping. No link and no ASPM > > > > > related bits. > > > > > > > > > > Hierarchy is usually something like: > > > > > > > > > > Segment 0 | VMD segment > > > > > Root Complex -> VMD | Type 0 (RP/Bridge; physical slot) - Type 1 > > > > > | Type 0 (RP/Bridge; physical slot) - Type 1 > > > > > > > > > > > And the devices under the VMD controller? I guess they are regular > > > > > > PCIe Endpoints, Switch Ports, etc? Obviously there's a Link involved > > > > > > somewhere. Does the VMD controller have some magic, non-architected > > > > > > Port on the downstream side? > > > > > > > > > > Correct: Type 0 and Type 1 devices, and any number of Switch ports as > > > > > it's usually pinned out to physical slot. > > > > > > > > > > > Does this patch enable ASPM on this magic Link between VMD and the > > > > > > next device? Configuring ASPM correctly requires knowledge and knobs > > > > > > from both ends of the Link, and apparently we don't have those for the > > > > > > VMD end. > > > > > > > > > > VMD itself doesn't have the link to it's domain. It's really just the > > > > > config/mmio aperture and MSI/X remapper. The PCIe link is between the > > > > > Type 0 and Type 1 devices on the VMD domain. So fortunately the VMD > > > > > itself is not the upstream part of the link. > > > > > > > > > > > Or is it for Links deeper in the hierarchy? I assume those should > > > > > > just work already, although there might be issues with latency > > > > > > computation, etc., because we may not be able to account for the part > > > > > > of the path above VMD. > > > > > > > > > > That's correct. This is for the links within the domain itself, such as > > > > > between a type 0 and NVMe device. > > > > > > > > OK, great. So IIUC, below the VMD, there is a Root Port, and the Root > > > > Port has a link to some Endpoint or Switch, e.g., an NVMe device. And > > > > we just want to enable ASPM on that link. > > > > > > > > That should not be a special case; we should be able to make this so > > > > it Just Works. Based on this patch, I guess the reason it doesn't > > > > work is because link->aspm_enabled for that link isn't set correctly. > > > > > > > > So is this just a consequence of us depending on the initial Link > > > > Control value from BIOS? That seems like something we shouldn't > > > > really depend on. > Seems like a good idea, that it should instead be quirked if ASPM is > found unusable on a link. Though I'm not aware of how many platforms > would require a quirk.. > > > > > > > > That's the crux. There's always pcie_aspm=force. > > > Something I've wondered is if there is a way we could 'discover' if the > > > link is ASPM safe? > > > > Sure. Link Capabilities is supposed to tell us that. If aspm.c > > depends on the BIOS settings, I think that's a design mistake. > > > > But what CONFIG_PCIEASPM_* setting are you using? The default > > is CONFIG_PCIEASPM_DEFAULT, which literally means "Use the BIOS > > defaults". If you're using that, and BIOS doesn't enable ASPM below > > VMD, I guess aspm.c will leave it disabled, and that seems like it > > would be the expected behavior. > > > > Does "pcie_aspm=force" really help you? I don't see any uses of it > > that should apply to your situation. > > > > Bjorn > > No you're right. I don't think we need pcie_aspm=force, just the policy > configuration. I'm not sure where we're at here. If the kernel is built with CONFIG_PCIEASPM_DEFAULT=y (which means "use the BIOS defaults"), and the BIOS doesn't enable ASPM on these links below VMD, then Linux will leave things alone. I think that's working as intended. If desired, we should be able to enable ASPM using sysfs in that case. We have a pci_disable_link_state() kernel interface that drivers can use to *disable* ASPM for their device. But I don't think there's any corresponding interface for drivers to *enable* ASPM. Maybe that's an avenue to explore? Bjorn