Re: [PATCH] PCI/ASPM: Allow ASPM enablement for devices behind Intel VMD

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 11, 2025 at 09:06:27AM -0700, David Box wrote:
> On Fri, Jul 11, 2025 at 05:49:03PM +0300, Ilpo Järvinen wrote:
> > On Fri, 11 Jul 2025, David Box wrote:
> > > On Thu, Jul 10, 2025 at 09:53:18PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Jul 10, 2025 at 3:16 AM David E. Box
> > > > <david.e.box@linux.intel.com> wrote:
> > > > >
> > > > > Devices behind Intel's Volume Management Device (VMD) controller reside on
> > > > > a synthetic PCI hierarchy that is intentionally hidden from ACPI and
> > > > > firmware. As such, BIOS does not configure ASPM for these devices, and the
> > > > > responsibility for link power management, including ASPM and LTR, falls
> > > > > entirely to the VMD driver.
> > > > >
> > > > > However, the current PCIe ASPM code prevents ASPM configuration when the
> > > > > ACPI_FADT_NO_ASPM flag is set, disallowing OS management. This leaves ASPM
> > > > > permanently disabled for these devices, contrary to the platform's design
> > > > > intent.
> > > > >
> > > > > Introduce a callback mechanism that allows the VMD driver to enable ASPM
> > > > > for its domain, bypassing the global ACPI_FADT_NO_ASPM restriction that is
> > > > > not applicable in this context. This ensures that devices behind VMD can
> > > > > benefit from ASPM savings as originally intended.
> ...

> > > > > +       /*
> > > > > +        * Devices behind Intel VMD operate on a synthetic PCI bus that BIOS
> > > > > +        * and ACPI do not enumerate or configure. ASPM for these devices must
> > > > > +        * be managed by the VMD driver itself, independent of global ACPI ASPM
> > > > > +        * constraints. This callback mechanism allows selective ASPM
> > > > > +        * enablement for such domains.
> > > > > +        */
> > > > > +       vmd_aspm_default = pci_get_vmd_link_state(parent);
> > > > > +
> > > > >         /* Save default state */
> > > > > -       link->aspm_default = link->aspm_enabled;
> > > > > +       if (vmd_aspm_default < 0)
> > > > > +               link->aspm_default = link->aspm_enabled;
> > > > > +       else
> > > > > +               link->aspm_default = vmd_aspm_default;
> > > > 
> > > > Well, this is not nice.
> > > > 
> > > > First off, it adds VMD-specific stuff to otherwise generic
> > > > ASPM code.  Second, it doesn't actually do anything about the
> > > > aspm_disabled checks all over the place, so they will still
> > > > trigger even though ASPM will be effectively enabled for
> > > > devices on the VMD bus.
> > > 
> > > I agree that it's not nice to be mixing VMD specific code here.
> > > It's a working bad solution to come up with a working good
> > > solution :)
> > > 
> > > The reason this patch works is that the aspm_disabled checks
> > > only gate OS-controlled ASPM configuration. They don't affect
> > > the BIOS default values, which are what we're setting in
> > > link->aspm_default. The ASPM code uses link->aspm_default as the
> > > fallback when ASPM is globally disabled, which is exactly what
> > > we want for devices behind VMD where the driver, not BIOS,
> > > effectively is the platform provider of the defaults.
> > 
> > Oh, this was a big news to me.
> > 
> > So what you're saying is that if aspm_disabled is set,
> > ->aspm_disable cannot be set and thus pcie_config_aspm_link() that
> > is not gated by aspm_disabled can alter ASPM state despite OS not
> > having ASPM control???
> 
> Yes, that’s correct. Bjorn can confirm, but I believe this is by
> design. The aspm_disabled flag is really a bit of a misnomer. It
> probably should have been called something like os_aspm_disabled.
> The intent as I understand it is that, when disallowed, the OS
> cannot select or manage the active ASPM policy, but it can still
> configure the link to match the BIOS provided policy.

Right, there's a long ugly history of this, here's the pcie_no_aspm()
comment:

  /*
   * Disabling ASPM is intended to prevent the kernel from modifying
   * existing hardware state, not to clear existing state. To that end:
   * (a) set policy to POLICY_DEFAULT in order to avoid changing state
   * (b) prevent userspace from changing policy
   */
  if (!aspm_force) {
	  aspm_policy = POLICY_DEFAULT;
	  aspm_disabled = 1;
  }

which came from 3c076351c402 ("PCI: Rework ASPM disable code").  All
the ASPM disable, force, support_enabled, policy, etc flags make this
ugly and impossible to read.  I guess renaming might help a little.

> In other words, ASPM isn’t fully disabled. It’s just not under OS
> control. The BIOS values, reflected in link->aspm_default, remain
> valid and pcie_config_aspm_link() can apply them regardless of the
> aspm_disabled setting.
> 
> > ...That's really odd logic which the ASPM driver seems to be full of.

+10

Also the ugliness of pcie_aspm_init_link_state() being called
completely out of the usual enumeration flow.  And the parallel device
hierarchy maintained in struct pcie_link_state.  And config options to
set the default policy.  Ugh.

I hope we can avoid adding VMD-specific code in aspm.c.

Bjorn