Re: [RFC PATCH 2/4] qemu_capabilities: Added new capability ebpf_rss_fds for QEMU.

On Mon, Oct 09, 2023 at 09:16:12 +0300, Andrew Melnychenko wrote:

Also, added logic for retrieving eBPF objects from QEMU. eBPF objects stored in the hash table during runtime. eBPF objects cached encoded in base64 in the .xml cache file.

Signed-off-by: Andrew Melnychenko <andrew@daynix.com> --- src/qemu/qemu_capabilities.c | 181 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 4 + 2 files changed, 185 insertions(+)

Once again, make sure to check that each patch builds and passes tests. This one fails to compile: u/libvirt_driver_qemu_impl.a.p/qemu_capabilities.c.o -c ../../../libvirt/src/qemu/qemu_capabilities.c ../../../libvirt/src/qemu/qemu_capabilities.c:808:6: error: no previous prototype for ‘virQEMUEbpfOBjectDataDestroy’ [-Werror=missing-prototypes] 808 | void virQEMUEbpfOBjectDataDestroy(gpointer data) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c: In function ‘virQEMUEbpfOBjectDataDestroy’: ../../../libvirt/src/qemu/qemu_capabilities.c:811:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 811 | struct virQEMUEbpfOBjectData *object = data; | ^~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c: In function ‘virQEMUCapsProbeQMPEbpfObject’: ../../../libvirt/src/qemu/qemu_capabilities.c:5560:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5560 | struct virQEMUEbpfOBjectData *object = g_malloc(sizeof(*object)); | ^~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c: In function ‘virQEMUCapsProbeQMPSchemaEbpf’: ../../../libvirt/src/qemu/qemu_capabilities.c:5573:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5573 | virJSONValue *requestSchema = virHashLookup(schema, "request-ebpf"); | ^~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5577:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5577 | const char *argType = virJSONValueObjectGetString(requestSchema, "arg-type"); | ^~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5581:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5581 | virJSONValue *argSchema = virHashLookup(schema, argType); | ^~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5586:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5586 | virJSONValue *members = virJSONValueObjectGetArray(argSchema, "members"); | ^~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5591:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5591 | virJSONValue *idSchema = NULL; | ^~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5601:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5601 | const char *ebpfIds = virJSONValueObjectGetString(idSchema, "type"); | ^~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5606:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5606 | virJSONValue *ebpfIdsArray = virJSONValueObjectGetArray(ebpfIdsSchema, "values"); | ^~~~~~~~~~~~ ../../../libvirt/src/qemu/qemu_capabilities.c:5611:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 5611 | virJSONValue *id = NULL; | ^~~~~~~~~~~~ cc1: all warnings being treated as errors After the compile error is fixed there are further errors caught by syntax-check.

@@ -789,6 +790,9 @@ struct _virQEMUCaps { virQEMUCapsAccel kvm; virQEMUCapsAccel hvf; virQEMUCapsAccel tcg; + + /* Hash of ebpf objects virQEMUEbpfOBjectData */ + GHashTable *ebpfObjects; };

struct virQEMUCapsSearchData { @@ -796,6 +800,18 @@ struct virQEMUCapsSearchData { const char *binaryFilter; };

+struct virQEMUEbpfOBjectData { + void *ebpfData; + size_t ebpfSize; +}; + +void virQEMUEbpfOBjectDataDestroy(gpointer data) { + if (!data) + return; + struct virQEMUEbpfOBjectData *object = data; + g_free(object->ebpfData); + g_free(data); +}

As noted, store the program as the base64 string throughout libvirt, none of the above will be needed.

static virClass *virQEMUCapsClass; static void virQEMUCapsDispose(void *obj); @@ -836,6 +852,19 @@ const char *virQEMUCapsArchToString(virArch arch) }

