Raphael, I'd like your help upstreaming this VMD power-saving patch, please

Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Kenneth R. Crudup]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]


Hello Raphael,

For almost two years now, I've been trying to get patches from Ubuntu that
enable ASPM for devices behind Intel's VMD, necessary to get full lower-power
states (including very-reduced power usage during s0ix sleep) on my Alderlake
(et al.) laptop, upstreamed into mainline.

One such thread: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/

Since the original set of patches on this, most of the work has been pushed
upstream, with only this last patch required to get fully into the "CPU%LPI"
and "SYS%LPI" (names according to "turbostat") states.

I'm surprised that with the number of VMD-enabled laptops out there (which I
had to keep on so I could dual-boot into Win11 (the disk geometry changes if
I disable it, rendering the Win11 partition useless)), that there haven't been
many reports of excessive power usage in Linux during sleep; perhaps because
many installations are running stock Ubuntu kernels (where I assume variants
of this patch remain) it isn't an issue, but I do believe having this upstreamed
is still valuable.

I don't have the resources you've got to test this fully for regressions, nor
the expertise getting a patch into the kernel, so I'd like to again bring this
up for discussion (hence the phone-book of a CC: here).

If there's anything I can do to help get this done, please let me know.

Thank you,

-Kenneth Crudup

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-PCI-ASPM-Fixup-ASPM-for-VMD-bridges.patch, Size: 2172 bytes --]

From ee3618a598a261bbc8a875557d42d6dbbbc4cdd0 Mon Sep 17 00:00:00 2001
From: "Kenneth R. Crudup" <kenny@panix.com>
Date: Fri, 13 Dec 2024 15:28:42 -0800
Subject: [PATCH] PCI/ASPM: Fixup ASPM for VMD bridges

Effectively a squashed commit of:
UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
---
 drivers/pci/pcie/aspm.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 29fcb0689a91..fdc1ce2755ff 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -788,6 +788,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
+{
+       struct pci_bus *bus = pdev->bus;
+       struct device *dev;
+       struct pci_driver *pdrv;
+
+       if (!pci_is_root_bus(bus))
+               return false;
+
+       dev = bus->bridge->parent;
+       if (dev == NULL)
+               return false;
+
+       pdrv = pci_dev_driver(to_pci_dev(dev));
+       if (pdrv == NULL || strcmp("vmd", pdrv->name))
+               return false;
+
+       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+       return true;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
+		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
-- 
2.48.1

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Bjorn Helgaas]

[+cc Sergey, Nirmal, Jonathan, Jian-Hong, David]

On Sun, May 11, 2025 at 01:10:24PM -0700, Kenneth R. Crudup wrote:
> 
> Hello Raphael,
> 
> For almost two years now, I've been trying to get patches from Ubuntu that
> enable ASPM for devices behind Intel's VMD, necessary to get full lower-power
> states (including very-reduced power usage during s0ix sleep) on my Alderlake
> (et al.) laptop, upstreamed into mainline.
> 
> One such thread: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/
> 
> Since the original set of patches on this, most of the work has been pushed
> upstream, with only this last patch required to get fully into the "CPU%LPI"
> and "SYS%LPI" (names according to "turbostat") states.
> 
> I'm surprised that with the number of VMD-enabled laptops out there (which I
> had to keep on so I could dual-boot into Win11 (the disk geometry changes if
> I disable it, rendering the Win11 partition useless)), that there haven't been
> many reports of excessive power usage in Linux during sleep; perhaps because
> many installations are running stock Ubuntu kernels (where I assume variants
> of this patch remain) it isn't an issue, but I do believe having this upstreamed
> is still valuable.
> 
> I don't have the resources you've got to test this fully for regressions, nor
> the expertise getting a patch into the kernel, so I'd like to again bring this
> up for discussion (hence the phone-book of a CC: here).
> 
> If there's anything I can do to help get this done, please let me know.
> 
> Thank you,
> 
> -Kenneth Crudup
> 
> -- 
> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

> From ee3618a598a261bbc8a875557d42d6dbbbc4cdd0 Mon Sep 17 00:00:00 2001
> From: "Kenneth R. Crudup" <kenny@panix.com>
> Date: Fri, 13 Dec 2024 15:28:42 -0800
> Subject: [PATCH] PCI/ASPM: Fixup ASPM for VMD bridges
> 
> Effectively a squashed commit of:
> UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> ---
>  drivers/pci/pcie/aspm.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 29fcb0689a91..fdc1ce2755ff 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -788,6 +788,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>  		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
>  }
>  
> +/*
> + * BIOS may not be able to access config space of devices under VMD domain, so
> + * it relies on software to enable ASPM for links under VMD.
> + */
> +static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
> +{
> +       struct pci_bus *bus = pdev->bus;
> +       struct device *dev;
> +       struct pci_driver *pdrv;
> +
> +       if (!pci_is_root_bus(bus))
> +               return false;
> +
> +       dev = bus->bridge->parent;
> +       if (dev == NULL)
> +               return false;
> +
> +       pdrv = pci_dev_driver(to_pci_dev(dev));
> +       if (pdrv == NULL || strcmp("vmd", pdrv->name))
> +               return false;
> +
> +       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
> +       return true;
> +}
> +
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	}
>  
>  	/* Save default state */
> -	link->aspm_default = link->aspm_enabled;
> +	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
> +		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;

PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
this potentially enables L1.2.  The L1.2 configuration depends on
T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
design and are not discoverable by the kernel.  See PCIe r6.0, sec
5.5.4:

  The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
  to the appropriate values based on the components and AC coupling
  capacitors used in the connection linking the two components. The
  determination of these values is design implementation specific.

aspm_calc_l12_info() tries to compute these values, but I don't think
it can do it correctly, and we have a related problem report where
BIOS set them correctly and then Linux came along and messed them up:

  https://lore.kernel.org/r/20250407155742.GA178541@bhelgaas

I think the only thing we can do with L1.2 is rely on values already
programmed by BIOS.  There is a _DSM method for Latency Tolerance
Reporting configuration (PCI Firmware r3.3, sec 4.6.6), but AFAICT it
only addresses the LTR Capability in upstream ports, not the
T_POWER_ON and Common_Mode_Restore_Time values.

_HPX Type 2 and 3 records (ACPI r6.5, sec 6.2.9.3 and 6.2.9.4) are
quite general and maybe there's a way to use them to configure the
L1.2 parameters, but I am skeptical that these have been implemented
in Linux and BIOSes.  At least, I don't recall any reports of problems
or of them being used.

Maybe something like this patch can be done for ASPM *except* for
L1.2?

Bjorn

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Kenneth Crudup]

What about a quirk, or whitelist?

That's hacky AF, but that way for systems that this is known to work on, 
it could be enabled?

-K


On 5/12/25 14:09, Bjorn Helgaas wrote:
> [+cc Sergey, Nirmal, Jonathan, Jian-Hong, David]
> 
> On Sun, May 11, 2025 at 01:10:24PM -0700, Kenneth R. Crudup wrote:
>>
>> Hello Raphael,
>>
>> For almost two years now, I've been trying to get patches from Ubuntu that
>> enable ASPM for devices behind Intel's VMD, necessary to get full lower-power
>> states (including very-reduced power usage during s0ix sleep) on my Alderlake
>> (et al.) laptop, upstreamed into mainline.
>>
>> One such thread: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/
>>
>> Since the original set of patches on this, most of the work has been pushed
>> upstream, with only this last patch required to get fully into the "CPU%LPI"
>> and "SYS%LPI" (names according to "turbostat") states.
>>
>> I'm surprised that with the number of VMD-enabled laptops out there (which I
>> had to keep on so I could dual-boot into Win11 (the disk geometry changes if
>> I disable it, rendering the Win11 partition useless)), that there haven't been
>> many reports of excessive power usage in Linux during sleep; perhaps because
>> many installations are running stock Ubuntu kernels (where I assume variants
>> of this patch remain) it isn't an issue, but I do believe having this upstreamed
>> is still valuable.
>>
>> I don't have the resources you've got to test this fully for regressions, nor
>> the expertise getting a patch into the kernel, so I'd like to again bring this
>> up for discussion (hence the phone-book of a CC: here).
>>
>> If there's anything I can do to help get this done, please let me know.
>>
>> Thank you,
>>
>> -Kenneth Crudup
>>
>> -- 
>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> 
>>  From ee3618a598a261bbc8a875557d42d6dbbbc4cdd0 Mon Sep 17 00:00:00 2001
>> From: "Kenneth R. Crudup" <kenny@panix.com>
>> Date: Fri, 13 Dec 2024 15:28:42 -0800
>> Subject: [PATCH] PCI/ASPM: Fixup ASPM for VMD bridges
>>
>> Effectively a squashed commit of:
>> UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
>> UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
>> UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>> ---
>>   drivers/pci/pcie/aspm.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 29fcb0689a91..fdc1ce2755ff 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -788,6 +788,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>>   		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
>>   }
>>   
>> +/*
>> + * BIOS may not be able to access config space of devices under VMD domain, so
>> + * it relies on software to enable ASPM for links under VMD.
>> + */
>> +static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
>> +{
>> +       struct pci_bus *bus = pdev->bus;
>> +       struct device *dev;
>> +       struct pci_driver *pdrv;
>> +
>> +       if (!pci_is_root_bus(bus))
>> +               return false;
>> +
>> +       dev = bus->bridge->parent;
>> +       if (dev == NULL)
>> +               return false;
>> +
>> +       pdrv = pci_dev_driver(to_pci_dev(dev));
>> +       if (pdrv == NULL || strcmp("vmd", pdrv->name))
>> +               return false;
>> +
>> +       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
>> +       return true;
>> +}
>> +
>>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>   {
>>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
>> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>   	}
>>   
>>   	/* Save default state */
>> -	link->aspm_default = link->aspm_enabled;
>> +	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
>> +		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
> 
> PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
> this potentially enables L1.2.  The L1.2 configuration depends on
> T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
> design and are not discoverable by the kernel.  See PCIe r6.0, sec
> 5.5.4:
> 
>    The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
>    to the appropriate values based on the components and AC coupling
>    capacitors used in the connection linking the two components. The
>    determination of these values is design implementation specific.
> 
> aspm_calc_l12_info() tries to compute these values, but I don't think
> it can do it correctly, and we have a related problem report where
> BIOS set them correctly and then Linux came along and messed them up:
> 
>    https://lore.kernel.org/r/20250407155742.GA178541@bhelgaas
> 
> I think the only thing we can do with L1.2 is rely on values already
> programmed by BIOS.  There is a _DSM method for Latency Tolerance
> Reporting configuration (PCI Firmware r3.3, sec 4.6.6), but AFAICT it
> only addresses the LTR Capability in upstream ports, not the
> T_POWER_ON and Common_Mode_Restore_Time values.
> 
> _HPX Type 2 and 3 records (ACPI r6.5, sec 6.2.9.3 and 6.2.9.4) are
> quite general and maybe there's a way to use them to configure the
> L1.2 parameters, but I am skeptical that these have been implemented
> in Linux and BIOSes.  At least, I don't recall any reports of problems
> or of them being used.
> 
> Maybe something like this patch can be done for ASPM *except* for
> L1.2?
> 
> Bjorn
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Russell Haley]

