This RFC proposes to allow a vfio-pci device to manipulate the PCI
Express capability of an associated root port to enable Atomic Op
completer support as equivalent to host capabilities.  This would
dynamically change capability bits in the config space of the root
port on realize and exit of the vfio-pci device under the following
condiations:

 - The vfio-pci device is directly connected to the root port and
   the root port implements a v2 PCIe capability, thereby supporting
   the DEVCAP2 register.

 - The vfio-pci device is exposed as a single-function device.

 - Atomic completer support is not otherwise reported on the root port.

 - The vfio-pci device reports the VFIO_DEVICE_INFO_CAP_PCI_ATOMIC_COMP
   capability with a non-zero set of supported atomic completer widths.

This proposal aims to avoid complications of reporting Atomic Ops
Routing, which can easily escalate into invalid P2P paths.  We also
require a specific VM configuration to avoid dependencies between
devices which may be sourced from dissimilar host paths.

While it's not exactly standard practice to modify root port device
capabilities runtime, it also does not seem to be precluded by the PCIe
Spec (6.0.1).  The Atomic Op completion bits of the DEVCAP2 register
are defined as Read-only:

7.4 Configuration Register Types
 Read-only - Register bits are read-only and cannot be altered by software.
             Where explicitly defined, these bits are used to reflect changing
             hardware state, and as a result bit values can be observed to
             change at run time. Register bit default values and bits that
             cannot change value at run time, are permitted to be hard-coded,
             initialized by system/device firmware, or initialized by hardware
             mechanisms such as pin strapping or nonvolatile storage.
             Initialization by system firmware is permitted only for system-
             integrated devices. If the optional feature that would Set the
             bits is not implemented, the bits must be hardwired to Zero.

Here "altered by software" is relative to guest writes to the config
space register, whereas in this implementation we're acting as hardware
and the bits are changing to reflect a change in runtime capabilities.
The spec does include a HwInit register type which would restrict the
value from changing at runtime outside of resets.  Therefore while it
would not be advised to update these bits arbitrarily, it does seem safe
and compatible with guest software to update the value on device attach
and detach.

Note that of the Linux in-kernel drivers that make use of Atomic Ops,
it's not common for the driver to test Atomic Ops support of the device
itself.  Support is assumed, therefore it's fruitless to provide masking
of support at the device rather than the root port.

Also, by allowing this dynamic support, enabling Atomic Ops becomes
transparent to VM management tools.  There is no requirement to
designate specific Atomic Ops capabilities to a root port and impose a
burden on other userspace utilities.

Feedback welcome.  Thanks,

Alex

v2:
 - Don't require cold-plug device, modify RP bits around realize/exit

Alex Williamson (4):
  linux-headers: Update for vfio capability reporting AtomicOps
  vfio: Implement a common device info helper
  pcie: Add a PCIe capability version helper
  vfio/pci: Enable AtomicOps completers on root ports

 hw/pci/pcie.c                 |  7 ++++
 hw/s390x/s390-pci-vfio.c      | 37 +++--------------
 hw/vfio/common.c              | 46 ++++++++++++++++-----
 hw/vfio/pci.c                 | 78 +++++++++++++++++++++++++++++++++++
 hw/vfio/pci.h                 |  1 +
 include/hw/pci/pcie.h         |  1 +
 include/hw/vfio/vfio-common.h |  1 +
 linux-headers/linux/vfio.h    | 14 +++++++
 8 files changed, 142 insertions(+), 43 deletions(-)

-- 
2.39.2

On 27/5/23 01:15, Alex Williamson wrote:
> This RFC proposes to allow a vfio-pci device to manipulate the PCI
> Express capability of an associated root port to enable Atomic Op
> completer support as equivalent to host capabilities.  This would
[...]

> While it's not exactly standard practice to modify root port device
> capabilities runtime, it also does not seem to be precluded by the PCIe
> Spec (6.0.1).  The Atomic Op completion bits of the DEVCAP2 register
> are defined as Read-only:
> 
> 7.4 Configuration Register Types
>   Read-only - Register bits are read-only and cannot be altered by software.
>               Where explicitly defined, these bits are used to reflect changing
>               hardware state, and as a result bit values can be observed to
>               change at run time. Register bit default values and bits that
>               cannot change value at run time, are permitted to be hard-coded,
>               initialized by system/device firmware, or initialized by hardware
>               mechanisms such as pin strapping or nonvolatile storage.
>               Initialization by system firmware is permitted only for system-
>               integrated devices. If the optional feature that would Set the
>               bits is not implemented, the bits must be hardwired to Zero.
> 
> Here "altered by software" is relative to guest writes to the config
> space register, whereas in this implementation we're acting as hardware
> and the bits are changing to reflect a change in runtime capabilities.
> The spec does include a HwInit register type which would restrict the
> value from changing at runtime outside of resets.  Therefore while it
> would not be advised to update these bits arbitrarily, it does seem safe
> and compatible with guest software to update the value on device attach
> and detach.

 From my previous (short) PCIe experience, this is also my understanding.