+const void * +virQEMUCapsGetEbpf(virQEMUCaps *qemuCaps, const char *id, size_t *size) +{ + struct virQEMUEbpfOBjectData *object = virHashLookup(qemuCaps->ebpfObjects, id); + + if (!object) + return NULL; + + *size = object->ebpfSize; + return object->ebpfData; +} + + /* Checks whether a domain with @guest arch can run natively on @host. */ bool @@ -1426,6 +1455,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioBlk[] = { static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = { { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL }, { "rss", QEMU_CAPS_VIRTIO_NET_RSS, NULL }, + { "ebpf_rss_fds", QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS, NULL },

The QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS capability is not actually connected to the 'request-ebpf' output caching. The ebpf programs are cached if the 'request-ebpf' command is present and in such case all programs are cached. Please split the patch such that one adds the 'request-ebpf' related changes and the second one adds the virtio-net capability.

};

static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPCIeRootPort[] = { @@ -1804,6 +1834,8 @@ virQEMUCapsNew(void) qemuCaps->invalidation = true; qemuCaps->flags = virBitmapNew(QEMU_CAPS_LAST);

+ qemuCaps->ebpfObjects = virHashNew(virQEMUEbpfOBjectDataDestroy); + return qemuCaps; }

@@ -1984,6 +2016,8 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps) ret->hypervCapabilities = g_memdup(qemuCaps->hypervCapabilities, sizeof(virDomainCapsFeatureHyperv));

+ ret->ebpfObjects = g_hash_table_ref(qemuCaps->ebpfObjects);

virQEMUCapsNewCopy is a deep copy function, thus you must deep copy the objects.

+ return g_steal_pointer(&ret);

[...]

@@ -4541,6 +4577,46 @@ virQEMUCapsValidateArch(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) }

+static int +virQEMUCapsParseEbpfObjects(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) +{ + g_autofree xmlNodePtr *nodes = NULL; + size_t i; + int n; + + if ((n = virXPathNodeSet("./ebpf", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu cached eBPF object")); + return -1; + } + + for (i = 0; i < n; i++) { + g_autofree char *id = NULL; + g_autofree char *ebpf = NULL; + struct virQEMUEbpfOBjectData *ebpfObject = NULL; + + if (!(id = virXMLPropString(nodes[i], "id"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing eBPF object ID in QEMU capabilities cache"));

Use virXMLPropStringRequired instead of reporting a custom error.

+ return -1; + } + + if (!(ebpf = virXMLNodeContentString(nodes[i]))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s %s",

broken format string in the error

+ _("can't get eBPF base64 data from cache for ID: "), id);

Store the program as an attribute

+ return -1; + } + + ebpfObject = g_malloc(sizeof(*ebpfObject));

we use exclusively g_new0 for allocation of structs.

+ ebpfObject->ebpfData = g_base64_decode(ebpf, &ebpfObject->ebpfSize);

As noted store this as a string you'll save this step here.