On 5/12/25 4:09 PM, Bjorn Helgaas wrote:

>>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>  {
>>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>  	}
>>  
>>  	/* Save default state */
>> -	link->aspm_default = link->aspm_enabled;
>> +	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
>> +		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
> 
> PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
> this potentially enables L1.2.  The L1.2 configuration depends on
> T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
> design and are not discoverable by the kernel.  See PCIe r6.0, sec
> 5.5.4:
> 
>   The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
>   to the appropriate values based on the components and AC coupling
>   capacitors used in the connection linking the two components. The
>   determination of these values is design implementation specific.

Does that apply to VMD?  As far as I know it's not an actual physical
PCIe device.

- Russell

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Kenneth Crudup]

Raphael, any input?

Thanks,

-Kenny

On 5/14/25 18:23, Russell Haley wrote:
> 
> 
> On 5/12/25 4:09 PM, Bjorn Helgaas wrote:
> 
>>>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>>   {
>>>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
>>> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>>   	}
>>>   
>>>   	/* Save default state */
>>> -	link->aspm_default = link->aspm_enabled;
>>> +	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
>>> +		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
>>
>> PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
>> this potentially enables L1.2.  The L1.2 configuration depends on
>> T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
>> design and are not discoverable by the kernel.  See PCIe r6.0, sec
>> 5.5.4:
>>
>>    The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
>>    to the appropriate values based on the components and AC coupling
>>    capacitors used in the connection linking the two components. The
>>    determination of these values is design implementation specific.
> 
> Does that apply to VMD?  As far as I know it's not an actual physical
> PCIe device.
> 
> - Russell
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Rafael J. Wysocki]

On Fri, May 23, 2025 at 9:21 PM Kenneth Crudup <kenny@panix.com> wrote:
>
> Raphael, any input?

Well, Bjorn's concerns are valid AFAICS.

> On 5/14/25 18:23, Russell Haley wrote:
> >
> >
> > On 5/12/25 4:09 PM, Bjorn Helgaas wrote:
> >
> >>>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >>>   {
> >>>     struct pci_dev *child = link->downstream, *parent = link->pdev;
> >>> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >>>     }
> >>>
> >>>     /* Save default state */
> >>> -   link->aspm_default = link->aspm_enabled;
> >>> +   link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
> >>> +           PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
> >>
> >> PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
> >> this potentially enables L1.2.  The L1.2 configuration depends on
> >> T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
> >> design and are not discoverable by the kernel.  See PCIe r6.0, sec
> >> 5.5.4:
> >>
> >>    The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
> >>    to the appropriate values based on the components and AC coupling
> >>    capacitors used in the connection linking the two components. The
> >>    determination of these values is design implementation specific.
> >
> > Does that apply to VMD?  As far as I know it's not an actual physical
> > PCIe device.

The devices on the VMD bus are physical PCIe devices, but they need to
be accessed in a special way and the BIOS doesn't have access to them.

I would try to replace the PCIE_LINK_STATE_ASPM_ALL in the patch with

PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1 |
PCIE_LINK_STATE_L1_1_PCIPM

and see how far it gets you.  Please let me know how it goes.

In the meantime, I'll try to find somebody who can pick this up.

Thanks!

Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Kenneth Crudup]

On 5/23/25 12:44, Rafael J. Wysocki wrote:

> I would try to replace the PCIE_LINK_STATE_ASPM_ALL in the patch with
> 
> PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1 |
> PCIE_LINK_STATE_L1_1_PCIPM

I could get "Pkg%pc8" with these but no deeper until I'd added 
"PCIE_LINK_STATE_L1_2" to the mix.

(I didn't see any power regressions after that (including 
suspend/resume, but I didn't try for a longer time), so didn't re-add 
"PCIE_LINK_STATE_L1_2_PCIPM", so that one may not be needed.)


-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA