Re: [PATCH] PCI/ACPI: Do not fiddle with ExtTag and RO in program_hpx_type2

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Alexander's @fb.com address]

On Fri, Dec 05, 2025 at 03:28:29PM +0100, Håkon Bugge wrote:
> After commit 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if
> supported"), the kernel controls enablement of extended tags
> (ExtTag). Similar, after commit a99b646afa8a ("PCI: Disable PCIe
> Relaxed Ordering if unsupported"), the kernel controls the relaxed
> ordering bit (RO), in the sense that the kernel keeps the bit set (if
> already set) unless the RC does not support it.
> 
> On some platforms, when program_hpx_type2() is called and the _HPX
> object's Type2 records are, let's say, dubious, we may end up
> resetting ExtTag and RO, although they were explicit set or kept set
> by the OSPM [1].

This text about Type 2 records in ACPI r6.6, sec 6.2.10.3, seems a
little ambiguous to me:

  A PCI Express-aware OS that has assumed ownership of native hot plug
  (via _OSC) but does not support or does not have ownership of the
  AER register set must use the data values returned by the _HPX
  object’s Type 2 record to program the AER registers of a hot-added
  PCI Express device. However, since the Type 2 record also includes
  register bits that have functions other than AER, OSPM must ignore
  values contained within this setting record that are not applicable.

If I squint, I can read that as meaning that Type 2 is really there
just for AER, and the OS:

  - should only use a Type 2 record when it owns PCIe native hotplug
    (native_pcie_hotplug) but does not own AER (!native_aer),

  - should only program AER registers, and

  - should *ignore* bits unrelated to AER.

Most of the registers in Type 2 are in the AER Capability.  Device
Control is in the PCIe Capability, but if _OSC has granted AER
ownership to the OS, that includes the Error Reporting Enable bits in
Device Control (there's a PCI Firmware spec ECN to this effect:
https://members.pcisig.com/wg/Firmware/document/20514).

Type 2 does include Link Control, which is in the PCIe Capability and
doesn't seem related to AER, so maybe I'm on the wrong track.  But if
Type 2 was intended to handle things *other* than AER, I would think
the PCIe Capability Slot Control and Root Control would have been
included.

So *maybe* program_hpx_type2() should mask out everything from
pci_exp_devctl_or except PCI_EXP_DEVCTL_CERE, PCI_EXP_DEVCTL_NFERE,
PCI_EXP_DEVCTL_FERE, and PCI_EXP_DEVCTL_URRE?  I have no idea what we
would do with Link Control though.

I wish I knew the history of this, but I don't.

> The Advanced Configuration and Power Interface (ACPI) Specification
> version 6.6 has a provision that gives the OSPM the ability to
> control these bits any way. In a note in section 6.2.9, it is stated:
> 
> "OSPM may override the settings provided by the _HPX object's Type2
> record (PCI Express Settings) or Type3 record (PCI Express Descriptor
> Settings) when OSPM has assumed native control of the corresponding
> feature."
> 
> So, in order to preserve the increased performance of ExtTag and RO on
> platforms that support any of these, we make sure program_hpx_type2()
> doesn't reset them.
> 
> [1] Operating System-directed configuration and Power Management
> 
> Fixes: 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported")
> Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/pci/pci-acpi.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 9369377725fa0..6a2ae1b821732 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -324,15 +324,18 @@ static void program_hpx_type2(struct pci_dev *dev, struct hpx_type2 *hpx)
>  		return;
>  	}
>  
> -	/*
> -	 * Don't allow _HPX to change MPS or MRRS settings.  We manage
> -	 * those to make sure they're consistent with the rest of the
> -	 * platform.
> +	/* Don't allow _HPX to change MPS, MRRS, ExtTag, or RO
> +	 * settings.  We manage those to make sure they're consistent
> +	 * with the rest of the platform.
>  	 */
>  	hpx->pci_exp_devctl_and |= PCI_EXP_DEVCTL_PAYLOAD |
> -				    PCI_EXP_DEVCTL_READRQ;
> +				   PCI_EXP_DEVCTL_READRQ  |
> +				   PCI_EXP_DEVCTL_EXT_TAG |
> +				   PCI_EXP_DEVCTL_RELAX_EN;
>  	hpx->pci_exp_devctl_or &= ~(PCI_EXP_DEVCTL_PAYLOAD |
> -				    PCI_EXP_DEVCTL_READRQ);
> +				    PCI_EXP_DEVCTL_READRQ  |
> +				    PCI_EXP_DEVCTL_EXT_TAG |
> +				    PCI_EXP_DEVCTL_RELAX_EN);
>  
>  	/* Initialize Device Control Register */
>  	pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> -- 
> 2.43.5
>