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

[Christian König]

On 9/21/26 13:04, Prica, Nikola wrote:
> On 9/18/2026 7:07 PM, Bjorn Helgaas wrote:
>> On Fri, Sep 18, 2026 at 02:07:46PM +0200, Christian König wrote:
>>> On 9/18/26 11:28, Nikola Prica wrote:
>>>> From: Nikola Prica <nikola.prica@xxxxxxx>
>>>>
>>>> pci_enable_atomic_ops_to_root() currently fails when no Root Port is
>>>> visible. That is common in passthrough guests (ESXi, Hyper-V): the
>>>> endpoint is assigned to the VM, but the guest topology has no Root
>>>> Port above it.
>>>>
>>>> In those setups the hypervisor may already have enabled AtomicOp
>>>> Requester Enable on the device. If PCI_EXP_DEVCTL2_ATOMIC_REQ is set,
>>>> treat AtomicOps as already enabled and return success instead of
>>>> failing the Root Port walk.
>>>>
>>>> Fixes: 1ae8c4ce1570 ("PCI: Enable AtomicOps only if Root Port supports them")
>>>> Reviewed-by: Christian König <christian.koenig@xxxxxxx>
>>>> Reviewed-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
>>>> Tested-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
>>>> Signed-off-by: Nikola Prica <nikola.prica@xxxxxxx>
>>>
>>> CC: stable@xxxxxxxxxxxxxxx # 7.1+
>>>
>>> Bjorn do you want to pick that up? Alternative I can push it through
>>> drm-misc-fixes.
>>>
>>> Initially I thought that this is a new feature, but it turned out to
>>> be an regression and we need to get it fixed ASAP.
>>
>> Let me just talk through this to make sure I understand it:
>>
>> Prior to 1ae8c4ce1570, pci_enable_atomic_ops_to_root() enabled
>> PCI_EXP_DEVCTL2_ATOMIC_REQ unless a switch port didn't support
>> AtomicOp routing, a switch upstream port blocked AtomicOp egress, or a
>> Root Port didn't support the requested sizes.
>>
> 
> To be technically correct, some hypervisors do not allow guest VM to
> set the PCI_EXP_DEVCTL2_ATOMIC_REQ. So there were cases where this
> set command didn't get through. But drivers relied on return value
> of pci_enable_atomic_ops_to_root(). It may be good to read it back
> to make sure that set went through.

Well that behavior of the hypervisor is a bit questionable.

I understand why the hypervisor blocks such configuration changes, but it is essentially a recipe for trouble.

So I agree that the Linux kernel should be as defensive as possible and double check if enabling the feature really worked as expected.

>> 1ae8c4ce1570 added the requirement that the Root Port be visible,
>> which fixed an s390 case where pci_enable_atomic_ops_to_root() enabled
>> AtomicOps when the Root Port did not support them but was not visible
>> to the kernel.
>>
>> IIUC the regression is on systems where the Root Port is not visible
>> but *does* support AtomicOps.  Prior to 1ae8c4ce1570, we would have
>> enabled AtomicOps in the endpoint and returned success.  After
>> 1ae8c4ce1570, we return -EINVAL because we can't see the Root Port to
>> verify its support, so the driver thinks it can't use AtomicOps.
>>
>> This patch fixes the regression by assuming that if we can't see the
>> Root Port but the endpoint already has AtomicOps enabled, the
>> hypervisor or host kernel that *can* see the Root Port has already
>> verified its AtomicOps support, so all we have to do is return success
>> so the driver can use them.
>>
>> I'm happy to merge this for v7.3, and I think we should add something
>> like this to the commit log to make it clear that it's a regression
>> worthy of a post-merge window fix:
>>
>>    After 1ae8c4ce1570 ("PCI: Enable AtomicOps only if Root Port
>>    supports them"), pci_enable_atomic_ops_to_root() always fails if the
>>    Root Port is not visible.  On systems where the Root Port is not
>>    visible but *does* support AtomicOps, this is a regression: prior to
>>    1ae8c4ce1570, it enabled AtomicOps in the endpoint and returned
>>    success.
>>
>> Is there any problem report for the regression?  I don't see anything
>> at https://linux-regtracking.leemhuis.info/regzbot/mainline/, so my
>> guess is no.
>>
> 
> Yes. That is correct. I'll send new v4 patch with updated commit log
> that appends your suggestion.
> 
> 
> This patch fixes a regression introduced in v7.0-rc1-2-g1ae8c4ce1570.
> 
> #regzbot introduced: 1ae8c4ce1570
> #regzbot title: Breaks atomics operations for headless passthrough devices
> 
> 
> Hope that this will be enough to mark it as regression. If not please let
> me know.

I don't think we have a public visible bug report on the issue anywhere, so that should probably do it.

> One more question though, we encountered this issue with 6.8.0-138-generic
> kernel, we figured out the regression point is 6.8.0-135-generic. Since our
> validation teams are using older kernels too, is it safe to assume that
> fix will be backported in new release for 6.8.0-*-generic version?

That is what the Fixes: tag takes care of. Most likely the kernel 6.8.0-135-generic doesn't work because the offending commit was back ported as fix.

So when the Fixes tag identifies that commit the stable maintainers should backport it as well.

Regards,
Christian.

> 
> Regards,
> Nikola
> 
> 
>>>> ---
>>>> v3: Add Fixes tag
>>>> v2: Be more strict and only apply logic in headless device case.
>>>> ---
>>>>   drivers/pci/pci.c | 12 +++++++++++-
>>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index b2879a6be5f8..62729ade496f 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3769,8 +3769,18 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
>>>>       }
>>>>         root = pcie_find_root_port(dev);
>>>> -    if (!root)
>>>> +    if (!root) {
>>>> +        /*
>>>> +         * A hypervisor may expose a headless topology with no
>>>> +         * visible root port.  If it has already set AtomicOp
>>>> +         * Requester Enable, there is nothing more to do.
>>>> +         */
>>>> +        pcie_capability_read_dword(dev, PCI_EXP_DEVCTL2, &ctl2);
>>>> +        if (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_REQ)
>>>> +            return 0;
>>>> +
>>>>           return -EINVAL;
>>>> +    }
>>>>         pcie_capability_read_dword(root, PCI_EXP_DEVCAP2, &cap);
>>>>       if ((cap & cap_mask) != cap_mask)
>>>
>