Hi Alex,

Thanks for taking the time to implement support for Atomic Op completer
support properly :). I have tested out these patches and the kernel
patch, and apart from a minor issue with patch 2 everything works fine;
ROCm programs that use device->host atomic operations work properly.

Something that I have been thinking about, are there any implications
involved with enabling this feature automatically with no possibility of
turning it off? I have no use case for that, though, and I cant really
think of a reason other than preventing the guest from finding out
hardware details about the host.

Thanks,

Robin Voetter, Stream HPC

On Wed, 31 May 2023 23:55:41 +0200
Robin Voetter <robin@streamhpc.com> wrote:

> Hi Alex,
> 
> Thanks for taking the time to implement support for Atomic Op completer
> support properly :). I have tested out these patches and the kernel
> patch, and apart from a minor issue with patch 2 everything works fine;

Yes, Cedric noted the extra semicolon as well, I'm about to post that
patch as non-RFC since it has no dependencies on anything else.

> ROCm programs that use device->host atomic operations work properly.

Great, thanks for testing!

> Something that I have been thinking about, are there any implications
> involved with enabling this feature automatically with no possibility of
> turning it off? I have no use case for that, though, and I cant really
> think of a reason other than preventing the guest from finding out
> hardware details about the host.

Not sure I follow, are you suggesting that Atomic Ops completion
support is proactively enabled in the VM to match the host, regardless
of attached assigned devices?  An obvious problem there is migration.
If I start a VM on a host with Atomic Ops support and want to migrate
to a host without Atomic Ops support, config space of the root ports is
now different and the migration fails.  QEMU would also require
elevated privileges to read config space to determine the host support,
and then what does it do if only some of the PCIe hierarchy supports
Atomic Ops.

Policy decisions like that are generally left to management tools, so
there would always be a means to enable or disable the feature.  In
fact, that's specifically why I test that the Atomic Op completer bits
are unset in the root port before changing them so that this automatic
enablement could live alongside a command line option to statically
enable some bits.

That does however remind me that it is often good with these sorts of
"clever" automatic features to have an opt-out, so I'll likely add an
x-no-rp-atomics device option in the next version to provide that.
Thanks,

Alex

On 6/1/23 00:24, Alex Williamson wrote:
> On Wed, 31 May 2023 23:55:41 +0200
> Robin Voetter <robin@streamhpc.com> wrote:
>> Something that I have been thinking about, are there any implications
>> involved with enabling this feature automatically with no possibility of
>> turning it off? I have no use case for that, though, and I cant really
>> think of a reason other than preventing the guest from finding out
>> hardware details about the host.
> 
> Not sure I follow, are you suggesting that Atomic Ops completion
> support is proactively enabled in the VM to match the host, regardless
> of attached assigned devices?  An obvious problem there is migration.
> If I start a VM on a host with Atomic Ops support and want to migrate
> to a host without Atomic Ops support, config space of the root ports is
> now different and the migration fails.  QEMU would also require
> elevated privileges to read config space to determine the host support,
> and then what does it do if only some of the PCIe hierarchy supports
> Atomic Ops.
> 
> Policy decisions like that are generally left to management tools, so
> there would always be a means to enable or disable the feature.  In
> fact, that's specifically why I test that the Atomic Op completer bits
> are unset in the root port before changing them so that this automatic
> enablement could live alongside a command line option to statically
> enable some bits.
> 
> That does however remind me that it is often good with these sorts of
> "clever" automatic features to have an opt-out, so I'll likely add an
> x-no-rp-atomics device option in the next version to provide that.

Yes, something like that that is exactly what I was thinking about.

Thanks,

Robin Voetter