+ + virHashAddEntry(qemuCaps->ebpfObjects, id, ebpfObject); + } + + return 0; +} + + /* * Parsing a doc that looks like * @@ -4688,6 +4764,8 @@ virQEMUCapsLoadCache(virArch hostArch, if (skipInvalidation) qemuCaps->invalidation = false;

+ virQEMUCapsParseEbpfObjects(qemuCaps, ctxt);

So the above function reports many errors, here you don't bother looking for them.

+ return 0; }

@@ -4925,6 +5003,32 @@ virQEMUCapsFormatHypervCapabilities(virQEMUCaps *qemuCaps, }

+static int +virQEMUCapsFormatEbpfObjectsIterator(void *payload, const char *name, void *opaque) +{ + virBuffer *buf = opaque; + struct virQEMUEbpfOBjectData *object = payload; + g_autofree char *objectBase64 = NULL; + + if (!buf || !object)

'buf' can't be null here, and neither 'object'.

+ return 0; + + objectBase64 = g_base64_encode(object->ebpfData, object->ebpfSize); + if (!objectBase64)

g_base64_encode can't return NULL thus this check is pointless.

+ return 0; + + virBufferAsprintf(buf, "<ebpf id='%s'>\n", name); + virBufferAdjustIndent(buf, 2); + + virBufferAddStr(buf, objectBase64); + virBufferAddLit(buf, "\n"); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</ebpf>\n");

This format is not acceptable. As noted, store it into a attribute instead of the element so that we can reasonably have subelements might it become required.

+ + return 0; +} + char * virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) { @@ -5015,6 +5119,8 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) if (qemuCaps->kvmSupportsSecureGuest) virBufferAddLit(&buf, "<kvmSupportsSecureGuest/>\n");

+ virHashForEach(qemuCaps->ebpfObjects, virQEMUCapsFormatEbpfObjectsIterator, &buf);

Missing container around the '<ebpf>' elements. I know that XML allows multiple, but we prefer stashing them in a container. Also use virHashForEachSorted here. It's obviously less efficient, but the capability cache is also handled in tests thus we must use a variant which has stable output.

+ virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n");

@@ -5437,6 +5543,79 @@ virQEMUCapsInitProcessCaps(virQEMUCaps *qemuCaps) }

+static int +virQEMUCapsProbeQMPEbpfObject(virQEMUCaps *qemuCaps, const char *id, + qemuMonitor *mon) +{ + void *ebpfObject = NULL; + size_t size = 0; + + if (!id) + return -1;

This can't happen. If it did you'd not report an error.

+ + ebpfObject = qemuMonitorGetEbpf(mon, id, &size); + if(ebpfObject == NULL)

As noted in previous patch, you can't determine here that NULL is failure. And you'd propagate the non-failure (without error set) to upper layers.

+ return -1; + + struct virQEMUEbpfOBjectData *object = g_malloc(sizeof(*object)); + object->ebpfData = ebpfObject; + object->ebpfSize = size; + + virHashAddEntry(qemuCaps->ebpfObjects, id, object);

This can return error and is not checked.

+ + return 0; +} + +static void virQEMUCapsProbeQMPSchemaEbpf(virQEMUCaps *qemuCaps, GHashTable* schema, qemuMonitor *mon) { + if (schema == NULL) + return;

Can't happen.

+ + virJSONValue *requestSchema = virHashLookup(schema, "request-ebpf"); + if (!requestSchema) + return; + + const char *argType = virJSONValueObjectGetString(requestSchema, "arg-type"); + if (!argType) + return; + + virJSONValue *argSchema = virHashLookup(schema, argType); + if (!argSchema) + return; + + /* Get member id type*/ + virJSONValue *members = virJSONValueObjectGetArray(argSchema, "members"); + if (!members) + return; + + /* Find "id" type */ + virJSONValue *idSchema = NULL; + for (size_t i = 0; (idSchema = virJSONValueArrayGet(members, i)) != NULL; ++i) { + const char *name = virJSONValueObjectGetString(idSchema, "name"); + if (strncmp(name, "id", 3) == 0) + break; + } + + if (!idSchema) + return; + + const char *ebpfIds = virJSONValueObjectGetString(idSchema, "type"); + virJSONValue *ebpfIdsSchema = virHashLookup(schema, ebpfIds); + if (!ebpfIdsSchema) + return; + + virJSONValue *ebpfIdsArray = virJSONValueObjectGetArray(ebpfIdsSchema, "values"); + if (!ebpfIdsArray) + return;

Instead of all of the above please use our existing schema query infrastructure: virQEMUQAPISchemaPathGet("request-ebpf/arg-type/id", schema, &ebpfIdsArray) This should fill ebpfIdsArray with what the whole code above does. Since the above code fails to compile I'll replace it by my variant: static int virQEMUCapsProbeQMPSchemaEbpf(virQEMUCaps *qemuCaps, GHashTable* schema, qemuMonitor *mon) { virJSONValue *ebpfIdsArray; virJSONValue *ebpfIdsSchema; size_t i; if (virQEMUQAPISchemaPathGet("request-ebpf/arg-type/id", schema, &ebpfIdsSchema) != 1) return 0; if (!(ebpfIdsArray = virJSONValueObjectGetArray(ebpfIdsSchema, "values"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed QMP schema of 'request-ebpf'")); return -1; } /* Try to request every eBPF */ for (i = 0; i < virJSONValueArraySize(ebpfIdsArray); i++) { virJSONValue *id = virJSONValueArrayGet(ebpfIdsArray, i); if (virQEMUCapsProbeQMPEbpfObject(qemuCaps, virJSONValueGetString(id), mon) < 0) return -1; } return 0; }

+ + /* Try to request every eBPF */ + virJSONValue *id = NULL; + for (size_t i = 0; (id = virJSONValueArrayGet(ebpfIdsArray, i)) != NULL; ++i) { + const char *name = virJSONValueGetString(id); + virQEMUCapsProbeQMPEbpfObject(qemuCaps, name, mon);

Ah, so you in the end ignore all the errors regardless.

+ } +} + + static int virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCaps *qemuCaps, qemuMonitor *mon) @@ -5466,6 +5645,8 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCaps *qemuCaps, virQEMUCapsSet(qemuCaps, cmd->flag); }

+ virQEMUCapsProbeQMPSchemaEbpf(qemuCaps, schema, mon); + return 0; }