[RFC: vf-token 0/5] Introduce vf-token when using userspace PF
vf token is set by a vfio-pci based PF driver and it must be known to the vfio-pci based VF driver. This vf-token is set by the PF driver before VF drivers can access the device. vfio-pci driver and qemu support vf-token. This RFC patch series adds support to provide the vf-token (uuid format) in the domain XML and to generate the qemu commandline including the vf-token. To support vf-token the new element will be used as follows: <hostdev mode='subsystem' type='pci' managed='yes'> <driver name='vfio'/> <source> <address domain='0x0000' bus='0x0' slot='0x00' function='0x1'> <vf-token uuid='00112233-4455-6677-8899-aabbccddeeff'/> </address> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </hostdev> The generated commandline will include the following: -device {"driver":"vfio-pci","host":"0000:00:0.1", "vf-token":"00112233-4455-6677-8899-aabbccddeeff", "id":"hostdev0","bus":"pci.0","addr":"0x1"} This patch is get feedback on the approach. Will post with add documentation and testcases in follow-up. Vivek Kashyap (5): virpci: Define vf-token qemu: vf-token capability conf: vf-token flag conf: vf-token parsing and formatting qemu: validate and generate vf-token on command line src/conf/device_conf.c | 31 +++++++++++++++++++++++++++++-- src/conf/device_conf.h | 3 +++ src/conf/domain_addr.h | 1 + src/conf/domain_conf.c | 5 +++++ src/conf/schemas/basictypes.rng | 11 +++++++++++ src/conf/schemas/domaincommon.rng | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 27 ++++++++++++++++++++++++--- src/qemu/qemu_domain_address.c | 3 +++ src/qemu/qemu_validate.c | 19 +++++++++++++++++++ src/util/virpci.c | 5 +++++ src/util/virpci.h | 11 +++++++++++ 14 files changed, 117 insertions(+), 5 deletions(-) -- 2.25.1
Define the vf-token extension for PCI device Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/util/virpci.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/util/virpci.h b/src/util/virpci.h index faca6cf6f9..b4e9e95d88 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -50,7 +50,15 @@ struct _virZPCIDeviceAddress { /* Don't forget to update virPCIDeviceAddressCopy if needed. */ }; +typedef struct _virPCIDeviceToken virPCIDeviceToken; + +struct _virPCIDeviceToken { + unsigned char uuid[VIR_UUID_BUFLEN]; + bool isSet; +}; + #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d" +#define VIR_PCI_DEVICE_TOKEN_FMT "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x" /* Represents format of PF's phys_port_name in switchdev mode: * 'p%u' or 'p%us%u'. New line checked since value is read from sysfs file. @@ -65,6 +73,7 @@ struct _virPCIDeviceAddress { virTristateSwitch multi; int extFlags; /* enum virPCIDeviceAddressExtensionFlags */ virZPCIDeviceAddress zpci; + virPCIDeviceToken token; /* Don't forget to update virPCIDeviceAddressCopy if needed. */ }; @@ -269,6 +278,8 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddress *bdf); bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr); bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr); +bool virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token); +bool virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, int pfNetDevIdx, -- 2.25.1
On Thu, Oct 05, 2023 at 16:05:00 -0700, Vivek Kashyap wrote:
Define the vf-token extension for PCI device
Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/util/virpci.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/util/virpci.h b/src/util/virpci.h index faca6cf6f9..b4e9e95d88 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -50,7 +50,15 @@ struct _virZPCIDeviceAddress { /* Don't forget to update virPCIDeviceAddressCopy if needed. */ };
+typedef struct _virPCIDeviceToken virPCIDeviceToken; + +struct _virPCIDeviceToken { + unsigned char uuid[VIR_UUID_BUFLEN]; + bool isSet; +}; + #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d" +#define VIR_PCI_DEVICE_TOKEN_FMT "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"
/* Represents format of PF's phys_port_name in switchdev mode: * 'p%u' or 'p%us%u'. New line checked since value is read from sysfs file. @@ -65,6 +73,7 @@ struct _virPCIDeviceAddress { virTristateSwitch multi; int extFlags; /* enum virPCIDeviceAddressExtensionFlags */ virZPCIDeviceAddress zpci; + virPCIDeviceToken token; /* Don't forget to update virPCIDeviceAddressCopy if needed. */ };
@@ -269,6 +278,8 @@ int virPCIDeviceAddressParse(char *address, virPCIDeviceAddress *bdf);
bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr); bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr); +bool virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token); +bool virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci);
These function definitions should go into the patch that actually implements the functions themselves.
Introduce qemu capability for vf-token Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3a1bfbf74d..f84c4df8db 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -698,6 +698,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 450 */ "run-with.async-teardown", /* QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN */ "virtio-blk-vhost-vdpa", /* QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA */ + "vf-token", /* QEMU_CAPS_VFIO_VFTOKEN */ ); @@ -1384,6 +1385,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-crypto-device", QEMU_CAPS_DEVICE_VIRTIO_CRYPTO }, { "cryptodev-backend-lkcf", QEMU_CAPS_OBJECT_CRYPTO_LKCF }, { "pvpanic-pci", QEMU_CAPS_DEVICE_PANIC_PCI }, + { "vf-token", QEMU_CAPS_VFIO_VFTOKEN }, }; @@ -1446,6 +1448,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioSCSI[] = { }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVfioPCI[] = { + { "vf-token", QEMU_CAPS_VFIO_VFTOKEN, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsSCSIDisk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3c4f7f625b..f97b1c9fd5 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -677,6 +677,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 450 */ QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN, /* asynchronous teardown -run-with async-teardown=on|off */ QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA, /* virtio-blk-vhost-vdpa block driver */ + QEMU_CAPS_VFIO_VFTOKEN, /* vf-token support */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.25.1
On Thu, Oct 05, 2023 at 16:05:01 -0700, Vivek Kashyap wrote:
Introduce qemu capability for vf-token
Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3a1bfbf74d..f84c4df8db 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -698,6 +698,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 450 */ "run-with.async-teardown", /* QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN */ "virtio-blk-vhost-vdpa", /* QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA */ + "vf-token", /* QEMU_CAPS_VFIO_VFTOKEN */ );
@@ -1384,6 +1385,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-crypto-device", QEMU_CAPS_DEVICE_VIRTIO_CRYPTO }, { "cryptodev-backend-lkcf", QEMU_CAPS_OBJECT_CRYPTO_LKCF }, { "pvpanic-pci", QEMU_CAPS_DEVICE_PANIC_PCI }, + { "vf-token", QEMU_CAPS_VFIO_VFTOKEN }, };
@@ -1446,6 +1448,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioSCSI[] = { };
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVfioPCI[] = { + { "vf-token", QEMU_CAPS_VFIO_VFTOKEN, NULL },
Extra trailing whitespace.
};
Also the 'vf-token' field was added in qemu-8.1 thus you are missing the appropriate test file updates. Also the whitespace error would be caught by our test suite. Please always make sure to post patches where our test suite passes after every single commit: https://www.libvirt.org/hacking.html#preparing-patches Notably you'll need to re-generate the 'qemucapabilitiestest' output files to include the new flag. To do so automatically run: VIR_TEST_REGENERATE_OUTPUT=1 ./tests/qemucapabilitiestest from your build directory after you build everything.
Define PCI address extension flag for vf-token Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/conf/device_conf.h | 3 +++ src/conf/domain_addr.h | 1 + src/qemu/qemu_domain_address.c | 3 +++ 3 files changed, 7 insertions(+) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index a83377417a..29e03baa99 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -188,6 +188,9 @@ bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info); int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddress *addr); +int virPCIDeviceTokenParseXML(xmlNodePtr node, + virPCIDeviceAddress *addr); + void virPCIDeviceAddressFormat(virBuffer *buf, virPCIDeviceAddress addr, bool includeTypeInAddr); diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index e72fb48847..bca96ee797 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -29,6 +29,7 @@ typedef enum { VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */ VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zPCI support */ + VIR_PCI_ADDRESS_EXTENSION_VFTOKEN = 1 << 1, /* Token support */ } virPCIDeviceAddressExtensionFlags; typedef enum { diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 099778b2a8..3be5acbc9e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -575,6 +575,9 @@ qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCaps *qemuCaps, extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI; } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_VFTOKEN)) + extFlags |= VIR_PCI_ADDRESS_EXTENSION_VFTOKEN; + return extFlags; } -- 2.25.1
On Thu, Oct 05, 2023 at 16:05:02 -0700, Vivek Kashyap wrote:
Define PCI address extension flag for vf-token
Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/conf/device_conf.h | 3 +++ src/conf/domain_addr.h | 1 + src/qemu/qemu_domain_address.c | 3 +++ 3 files changed, 7 insertions(+)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index a83377417a..29e03baa99 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -188,6 +188,9 @@ bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info); int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddress *addr);
+int virPCIDeviceTokenParseXML(xmlNodePtr node, + virPCIDeviceAddress *addr);
Broken alignment of the second line. And same as in 1/5 the function is not used here so don't define it in this patch.
Introduce XML parsing and formatting of vf-token attribute Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/conf/device_conf.c | 31 +++++++++++++++++++++++++++++-- src/conf/domain_conf.c | 5 +++++ src/conf/schemas/basictypes.rng | 11 +++++++++++ src/conf/schemas/domaincommon.rng | 1 + src/libvirt_private.syms | 1 + src/util/virpci.c | 5 +++++ 6 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index f3d977f2b7..d5ed4ef452 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -188,11 +188,20 @@ virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info) virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci); } +bool +virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci) +{ + return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsPresent(&pci->zpci)) || + ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) && + pci->token.isSet); +} + bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) { - return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && - virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci); + return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) && + virDeviceExtensionIsPresent(&info->addr.pci); } int @@ -200,6 +209,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddress *addr) { xmlNodePtr zpci; + xmlNodePtr token; memset(addr, 0, sizeof(*addr)); @@ -231,6 +241,10 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, return -1; } + if ((token = virXMLNodeGetSubelement(node, "vf-token"))) + if (virPCIDeviceTokenParseXML(token, addr) < 0) + return -1; + return 0; } @@ -248,6 +262,19 @@ virPCIDeviceAddressFormat(virBuffer *buf, addr.function); } +int +virPCIDeviceTokenParseXML(xmlNodePtr node, + virPCIDeviceAddress *addr) +{ + if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_NONE, + addr->token.uuid) < 0) + return -1; + + addr->token.isSet = 1; + + return 0; +} + int virCCWDeviceAddressParseXML(xmlNodePtr node, virCCWDeviceAddress *addr) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4435ee2ad4..cceb1d3b83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5389,6 +5389,11 @@ virDomainDeviceInfoFormat(virBuffer *buf, info->addr.pci.zpci.uid.value, info->addr.pci.zpci.fid.value); } + if (virPCIVFIOTokenIDIsPresent(&info->addr.pci.token)) { + virBufferAsprintf(&childBuf, + "<vf-token uuid='%s'/>\n", + info->addr.pci.token.uuid); + } break; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: diff --git a/src/conf/schemas/basictypes.rng b/src/conf/schemas/basictypes.rng index 26eb538077..bf2c30dd18 100644 --- a/src/conf/schemas/basictypes.rng +++ b/src/conf/schemas/basictypes.rng @@ -138,6 +138,17 @@ </element> </optional> </define> + <define name="vf-tokenid"> + <optional> + <element name="vf-token"> + <optional> + <attribute name="uuid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </optional> + </define> <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> <!-- The lowest bit of the 1st byte is the "multicast" bit. a --> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index a26986b5ce..b228a3ca73 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -7034,6 +7034,7 @@ </attribute> <ref name="pciaddress"/> <ref name="zpciaddress"/> + <ref name="vf-tokenid"/> </group> <group> <attribute name="type"> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e475d5b1a..0d4866cd56 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3138,6 +3138,7 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; +virPCIVFIOTokenIDIsPresent; virPCIVirtualFunctionListFree; virZPCIDeviceAddressIsIncomplete; virZPCIDeviceAddressIsPresent; diff --git a/src/util/virpci.c b/src/util/virpci.c index 08b82708b1..dfb0f29182 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2312,6 +2312,11 @@ virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr) return addr->uid.isSet || addr->fid.isSet; } +bool +virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token) +{ + return token->isSet; +} void virPCIVirtualFunctionListFree(virPCIVirtualFunctionList *list) -- 2.25.1
On Thu, Oct 05, 2023 at 16:05:03 -0700, Vivek Kashyap wrote:
Introduce XML parsing and formatting of vf-token attribute
Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/conf/device_conf.c | 31 +++++++++++++++++++++++++++++-- src/conf/domain_conf.c | 5 +++++ src/conf/schemas/basictypes.rng | 11 +++++++++++ src/conf/schemas/domaincommon.rng | 1 + src/libvirt_private.syms | 1 + src/util/virpci.c | 5 +++++ 6 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index f3d977f2b7..d5ed4ef452 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -188,11 +188,20 @@ virDeviceInfoPCIAddressExtensionIsWanted(const virDomainDeviceInfo *info) virZPCIDeviceAddressIsIncomplete(&info->addr.pci.zpci); }
+bool +virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci) +{ + return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && + virZPCIDeviceAddressIsPresent(&pci->zpci)) || + ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) && + pci->token.isSet);
The code alignment is broken here. Also add a function documentation describing what this function does as it's really not obvious.
+} +
Code style dictates two empty lines between functions.
bool virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info) { - return (info->addr.pci.extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) && - virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci); + return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) && + virDeviceExtensionIsPresent(&info->addr.pci);
This change is suspicious and should be justified. I'm not going to comment on whether it's correct or not.
}
int @@ -200,6 +209,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddress *addr) { xmlNodePtr zpci; + xmlNodePtr token;
memset(addr, 0, sizeof(*addr));
@@ -231,6 +241,10 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, return -1; }
+ if ((token = virXMLNodeGetSubelement(node, "vf-token")))
Multi-line condition bodies dictate use of a { } block around it.
+ if (virPCIDeviceTokenParseXML(token, addr) < 0) + return -1;
This one is okay though.
+ return 0; }
@@ -248,6 +262,19 @@ virPCIDeviceAddressFormat(virBuffer *buf, addr.function); }
+int +virPCIDeviceTokenParseXML(xmlNodePtr node, + virPCIDeviceAddress *addr) +{ + if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_NONE, + addr->token.uuid) < 0)
virXMLPropUUID parses into a buffer which is not a string (NUL-byte terminated) but a byte array ...
+ return -1; + + addr->token.isSet = 1; + + return 0; +} +
Two empty lines between functions.
int virCCWDeviceAddressParseXML(xmlNodePtr node, virCCWDeviceAddress *addr) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4435ee2ad4..cceb1d3b83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5389,6 +5389,11 @@ virDomainDeviceInfoFormat(virBuffer *buf, info->addr.pci.zpci.uid.value, info->addr.pci.zpci.fid.value); } + if (virPCIVFIOTokenIDIsPresent(&info->addr.pci.token)) { + virBufferAsprintf(&childBuf, + "<vf-token uuid='%s'/>\n", + info->addr.pci.token.uuid);
... but here you use a *string* formatting function. So this will either not print any reasonable output or potentially crash if the UUID contains no NUL-bytes. You would figure this out if you'd add any tests for this feature which is in fact required. It is okay to add the tests at the end though.
+ } break;
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: diff --git a/src/conf/schemas/basictypes.rng b/src/conf/schemas/basictypes.rng index 26eb538077..bf2c30dd18 100644 --- a/src/conf/schemas/basictypes.rng +++ b/src/conf/schemas/basictypes.rng @@ -138,6 +138,17 @@ </element> </optional> </define> + <define name="vf-tokenid"> + <optional> + <element name="vf-token"> + <optional> + <attribute name="uuid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </optional> + </define>
<!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> <!-- The lowest bit of the 1st byte is the "multicast" bit. a --> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index a26986b5ce..b228a3ca73 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -7034,6 +7034,7 @@ </attribute> <ref name="pciaddress"/> <ref name="zpciaddress"/> + <ref name="vf-tokenid"/> </group> <group> <attribute name="type">
Any XML addition _must_ come with correspondign documentation update. Please document the new flag and how it's supposed to be used in docs/formatdomain.rst.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e475d5b1a..0d4866cd56 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3138,6 +3138,7 @@ virPCIHeaderTypeToString; virPCIIsVirtualFunction; virPCIStubDriverTypeFromString; virPCIStubDriverTypeToString; +virPCIVFIOTokenIDIsPresent; virPCIVirtualFunctionListFree; virZPCIDeviceAddressIsIncomplete; virZPCIDeviceAddressIsPresent;
This hunk and all others most likely belong to the patch which was defining the function or vice-versa.
diff --git a/src/util/virpci.c b/src/util/virpci.c index 08b82708b1..dfb0f29182 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2312,6 +2312,11 @@ virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr) return addr->uid.isSet || addr->fid.isSet; }
+bool +virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token) +{ + return token->isSet; +}
void virPCIVirtualFunctionListFree(virPCIVirtualFunctionList *list) -- 2.25.1
Introduce a validation function for vf-token support in qemu and generate vf-token device attribute in qmeu command line. Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/qemu/qemu_command.c | 27 ++++++++++++++++++++++++--- src/qemu/qemu_validate.c | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8a7b80719f..91d7836f5c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1979,7 +1979,6 @@ qemuBuildZPCIDevProps(virDomainDeviceInfo *dev) return props; } - static int qemuCommandAddExtDevice(virCommand *cmd, virDomainDeviceInfo *dev, @@ -4729,6 +4728,8 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def, pcisrc->addr.slot, pcisrc->addr.function); const char *failover_pair_id = NULL; + g_autofree char *token = NULL; + int ret = 0; /* caller has to assign proper passthrough backend type */ switch (pcisrc->backend) { @@ -4755,13 +4756,33 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def, teaming->persistent) failover_pair_id = teaming->persistent; - if (virJSONValueObjectAdd(&props, + if ((dev->info->pciAddrExtFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) && + pcisrc->addr.token.isSet) { + const unsigned char *uuid = pcisrc->addr.token.uuid; + + token = g_strdup_printf(VIR_PCI_DEVICE_TOKEN_FMT, + uuid[0], uuid[1], uuid[2], uuid[3], + uuid[4], uuid[5], uuid[6], uuid[7], + uuid[8], uuid[9], uuid[10], uuid[11], + uuid[12], uuid[13], uuid[14], uuid[15]); + + ret = virJSONValueObjectAdd(&props, "s:driver", "vfio-pci", "s:host", host, + "s:vf-token", token, "s:id", dev->info->alias, "p:bootindex", dev->info->effectiveBootIndex, "S:failover_pair_id", failover_pair_id, - NULL) < 0) + NULL); + } else + ret = virJSONValueObjectAdd(&props, + "s:driver", "vfio-pci", + "s:host", host, + "s:id", dev->info->alias, + "p:bootindex", dev->info->effectiveBootIndex, + "S:failover_pair_id", failover_pair_id, + NULL); + if (ret < 0) return NULL; if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 93df9e4c8e..d628949597 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1363,6 +1363,23 @@ qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfo *info, return 0; } +static int +qemuValidateDomainDeviceDefVFTokenId(virDomainDeviceInfo *info, + virQEMUCaps *qemuCaps) +{ + virPCIDeviceToken *vftoken = &info->addr.pci.token; + + if (virPCIVFIOTokenIDIsPresent(vftoken) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_VFTOKEN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("This QEMU binary doesn't support vf token ids")); + return -1; + } + + return 0; +} + static int qemuValidateDomainDeviceDefAddressDrive(virDomainDeviceInfo *info, @@ -1483,6 +1500,8 @@ qemuValidateDomainDeviceDefAddress(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (qemuValidateDomainDeviceDefZPCIAddress(info, qemuCaps) < 0) return -1; + if (qemuValidateDomainDeviceDefVFTokenId(info, qemuCaps) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: -- 2.25.1
On Thu, Oct 05, 2023 at 16:05:04 -0700, Vivek Kashyap wrote:
Introduce a validation function for vf-token support in qemu and generate vf-token device attribute in qmeu command line.
Signed-off-by: Vivek Kashyap <vivek.kashyap@linux.intel.com> --- src/qemu/qemu_command.c | 27 ++++++++++++++++++++++++--- src/qemu/qemu_validate.c | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-)
Missing qemuxml2argvtest and qemuxml2xmtest additions, which are absolutely required for anything that adds/changes the commandline syntax.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8a7b80719f..91d7836f5c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1979,7 +1979,6 @@ qemuBuildZPCIDevProps(virDomainDeviceInfo *dev) return props; }
-
Spurious whitespace change. As noted we do 2 lines between functions.
static int qemuCommandAddExtDevice(virCommand *cmd, virDomainDeviceInfo *dev, @@ -4729,6 +4728,8 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def, pcisrc->addr.slot, pcisrc->addr.function); const char *failover_pair_id = NULL; + g_autofree char *token = NULL; + int ret = 0;
/* caller has to assign proper passthrough backend type */ switch (pcisrc->backend) { @@ -4755,13 +4756,33 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def, teaming->persistent) failover_pair_id = teaming->persistent;
- if (virJSONValueObjectAdd(&props, + if ((dev->info->pciAddrExtFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) && + pcisrc->addr.token.isSet) {
Broken alignment/code style.
+ const unsigned char *uuid = pcisrc->addr.token.uuid; + + token = g_strdup_printf(VIR_PCI_DEVICE_TOKEN_FMT, + uuid[0], uuid[1], uuid[2], uuid[3], + uuid[4], uuid[5], uuid[6], uuid[7], + uuid[8], uuid[9], uuid[10], uuid[11], + uuid[12], uuid[13], uuid[14], uuid[15]);
Broken alignment/code style.
+ + ret = virJSONValueObjectAdd(&props, "s:driver", "vfio-pci", "s:host", host, + "s:vf-token", token, "s:id", dev->info->alias, "p:bootindex", dev->info->effectiveBootIndex, "S:failover_pair_id", failover_pair_id, - NULL) < 0) + NULL);
So firstly you break the code style here by your addition. Secondly there's no need to duplicate the whole virJSONValueObjectAdd block in the first place as we allow conditional formatting of fields. You need to fill the 'token' field only optionally what you do, but then you can unconditionally format it using: "S:vf-token", token, argument tuple in virJSONValueObjectAdd. The 'S' modifier formats the attribute only if the string argument is non-NULL.
+ } else
Trailing whitespace, but this block is not needed ... and in fact not desired as described above.
+ ret = virJSONValueObjectAdd(&props, + "s:driver", "vfio-pci", + "s:host", host, + "s:id", dev->info->alias, + "p:bootindex", dev->info->effectiveBootIndex, + "S:failover_pair_id", failover_pair_id, + NULL); + if (ret < 0) return NULL;
if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0)
On Thu, Oct 05, 2023 at 16:04:59 -0700, Vivek Kashyap wrote:
vf token is set by a vfio-pci based PF driver and it must be known to the vfio-pci based VF driver. This vf-token is set by the PF driver before VF drivers can access the device. vfio-pci driver and qemu support vf-token. This RFC patch series adds support to provide the vf-token (uuid format) in the domain XML and to generate the qemu commandline including the vf-token.
To support vf-token the new element will be used as follows:
<hostdev mode='subsystem' type='pci' managed='yes'> <driver name='vfio'/> <source> <address domain='0x0000' bus='0x0' slot='0x00' function='0x1'> <vf-token uuid='00112233-4455-6677-8899-aabbccddeeff'/> </address> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </hostdev>
The generated commandline will include the following:
-device {"driver":"vfio-pci","host":"0000:00:0.1", "vf-token":"00112233-4455-6677-8899-aabbccddeeff", "id":"hostdev0","bus":"pci.0","addr":"0x1"}
This patch is get feedback on the approach. Will post with add documentation and testcases in follow-up.
Vivek Kashyap (5): virpci: Define vf-token qemu: vf-token capability conf: vf-token flag conf: vf-token parsing and formatting qemu: validate and generate vf-token on command line
Please note that my forthcoming review will not include any comments on whether this feature itself makes sense or whether the XML desing you are proposing is reasonable ...
src/conf/device_conf.c | 31 +++++++++++++++++++++++++++++-- src/conf/device_conf.h | 3 +++ src/conf/domain_addr.h | 1 + src/conf/domain_conf.c | 5 +++++ src/conf/schemas/basictypes.rng | 11 +++++++++++ src/conf/schemas/domaincommon.rng | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 27 ++++++++++++++++++++++++--- src/qemu/qemu_domain_address.c | 3 +++ src/qemu/qemu_validate.c | 19 +++++++++++++++++++ src/util/virpci.c | 5 +++++ src/util/virpci.h | 11 +++++++++++ 14 files changed, 117 insertions(+), 5 deletions(-)
This is mostly because the patches do not contain any changes to documentation that would explain it and I'm not familiar with what the feature is supposed to do. Thus my comments will be purely for the code itself and a further review will be required.
On Fri, 6 Oct 2023, Peter Krempa wrote: ...
This is mostly because the patches do not contain any changes to documentation that would explain it and I'm not familiar with what the feature is supposed to do. Thus my comments will be purely for the code itself and a further review will be required.
Thanks! I've gone over your comments on the patches and will repost next week with the fixes and include documentation and tests. -vk
participants (2)
-
Peter Krempa -
Vivek Kashyap