linux-kernel - Re: [PATCH V5 3/3] PCI: vmd: Configure PCIe ASPM and LTR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f5318c1171c5733388c9b455f2058f9aed127b8b.camel@linux.intel.com>
Date:   Sat, 19 Feb 2022 10:59:05 -0800
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Jonathan Derrick <jonathan.derrick@...ux.dev>,
        nirmal.patel@...ux.intel.com, lorenzo.pieralisi@....com,
        hch@...radead.org, kw@...ux.com, robh@...nel.org,
        bhelgaas@...gle.com, michael.a.bottini@...ux.intel.com,
        rafael@...nel.org, me@...ityamohan.in
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V5 3/3] PCI: vmd: Configure PCIe ASPM and LTR

Hi,

On Fri, 2022-02-18 at 14:30 -0700, Jonathan Derrick wrote:
> 
> On 2/17/2022 9:50 PM, David E. Box wrote:
> > Currently, PCIe ports reserved for VMD use are not visible to BIOS
> > and
> > therefore not configured to enable PCIE ASPM. Additionally, PCIE
> > LTR
> > values may be left unset since BIOS will set a default maximum LTR
> > value
> > on endpoints to ensure that misconfigured devices don't block SoC
> > power
> > management. Lack of this programming results in high power
> > consumption
> > on laptops as reported in bugzilla [1].  For currently affected
> > products, use pci_enable_default_link_state to set the allowed link
> > states for devices on the root ports. Also set the LTR value to the
> > maximum value needed for the SoC. Per the VMD hardware team future
> > products using VMD will enable BIOS configuration of these
> > capabilities.
> Will the refreshes of the affected CPU be supported in BIOS or this
> workaround?

I'll have to check with the hardware/BIOS team. Still because it is
BIOS this should check for signs that it hasn't already configured ASPM
before modifying anything. I'll make this change.

David

> 
> > This solution is a workaround for current products that mainly
> > targets
> > laptops. Support is not provided if a switch is used nor for
> > hotplug.
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
> > 
> > Signed-off-by: Michael Bottini <michael.a.bottini@...ux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > ---
> > V5
> >   - Provide the LTR value as driver data.
> >   - Use DWORD for the config space write to avoid PCI WORD access
> > bug.
> >   - Set ASPM links firsts, enabling all link states, before setting
> > a
> >     default LTR if the capability is present
> >   - Add kernel message that VMD is setting the device LTR.
> > V4
> >   - Refactor vmd_enable_apsm() to exit early, making the lines
> > shorter
> >     and more readable. Suggested by Christoph.
> > V3
> >   - No changes
> > V2
> >   - Use return status to print pci_info message if ASPM cannot be
> > enabled.
> >   - Add missing static declaration, caught by lkp@...el.com
> > 
> >   drivers/pci/controller/vmd.c | 48
> > +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c
> > index a582c351b461..eac379c80cd7 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -67,10 +67,19 @@ enum vmd_features {
> >   	 * interrupt handling.
> >   	 */
> >   	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
> > +
> > +	/*
> > +	 * Enable ASPM on the PCIE root ports and set the default LTR
> > of the
> > +	 * storage devices on platforms where these values are not
> > configured by
> > +	 * BIOS. This is needed for laptops, which require these
> > settings for
> > +	 * proper power management of the SoC.
> > +	 */
> > +	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
> >   };
> >   
> >   struct vmd_device_data {
> >   	enum vmd_features features;
> > +	u16 ltr;
> >   };
> >   
> >   static DEFINE_IDA(vmd_instance_ida);
> > @@ -714,6 +723,38 @@ static void vmd_copy_host_bridge_flags(struct
> > pci_host_bridge *root_bridge,
> >   	vmd_bridge->native_dpc = root_bridge->native_dpc;
> >   }
> >   
> > +/*
> > + * Enable ASPM and LTR settings on devices that aren't configured
> > by BIOS.
> > + */
> > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void
> > *userdata)
> > +{
> > +	struct vmd_device_data *info = userdata;
> > +	u32 ltr_reg;
> > +	int pos;
> > +
> > +	if (!(info->features & VMD_FEAT_BIOS_PM_QUIRK))
> > +		return 0;
> > +
> > +	pci_enable_default_link_state(pdev, PCIE_LINK_STATE_ALL);
> > +
> > +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > +	if (!pos)
> > +		return 0;
> > +
> > +	/*
> > +	 * If the LTR capability is present, set the default values to
> > the
> > +	 * maximum required by the platform to allow the deepest power
> > +	 * management savings. Write this as a single DWORD where the
> > lower word
> > +	 * is the max snoop latency and the upper word is the max non-
> > snoop
> > +	 * latency.
> > +	 */
> > +	pci_info(pdev, "VMD: Setting a default LTR\n");
> > +	ltr_reg = (info->ltr << 16) | info->ltr;
> > +	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
> > ltr_reg);
> > +
> > +	return 0;
> > +}
> > +
> >   static int vmd_enable_domain(struct vmd_dev *vmd, struct
> > vmd_device_data *info)
> >   {
> >   	struct pci_sysdata *sd = &vmd->sysdata;
> > @@ -867,6 +908,8 @@ static int vmd_enable_domain(struct vmd_dev
> > *vmd, struct vmd_device_data *info)
> >   		pci_reset_bus(child->self);
> >   	pci_assign_unassigned_bus_resources(vmd->bus);
> >   
> > +	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, info);
> > +
> >   	/*
> >   	 * VMD root buses are virtual and don't return true on
> > pci_is_pcie()
> >   	 * and will fail pcie_bus_configure_settings() early. It can
> > instead be
> > @@ -1012,7 +1055,10 @@ static const struct vmd_device_data
> > vmd_28c0_data = {
> >   static const struct vmd_device_data vmd_467f_data = {
> >   	.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >   		    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > -		    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +		    VMD_FEAT_OFFSET_FIRST_VECTOR |
> > +		    VMD_FEAT_BIOS_PM_QUIRK,
> > +	/* 3145728 ns (LatencyScale of 1048576 ns with a LatencyValue
> > of 3) */
> > +	.ltr = 0x1003,
> >   };
> >   
> >   static const struct pci_device_id vmd_ids[] = {