Re: [PATCH] PCI: Accept AtomicOps already enabled by the hypervisor

["Prica, Nikola" <nikola.prica@amd.com>]

Ok. That makes sense. I'll address it in v2 and incorporate Christians comment too.

On 9/3/2026 2:12 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unconditionally returning success when ATOMIC_REQ is already set bypasses root port and bridge capability validation.
> --
> 
> commit 174dddecfad7749ccba344bb23492b342569e86c
> Author: Nikola Prica <nikola.prica@amd.com>
> 
> PCI: Accept AtomicOps already enabled by the hypervisor
> 
> This commit modifies pci_enable_atomic_ops_to_root() to check if the
> PCI_EXP_DEVCTL2_ATOMIC_REQ bit is already set by a hypervisor. If it is, the
> function returns success early to avoid failing in passthrough topologies
> without a visible Root Port.
> 
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b2879a6be5f80..7c62c7f317572 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3768,6 +3768,14 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
>>                return -EINVAL;
>>        }
>>
>> +     /*
>> +      * Some hypervisors already set AtomicOp Requester Enable.  If it
>> +      * is already set, there is nothing more to do.
>> +      */
>> +     pcie_capability_read_dword(dev, PCI_EXP_DEVCTL2, &ctl2);
>> +     if (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_REQ)
>> +             return 0;
> 
> [Severity: High]
> Will this unconditional early return bypass the capability validation for newly
> requested atomic sizes?
> 
> When drivers or subsystems sequentially check for atomic support, they call
> pci_enable_atomic_ops_to_root() for different sizes. For example, in
> drivers/vfio/pci/vfio_pci_core.c:vfio_pci_info_atomic_cap() (around line
> 1059), the code sequentially checks for 32-bit and 64-bit atomic support:
> 
> vfio_pci_core.c:vfio_pci_info_atomic_cap() {
>      ...
>      if ((devcap2 & PCI_EXP_DEVCAP2_ATOMIC_COMP64) &&
>          !pci_enable_atomic_ops_to_root(pdev, PCI_EXP_DEVCAP2_ATOMIC_COMP64))
>          cap.flags |= VFIO_PCI_ATOMIC_COMP64;
>      ...
> }
> 
> A similar sequential check occurs in
> drivers/infiniband/hw/bnxt_re/qplib_res.c:bnxt_qplib_determine_atomics()
> (around line 1001).
> 
> The first successful call (e.g., for COMP32) validates the topology and sets
> the ATOMIC_REQ bit on the endpoint. If a subsequent call is made for a larger
> size (like COMP64), this new logic sees the ATOMIC_REQ bit is already set and
> instantly returns success.
> 
> By returning early, doesn't this bypass the cap_mask checks on the root port
> and bridges for the larger atomic size?
> 
> Could this cause the kernel to incorrectly advertise 64-bit or 128-bit atomic
> support to VMs or drivers when the root port only supports 32-bit? If a driver
> or VM attempts to use these unsupported sizes, the root port would treat them
> as an Unsupported Request (UR), potentially triggering a fatal AER error and
> crashing the system or terminating the VM.
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260903121218.4955-1-nikprica@amd.com?part=1