linux-kernel - Re: [PATCH] PCI/ACPI: Confine program_hpx

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] [day] [month] [year] [list]

Message-ID: <D575500A-F203-46A3-A7AE-B9F45B396115@oracle.com>
Date: Mon, 19 Jan 2026 15:52:01 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Johannes Thumshirn <morbidrsa@...il.com>
CC: Bjorn Helgaas <helgaas@...nel.org>,
        "Rafael J. Wysocki"
	<rafael@...nel.org>, Len Brown <lenb@...nel.org>,
        Bjorn Helgaas
	<bhelgaas@...gle.com>,
        Greg Kroah-Hartman <gregkh@...e.de>,
        Kenji Kaneshige
	<kaneshige.kenji@...fujitsu.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>,
        Johannes Thumshirn <jth@...nel.org>,
        Myron
 Stowe <myron.stowe@...hat.com>
Subject: Re: [PATCH] PCI/ACPI: Confine program_hpx_type2 to the AER bits



> On 19 Jan 2026, at 12:56, Johannes Thumshirn <morbidrsa@...il.com> wrote:
> 
> On 1/16/26 10:11 PM, Bjorn Helgaas wrote:
>> [+cc Johannes (author of e42010d8207f ("PCI: Set Read Completion
>> Boundary to 128 iff Root Port supports it (_HPX)"), Myron; start of
>> thread:
>> https://lore.kernel.org/r/20260113171522.3446407-1-haakon.bugge@oracle.com]
>> 
>> On Fri, Jan 16, 2026 at 10:10:43AM +0000, Haakon Bugge wrote:
>>>> On Thu, Jan 15, 2026 at 03:39:21PM +0000, Haakon Bugge wrote:
>>>>> Thanks for the review, Bjørn!
>>>>> ...
>> I should have mentioned this earlier, but I think the commit log
>> should include something about the problem this change fixes.  I
>> assume that the current code changes ExtTag and/or RO, and that causes
>> something bad.  That's what is motivating this change.

Indeed, the commit log contains:

    So, in order to preserve the non-AER bits in PCIe Device Control, in
    particular the performance sensitive ExtTag and RO, we make sure
    program_hpx_type2() if called, doesn't modify any non-AER bits.


Should I be more specific?

I observed ~15% higher I/O rates when ExtTag and RO were not reset on the system that had the "broken" type 2 record.


>>>>>>> if (pcie_cap_has_lnkctl(dev)) {
>>>>>>> + u16 lnkctl;
>>>>>>> 
>>>>>>> - /*
>>>>>>> -  * If the Root Port supports Read Completion Boundary of
>>>>>>> -  * 128, set RCB to 128.  Otherwise, clear it.
>>>>>>> -  */
>>>>>>> - hpx->pci_exp_lnkctl_and |= PCI_EXP_LNKCTL_RCB;
>>>>>>> - hpx->pci_exp_lnkctl_or &= ~PCI_EXP_LNKCTL_RCB;
>>>>>>> - if (pcie_root_rcb_set(dev))
>>>>>>> - hpx->pci_exp_lnkctl_or |= PCI_EXP_LNKCTL_RCB;
>>>>>>> -
>>>>>>> - pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
>>>>>>> - ~hpx->pci_exp_lnkctl_and, hpx->pci_exp_lnkctl_or);
>>>>>>> + pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
>>>>>>> + if (lnkctl)
>>>>>>> + pci_warn(dev, "Some bits in PCIe Link Control are set: 0x%04x\n",
>>>>>>> +  lnkctl);
>>>>>>> 
>>>>>> Sorry, I wasn't clear about this.  I meant that we could log the
>>>>>> LNKCTL AND/OR values from _HPX, not the values from
>>>>>> PCI_EXP_LNKCTL itself.  There will definitely be bits set in
>>>>>> PCI_EXP_LNKCTL in normal operation, which is perfectly fine.
>>>>>> 
>>>>>> But if pci_exp_lnkctl_and or pci_exp_lnkctl_or are non-zero, the
>>>>>> platform is telling us to do something, and we're ignoring it.
>>>>>> *That's* what I think we might want to know about.  pci_info()
>>>>>> is probably sufficient; the user doesn't need to *do* anything
>>>>>> with it, I just want it in case we need to debug an issue.
>>>>> My bad, Yes, that makes more sense to me. And, you're OK with
>>>>> removing the RCB tweaking as well?
>>>> Good question.  My hope is that the code here is just to make sure
>>>> that we don't *clear* PCI_EXP_LNKCTL_RCB when we want it set but a
>>>> type 2 record might clear it by mistake.
>>> Commit e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff
>>> Root Port supports it (_HPX)") fixes the "opposite" case, where _HPX
>>> sets the RCB even though the RC does not support it. That commit
>>> removes any RCB setting from the type 2 record from the equation,
>>> and sets RCB if the RC has the bit set. And to me, that seems to be
>>> the correct behaviour.
>> Thanks for digging into that.  You're right that it looks like
>> e42010d8207f ("PCI: Set Read Completion Boundary to 128 iff Root Port
>> supports it (_HPX)") was motivated by a machine with a Root Port with
>> PCI_EXP_LNKCTL_RCB cleared, but an _HPX record telling us to set
>> PCI_EXP_LNKCTL_RCB.
> 
> IIRC (this is nearly 10 years old) that's been the case. But back then it clearly was a bios issue, but we decided to fix it in the kernel if my memory serves me well.

Yes, looks like we are aligned. Will send a v2, which will be a two commit series, and we take it from there.


Thxs, Håkon

>