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] [thread-next>] [day] [month] [year] [list]
Message-ID: <A6A85012-A9E5-46D8-8BB5-EEC20898B6C6@oracle.com>
Date: Fri, 16 Jan 2026 10:10:43 +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>,
        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>
Subject: Re: [PATCH] PCI/ACPI: Confine program_hpx_type2 to the AER bits

> On Thu, Jan 15, 2026 at 03:39:21PM +0000, Haakon Bugge wrote:
>> Thanks for the review, Bjørn!
>> ...
> 
>>>> + hpx->pci_exp_devctl_or &= ~(PCI_EXP_DEVCTL_RELAX_EN   |
>>>> +     PCI_EXP_DEVCTL_PAYLOAD    |
>>>> +     PCI_EXP_DEVCTL_EXT_TAG    |
>>>> +     PCI_EXP_DEVCTL_PHANTOM    |
>>>> +     PCI_EXP_DEVCTL_AUX_PME    |
>>>> +     PCI_EXP_DEVCTL_NOSNOOP_EN |
>>>> +     PCI_EXP_DEVCTL_READRQ     |
>>>> +     PCI_EXP_DEVCTL_BCR_FLR);
>>>> 
>>> Instead of listing the bits we *don't* want to touch, I think we
>>> should explicitly *include* CERE, NFERE, FERE, URRE.  Maybe we should
>>> move the PCI_EXP_AER_FLAGS #define to drivers/pci/pci.h so we could
>>> use it directly, e.g.,
>>> 
>>> hpx->pci_exp_devctl_and |= ~PCI_EXP_AER_FLAGS;
>>> hpx->pci_exp_devctl_or &= PCI_EXP_AER_FLAGS;
>> 
>> Good idea. But what about moving it to include/uapi/linux/pci_regs.h
>> and also rename it from PCI_EXP_AER_FLAGS to PCI_EXP_DEVCTL_AER, to
>> match the convention for DEVCTL in pci_regs.h?
> 
> I suggested drivers/pci/pci.h because (so far) the only need for
> PCI_EXP_AER_FLAGS is in drivers/pci, and that set of flags seems like
> an OS policy.  Most of pci_regs.h is basically translating the PCI
> spec into #defines, without any real usage or policy parts.  I'm not
> sure whether PCI_EXP_AER_FLAGS would be useful to userspace.

Well. My thinking was that since PCI_EXP_DEVCTL_{CERE,NFERE,FERE,URRE} is already present in pci_regs.h, the OR of them won't hurt and we gain consistency in naming:

@@ -505,6 +505,7 @@
 #define  PCI_EXP_DEVCAP_FLR     0x10000000 /* Function Level Reset */
 #define  PCI_EXP_DEVCAP_TEE     0x40000000 /* TEE I/O (TDISP) Support */
 #define PCI_EXP_DEVCTL         0x08    /* Device Control */
+#define  PCI_EXP_DEVCTL_AER    0x000f  /* AER Flags */
 #define  PCI_EXP_DEVCTL_CERE   0x0001  /* Correctable Error Reporting En. */
 #define  PCI_EXP_DEVCTL_NFERE  0x0002  /* Non-Fatal Error Reporting Enable */
 #define  PCI_EXP_DEVCTL_FERE   0x0004  /* Fatal Error Reporting Enable */

I am fine either way.

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

> We should audit PCI_EXP_LNKCTL_RCB usage to be sure that if we remove
> this code, PCI_EXP_LNKCTL_RCB will still be set whenever it needs to
> be set.  If we rely on the existence of an _HPX type 2 record for it
> to be set, that would be completely wrong.

I'l keep the RCB tweaking as is and add the pci_info() logging if the type 2 record attempts to change any bits in the Link Control register.


Thxs, Håkon