Re: [RFC: vf-token 0/5] Introduce vf-token when using userspace PF
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
Re: [RFC: vf-token 2/5] qemu: vf-token capability
On Thu, Oct 05, 2023 at 16:05:01 -0700, Vivek Kashyap wrote:
> Introduce qemu capability for vf-token
>
> Signed-off-by: Vivek Kashyap
> ---
> 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.
[RFC: vf-token 4/5] conf: vf-token parsing and formatting
Introduce XML parsing and formatting of vf-token attribute
Signed-off-by: Vivek Kashyap
---
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(>addr.pci.zpci);
}
+bool
+virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci)
+{
+return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
+virZPCIDeviceAddressIsPresent(>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(>addr.pci.zpci);
+return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) &&
+virDeviceExtensionIsPresent(>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(>addr.pci.token)) {
+virBufferAsprintf(,
+ "\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 @@
+
+
+
+
+
+
+
+
+
+
+
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 @@
+
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
[RFC: vf-token 3/5] conf: vf-token flag
Define PCI address extension flag for vf-token
Signed-off-by: Vivek Kashyap
---
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
[RFC: vf-token 1/5] virpci: Define vf-token
Define the vf-token extension for PCI device
Signed-off-by: Vivek Kashyap
---
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
[RFC: vf-token 5/5] qemu: validate and generate vf-token on command line
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
---
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(,
+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(,
"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(,
+ "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 = >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
[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:
The generated commandline will include the following:
-device {"driver":"vfio-pci","host":":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
[RFC: vf-token 2/5] qemu: vf-token capability
Introduce qemu capability for vf-token
Signed-off-by: Vivek Kashyap
---
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
Re: [RFC: vf-token 3/5] conf: vf-token flag
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 > --- > 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.
Re: [RFC: vf-token 4/5] conf: vf-token parsing and formatting
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
> ---
> 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(>addr.pci.zpci);
> }
>
> +bool
> +virDeviceExtensionIsPresent(const virPCIDeviceAddress *pci)
> +{
> +return ((pci->extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> +virZPCIDeviceAddressIsPresent(>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(>addr.pci.zpci);
> +return (info->addr.pci.extFlags != VIR_PCI_ADDRESS_EXTENSION_NONE) &&
> +virDeviceExtensionIsPresent(>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(>addr.pci.token)) {
> +virBufferAsprintf(,
> + "\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 @@
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
>
>
> 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 @@
>
>
>
> +
>
>
>
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
Re: [RFC: vf-token 0/5] Introduce vf-token when using userspace PF
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:
>
>
>
>
>
>
>
>
>
>
>
> The generated commandline will include the following:
>
> -device {"driver":"vfio-pci","host":":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.
Re: [RFC: vf-token 1/5] virpci: Define vf-token
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
> ---
> 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.
Re: [RFC: vf-token 5/5] qemu: validate and generate vf-token on command line
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
> ---
> 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(,
> +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(,
>"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(,
> + "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)
