Re: [PATCH v2] vfio: Add property documentation

["Cédric Le Goater" <clg@redhat.com>]

On 2/13/25 22:45, Alex Williamson wrote:
> On Thu, 13 Feb 2025 14:50:50 +0100
> Cédric Le Goater <clg@redhat.com> wrote:
> 
>> Investigate the git history to uncover when and why the VFIO
>> properties were introduced and update the models. This is mostly
>> targeting vfio-pci device, since vfio-plateform, vfio-ap and vfio-ccw
>> devices are simpler.
>>
>> Organize the vfio-pci properties in topics. It would be great to have
>> a way to do the same for the output.
>>
>> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
>> Cc: Eric Farman <farman@linux.ibm.com>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   Changes in v2:
>>
>>   - Fixed version numbers
>>   - Fixed #ifdef in vfio/ccw.c
>>   - Addressed vfio-pci-nohotplug
>>   - Organize the vfio-pci properties in topics
>>
>>   hw/vfio/ap.c       |   9 +++
>>   hw/vfio/ccw.c      |  15 +++++
>>   hw/vfio/pci.c      | 134 +++++++++++++++++++++++++++++++++++++++++++++
>>   hw/vfio/platform.c |  23 ++++++++
>>   4 files changed, 181 insertions(+)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 30b08ad375d5ecae886c5000fbaa364799fe76d0..ec1150e5d627fce83a5a6319af471fd0aa45ae9b 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -257,6 +257,15 @@ static void vfio_ap_class_init(ObjectClass *klass, void *data)
>>       dc->hotpluggable = true;
>>       device_class_set_legacy_reset(dc, vfio_ap_reset);
>>       dc->bus_type = TYPE_AP_BUS;
>> +
>> +    object_class_property_set_description(klass, /* 3.1 */
>> +                                          "sysfsdev",
>> +                                          "Host sysfs path of assigned device");
>> +#ifdef CONFIG_IOMMUFD
>> +    object_class_property_set_description(klass, /* 9.0 */
>> +                                          "iommufd",
>> +                                          "Set host IOMMUFD backend device ");
>> +#endif
>>   }
>>   
>>   static const TypeInfo vfio_ap_info = {
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 67bc137f9be6d43c5970c6271f3cdbfffd8a32de..242dc39660fcd028722093d637b7b64440649863 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -717,6 +717,21 @@ static void vfio_ccw_class_init(ObjectClass *klass, void *data)
>>       cdc->handle_halt = vfio_ccw_handle_halt;
>>       cdc->handle_clear = vfio_ccw_handle_clear;
>>       cdc->handle_store = vfio_ccw_handle_store;
>> +
>> +    object_class_property_set_description(klass, /* 2.10 */
>> +                                          "sysfsdev",
>> +                                          "Host sysfs path of assigned device");
>> +    object_class_property_set_description(klass, /* 3.0 */
>> +                                          "force-orb-pfch",
>> +                                          "Force unlimited prefetch");
>> +#ifdef CONFIG_IOMMUFD
>> +    object_class_property_set_description(klass, /* 9.0 */
>> +                                          "iommufd",
>> +                                          "Set host IOMMUFD backend device ");
>> +#endif
>> +    object_class_property_set_description(klass, /* 9.2 */
>> +                                          "loadparm",
>> +                                          "Define which devices that can be used for booting");
>>   }
>>   
>>   static const TypeInfo vfio_ccw_info = {
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 9a55e7b77324babf7295132b08e3ba23b482a291..fbd8cf566b1cfd508ccb0042a395e3b79ba781c0 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3433,6 +3433,133 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
>>       pdc->exit = vfio_exitfn;
>>       pdc->config_read = vfio_pci_read_config;
>>       pdc->config_write = vfio_pci_write_config;
>> +
>> +    object_class_property_set_description(klass, /* 1.3 */
>> +                                          "host",
>> +                                          "Host PCI address [domain:]<bus:slot.function> of assigned device");
>> +    object_class_property_set_description(klass, /* 2.6 */
>> +                                          "sysfsdev",
>> +                                          "Host sysfs path of assigned device");
>> +    /*
>> +     * Display
>> +     */
>> +
>> +    object_class_property_set_description(klass, /* 1.5 */
>> +                                          "x-vga",
>> +                                          "Add support for VGA MMIO and I/O port access");
> 
> "Expose VGA address spaces for device"
> 
>> +    object_class_property_set_description(klass, /* 2.12 */
>> +                                          "display",
>> +                                          "Add display support");
> 
> "Enable display support for device, ex. vGPU"
> 
>> +    object_class_property_set_description(klass, /* 3.2 */
>> +                                          "xres",
>> +                                          "Set X display resolution the vgpu should use");
> 
> Maybe capitalize for consistency, vGPU.
> 
>> +    object_class_property_set_description(klass, /* 3.2 */
>> +                                          "yres",
>> +                                          "Set Y display resolution the vgpu should use");
>> +
>> +    /*
>> +     * IGD
>> +     */
>> +
>> +    object_class_property_set_description(klass, /* 2.7 */
>> +                                          "x-igd-opregion",
>> +                                          "Add IGD OpRegion support for (headless system)");
> 
> [Cc Tomita and Corvin have more recent understanding of IGD options]
> 
> Not necessarily for headless systems, unless others have better
> suggestions, maybe just "Expose host IGD OpRegion table to guest".
> 
>> +    object_class_property_set_description(klass, /* 2.7 (See c4c45e943e51) */
>> +                                          "x-igd-gms",
>> +                                          "Add Intel graphics legacy mode device assignment support. "
>> +                                          "Assign 00:02.0 from the host to 00:02.0 in the VM");
> 
> Not really.  Tomita added a useful comment and commit log in
> 37f05a59e869.  Perhaps:
> 
> "Override DVMT Pre-Allocated value for IGD stolen memory. (32MiB units)"
> 
>> +
>> +    /*
>> +     * NVIDIA
>> +     */
>> +    object_class_property_set_description(klass, /* 2.12 */
>> +                                          "x-no-geforce-quirks",
>> +                                          "Disable GeForce quirks (for NVIDIA Quadro/GRID/Tesla). Improves performance");
>> +    object_class_property_set_description(klass, /* 3.0 */
>> +                                          "x-no-kvm-ioeventfd",
>> +                                          "Disable ioeventfd quirk (NVIDIA)");
>> +    object_class_property_set_description(klass, /* 3.0 */
>> +                                          "x-no-vfio-ioeventfd",
>> +                                          "Enable ioeventfd quirks to be handled by VFIO directly. Improves performance");
> 
> The above two should be under debugging, they're only used by NVIDIA
> quirks but they exist for the purpose of validating behavior with and
> without.  The first disables registering an ioeventfd with KVM and the
> latter disables directly "wiring" it through to vfio.  Maybe
> (respectively):
> 
> "Disable registration of ioeventfds with KVM"
> 
> "Disable linking of KVM ioeventfds to VFIO ioeventfds"
> 
> NB. setting the latter does not improve performance.
> 
>> +    object_class_property_set_description(klass, /* 2.11 */
>> +                                          "x-nv-gpudirect-clique",
>> +                                          "Add NVIDIA GPUDirect Cliques support");
> 
> "Add NVIDIA GPUDirect capability indicating P2P DMA clique for device. (0~15)"
> 
> (maybe this is already specified as a uint4 given the PropertyInfo?)

It's not. I will add range [0-15]
> 
>> +
>> +    /*
>> +     * Migration support
>> +     */
>> +    object_class_property_set_description(klass, /* 5.2 */
>> +                                          "x-pre-copy-dirty-page-tracking",
>> +                                          "Disable dirty pages tracking during iterative phase");
>> +    object_class_property_set_description(klass, /* 9.1 */
>> +                                          "x-device-dirty-page-tracking",
>> +                                          "Disable device dirty page tracking and use container-based dirty page tracking");
> 
> These are really debug as well, right?  They just happen to be
> migration related debug.

I asked Joao and Kirti.

>> +    object_class_property_set_description(klass, /* 5.2, 8.0 non-experimetal */
>> +                                          "enable-migration",
>> +                                          "Enale device migration. Also requires a host VFIO PCI variant "
>> +                                          "driver with migration support enabled");
>> +    object_class_property_set_description(klass, /* 9.1 */
>> +                                          "migration-events",
>> +                                          "Emit VFIO migration QAPI event when a VFIO device changes its migration "
>> +                                          "state. For management applications");
>> +    object_class_property_set_description(klass, /* 9.1 */
>> +                                          "skip-vsc-check",
>> +                                          "Skip config space check for Vendor Specific Capability. Useful for "
>> +                                          "NVIDIA vGPU driver migration");
> 
> The vsc check is really for debug too since it's enabled by default and
> mostly only exists as a visible option for manipulation by the machine
> type.
> 
>> +
>> +    /*
>> +     * debug, tracing
>> +     */
>> +    object_class_property_set_description(klass, /* 2.4 and 2.5 */
>> +                                          "x-no-mmap",
>> +                                          "Disable MMAP for device. Allows to trace MMIO accesses");
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-no-kvm-intx",
>> +                                          "Bypass INTx interrupts. Allows interrupt tracing");
> 
> "Disable direct VFIO->KVM INTx injection. Allows to trace INTx interrupts"
> 
> s/INTx/MSI/
> s/INTx/MSIx/
> 
> for the next.
> 
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-no-kvm-msi",
>> +                                          "Bypass MSI interrupts. Allows interrupt tracing");
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-no-kvm-msix",
>> +                                          "Bypass MSIx interrupts. Allows interrupt tracing");
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-pci-vendor-id",
>> +                                          "Set emulated PCI Vendor ID. Allows testing quirks");
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-pci-device-id",
>> +                                          "Set emulated PCI device ID. Allows testing quirks");
> 
> "Override PCI Vendor ID with provided value"
> 
> s/Vendor/Device/
> 
> There are also "sub" versions of these for subsystem Vendor and Device
> IDs.
> 
> None of these are really for quirks, they're more for making devices
> appear as other devices.  For reasons.
> 
>> +
>> +    /*
>> +     * other
>> +     */
>> +    object_class_property_set_description(klass, /* 8.1 */
>> +                                          "vf-token",
>> +                                          "Add support for VF token among PF and VFs (Linux 5.7+)");
> 
> "Specify UUID VF token.  Required for VF when PF is owned by another VFIO driver"
> 
>> +    object_class_property_set_description(klass, /* 1.3 */
>> +                                          "x-intx-mmap-timeout-ms",
>> +                                          "Timeout value in milliseconds to re-enable BAR mapping when under "
>> +                                          "INTx interrupts. Improves performance");
> 
> Changes performance perhaps.  This should also be under debug.
> 
> "When EOI is not provided by KVM/QEMU, wait time to re-enable device direct access after INTx"
> 
>> +    object_class_property_set_description(klass, /* 2.3 */
>> +                                          "x-req",
>> +                                          "Add device request notification support (Linux 4.0+)");
> 
> Also debug.
> 
> "Disable device request notification support"
> 
>> +    object_class_property_set_description(klass, /* 3.1 */
>> +                                          "x-balloon-allowed",
>> +                                          "Allow devices to opt-in for ballooning");
> 
> Debug.
> 
> "Override allowing ballooning with device. (DANGER)"
> 
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-pci-sub-vendor-id",
>> +                                          "Set emulated PCI Sub-vendor ID");
>> +    object_class_property_set_description(klass, /* 2.5 */
>> +                                          "x-pci-sub-device-id",
>> +                                          "Set emulated PCI Sub-device ID");
> 
> Ah, here they are.  Grouping with non-subsystem would make sense to me.
> 
>> +    object_class_property_set_description(klass, /* 2.12 */
>> +                                          "x-msix-relocation",
>> +                                          "Allow relocating MSI-X MMIO on systems which page size is larger "
>> +                                          "than the PCI spec recommendation. Mostly for sPAPR");
> 
> Yes, we added it because of systems with >4K pages, but there do exist
> devices that do not follow the PCI spec recommendation and place
> non-MSI related registers too close the vector table and pba.
> 
> "Specify MSI-X MMIO relocation to the end of specified existing BAR or
> new BAR to avoid virtualization overhead due to adjacent device
> registers"
> 
>> +#ifdef CONFIG_IOMMUFD
>> +    object_class_property_set_description(klass, /* 9.0 */
>> +                                          "iommufd",
>> +                                          "Set host IOMMUFD backend device ");
>> +#endif
>>   }
>>   
>>   static const TypeInfo vfio_pci_dev_info = {
>> @@ -3461,6 +3588,13 @@ static void vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
>>   
>>       device_class_set_props(dc, vfio_pci_dev_nohotplug_properties);
>>       dc->hotpluggable = false;
>> +    object_class_property_set_description(klass, /* 3.1 */
>> +                                          "ramfb",
>> +                                          "Add ramfb support");
> 
> "Enable ramfb to provide pre-boot graphics for devices enabling display option"
> 
>> +    object_class_property_set_description(klass, /* 8.2 */
>> +                                          "x-ramfb-migrate",
>> +                                          "Add ramfb migration support");
> 
> Debug.
> 
> "Override default migration support for ramfb support"
> 
>> +
>>   }
>>   
>>   static const TypeInfo vfio_pci_nohotplug_dev_info = {
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index 1070a2113a17edb9ebafb5066e51ee2bc52a767d..8e646e543692221e86b16fecd8bf40316f064a7d 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -674,6 +674,29 @@ static void vfio_platform_class_init(ObjectClass *klass, void *data)
>>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>       /* Supported by TYPE_VIRT_MACHINE */
>>       dc->user_creatable = true;
>> +
>> +    object_class_property_set_description(klass, /* 2.4 */
>> +                                          "host",
>> +                                          "Host device name of assigned device");
>> +    object_class_property_set_description(klass, /* 2.6 */
>> +                                          "sysfsdev",
>> +                                          "Host sysfs path of assigned device");
>> +    object_class_property_set_description(klass, /* 2.4 and 2.5 */
>> +                                          "x-no-mmap",
>> +                                          "Disable MMAP for device. Allows to trace MMIO accesses");
>> +    object_class_property_set_description(klass, /* 2.4 */
>> +                                          "mmap-timeout-ms",
>> +                                          "Timeout value in milliseconds to re-enable BAR mapping");
> 
> This is essentially the same as the vfio-pci INTx version, only used
> when we don't have a mechanism for EOI.
> 
>> +    object_class_property_set_description(klass, /* 2.4 */
>> +                                          "x-irqfd",
>> +                                          "Use irqfd for IRQ handling");
> 
> Debug.  Allow disabling irqfd support.
> 
> Maybe rather than trying to group debug options together they should
> just be explicitly labeled in the description, ex. "[DEBUG]"

I came to the same conclusion. If the property name or description
is not clear I will add a label (DEBUG, NVIDIA, etc) and order the
properties by QEMU version.

I find the QEMU version in which a property was introduced interesting
to save. It's less tedious then digging with "git log -S".

May be should we introduce documentation for properties like the kernel
has in Documentation/ABI/*/sysfs-* ?

Thanks,

C.