Message-ID: <EF44D08B-F7CC-41A1-A7AE-39A392182322@oracle.com>
Date: Mon, 12 Jan 2026 12:07:01 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, Sinan Kaya <okaya@...eaurora.org>,
        Casey
 Leedom <leedom@...lsio.com>, Ashok Raj <ashok.raj@...el.com>,
        "David S.
 Miller" <davem@...emloft.net>,
        dingtianhong <dingtianhong@...wei.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Alexander Duyck
	<alexanderduyck@...com>,
        "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>,
        "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI/ACPI: Do not fiddle with ExtTag and RO in
 program_hpx_type2



> On 8 Jan 2026, at 18:53, Bjorn Helgaas <helgaas@...nel.org> wrote:
> 
> On Wed, Jan 07, 2026 at 05:22:05PM -0600, Bjorn Helgaas wrote:
>> 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.
> 
> I poked around in the old specs and found that _HPX was added in ACPI
> r3.0, where the Type 2 record only contained AER registers
> (Uncorrectable Error Mask, Uncorrectable Error Severity, and
> Correctable Error Mask), and the description said this:
> 
>  OSPM will only evaluate _HPX with Setting Record – Type 2 if OSPM is
>  not controlling the PCI Express Advanced Error Reporting capability.
> 
> ACPI r4.0 changed _HPX to the current r6.6 description and added all
> the registers included in r6.6 (without changing the record revision).
> 
> So I think we should do what I proposed above (only do
> program_hpx_type2() if the OS owns PCIe hotplug but not AER, and only
> update the CERE/NFERE/FERE/URRE bits in Device Control).  I would
> probably log the Link Control values if they're set, but otherwise
> ignore them.

OK. I must admit I was a little puzzled when I saw that program_hpx_type2() was called unconditionally. Let me take a stab on a v2 for this.


Thxs, Håkon

> 
>>> 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@...cle.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

Powered by blists - more mailing lists