On 1/6/23 00:24, Alex Williamson wrote:
> On Wed, 31 May 2023 23:55:41 +0200
> Robin Voetter <robin@streamhpc.com> wrote:
> 
>> Hi Alex,
>>
>> Thanks for taking the time to implement support for Atomic Op completer
>> support properly :). I have tested out these patches and the kernel
>> patch, and apart from a minor issue with patch 2 everything works fine;
> 
> Yes, Cedric noted the extra semicolon as well, I'm about to post that
> patch as non-RFC since it has no dependencies on anything else.


> Policy decisions like that are generally left to management tools, so
> there would always be a means to enable or disable the feature.  In
> fact, that's specifically why I test that the Atomic Op completer bits
> are unset in the root port before changing them so that this automatic
> enablement could live alongside a command line option to statically
> enable some bits.
> 
> That does however remind me that it is often good with these sorts of
> "clever" automatic features to have an opt-out, so I'll likely add an
> x-no-rp-atomics device option in the next version to provide that.

I was just going to suggest that, just in case :)

Hi Markus, Marc-André,

> On 1/6/23 00:24, Alex Williamson wrote:
>> Policy decisions like that are generally left to management tools, so
>> there would always be a means to enable or disable the feature.  In
>> fact, that's specifically why I test that the Atomic Op completer bits
>> are unset in the root port before changing them so that this automatic
>> enablement could live alongside a command line option to statically
>> enable some bits.
>>
>> That does however remind me that it is often good with these sorts of
>> "clever" automatic features to have an opt-out, so I'll likely add an
>> x-no-rp-atomics device option in the next version to provide that.

Still thinking about this series I remembered since commit a3c45b3e62
("qapi: New special feature flag "unstable"") the "x-" prefix is
obsolete (superseded) in QAPI generated code.

I wonder about device properties:

$ git grep -F '"x-' hw | wc -l
      130

$ git grep -F '"x-' hw/vfio/pci.c
hw/vfio/pci.c:2987:        error_setg(errp, "x-balloon-allowed only 
potentially compatible "
hw/vfio/pci.c:3335: 
DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", VFIOPCIDevice,
hw/vfio/pci.c:3342:    DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", 
VFIOPCIDevice,
hw/vfio/pci.c:3344:    DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
hw/vfio/pci.c:3346:    DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
hw/vfio/pci.c:3348:    DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, 
features,
hw/vfio/pci.c:3350:    DEFINE_PROP_BOOL("x-enable-migration", VFIOPCIDevice,
hw/vfio/pci.c:3352:    DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, 
vbasedev.no_mmap, false),
hw/vfio/pci.c:3353:    DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
hw/vfio/pci.c:3355:    DEFINE_PROP_BOOL("x-no-kvm-intx", VFIOPCIDevice, 
no_kvm_intx, false),
hw/vfio/pci.c:3356:    DEFINE_PROP_BOOL("x-no-kvm-msi", VFIOPCIDevice, 
no_kvm_msi, false),
hw/vfio/pci.c:3357:    DEFINE_PROP_BOOL("x-no-kvm-msix", VFIOPCIDevice, 
no_kvm_msix, false),
hw/vfio/pci.c:3358:    DEFINE_PROP_BOOL("x-no-geforce-quirks", 
VFIOPCIDevice,
hw/vfio/pci.c:3360:    DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", 
VFIOPCIDevice, no_kvm_ioeventfd,
hw/vfio/pci.c:3362:    DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", 
VFIOPCIDevice, no_vfio_ioeventfd,
hw/vfio/pci.c:3364:    DEFINE_PROP_UINT32("x-pci-vendor-id", 
VFIOPCIDevice, vendor_id, PCI_ANY_ID),
hw/vfio/pci.c:3365:    DEFINE_PROP_UINT32("x-pci-device-id", 
VFIOPCIDevice, device_id, PCI_ANY_ID),
hw/vfio/pci.c:3366:    DEFINE_PROP_UINT32("x-pci-sub-vendor-id", 
VFIOPCIDevice,
hw/vfio/pci.c:3368:    DEFINE_PROP_UINT32("x-pci-sub-device-id", 
VFIOPCIDevice,
hw/vfio/pci.c:3370:    DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, 
igd_gms, 0),
hw/vfio/pci.c:3371: 
DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
hw/vfio/pci.c:3374:    DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", 
VFIOPCIDevice, msix_relo,

Is there a plan to use something similar for QOM properties?
Is it OK to keep using the "x-" prefix there?

Thanks,

Phil.