Added qemu's property check for virtio-net. And added capability QEMU_CAPS_VIRTIO_RSS. With "rss" and "rss_hash_report" from domain config, qemu should enable "rss" and "hash" for virtio-net. Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_validate.c | 16 ++++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9558938866..7cd6dab259 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -637,6 +637,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "confidential-guest-support", "query-display-options", "s390-pv-guest", + "virtio-net.rss", ); @@ -1421,6 +1422,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = { { "failover", QEMU_CAPS_VIRTIO_NET_FAILOVER, NULL }, { "packed", QEMU_CAPS_VIRTIO_PACKED_QUEUES, NULL }, { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL }, + { "rss", QEMU_CAPS_VIRTIO_RSS, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPCIeRootPort[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 2b1bb57a49..6b2446fe5f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -617,6 +617,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT, /* -machine confidential-guest-support */ QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command present */ QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */ + QEMU_CAPS_VIRTIO_RSS, /* virtio-net rss feature */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index a964c8593d..265938a99f 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1607,6 +1607,22 @@ qemuValidateDomainDeviceDefNetwork(const virDomainNetDef *net, } } + if (net->driver.virtio.rss && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_RSS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio rss is not supported with this " + "QEMU binary")); + return -1; + } + + if (net->driver.virtio.rss_hash_report && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_RSS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio rss hash report is not supported with this " + "QEMU binary")); + return -1; + } + if (net->mtu && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.31.1

Similar to virSocketRecvFD() added virSocketRecvMultipleFDs(). This function returns multiple fds through unix socket. New function is required for "qemu-ebpf-rss-helper" program. The helper may pass few file descriptors - eBPF program and maps. Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; --- src/libvirt_private.syms | 1 + src/util/virsocket.c | 83 ++++++++++++++++++++++++++++++++++++++++ src/util/virsocket.h | 2 + 3 files changed, 86 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43493ea76e..6987ff00c2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3226,6 +3226,7 @@ virSecureEraseString; # util/virsocket.h virSocketRecvFD; virSocketSendFD; +virSocketRecvMultipleFDs; # util/virsocketaddr.h diff --git a/src/util/virsocket.c b/src/util/virsocket.c index b971da16e3..da8af42e72 100644 --- a/src/util/virsocket.c +++ b/src/util/virsocket.c @@ -486,6 +486,82 @@ virSocketRecvFD(int sock, int fdflags) return fd; } + + +/* virSocketRecvMultipleFDs receives few file descriptors through the socket. + The flags are a bitmask, possibly including O_CLOEXEC (defined in <fcntl.h>). + + Return the number of recived file descriptors on success, + or -1 with errno set in case of error. +*/ +int +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags) +{ + char byte = 0; + struct iovec iov; + struct msghdr msg; + int ret = -1; + ssize_t len; + struct cmsghdr *cmsg; + char buf[CMSG_SPACE(sizeof(int) * nfds)]; + int fdflags_recvmsg = fdflags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0; + int fdSize = -1; + int i = 0; + int saved_errno = 0; + + if ((fdflags & ~O_CLOEXEC) != 0) { + errno = EINVAL; + return -1; + } + + /* send at least one char */ + memset(&msg, 0, sizeof(msg)); + iov.iov_base = &byte; + iov.iov_len = 1; + msg.msg_iov = &iov; + msg.msg_iovlen = 1; + msg.msg_name = NULL; + msg.msg_namelen = 0; + + msg.msg_control = buf; + msg.msg_controllen = sizeof(buf); + + len = recvmsg(sock, &msg, fdflags_recvmsg); + if (len < 0) { + return -1; + } + + cmsg = CMSG_FIRSTHDR(&msg); + /* be paranoiac */ + if (len == 0 || cmsg == NULL || cmsg->cmsg_len < CMSG_LEN(sizeof(int)) + || cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) { + /* fake errno: at end the file is not available */ + errno = len ? EACCES : ENOTCONN; + return -1; + } + + fdSize = cmsg->cmsg_len - CMSG_LEN(0); + memcpy(fds, CMSG_DATA(cmsg), fdSize); + ret = fdSize/sizeof(int); + + /* set close-on-exec flag */ + if (!MSG_CMSG_CLOEXEC && (fdflags & O_CLOEXEC)) { + for (i = 0; i < ret; ++i) { + if (virSetCloseExec(fds[i]) < 0) { + saved_errno = errno; + goto error; + } + } + } + + return ret; +error: + for (i = 0; i < ret; ++i) { + VIR_FORCE_CLOSE(fds[i]); + } + errno = saved_errno; + return -1; +} #else /* WIN32 */ int virSocketSendFD(int sock G_GNUC_UNUSED, int fd G_GNUC_UNUSED) @@ -500,4 +576,11 @@ virSocketRecvFD(int sock G_GNUC_UNUSED, int fdflags G_GNUC_UNUSED) errno = ENOSYS; return -1; } + +int +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags) +{ + errno = ENOSYS; + return -1; +} #endif /* WIN32 */ diff --git a/src/util/virsocket.h b/src/util/virsocket.h index 419da8b3ae..c926effbc3 100644 --- a/src/util/virsocket.h +++ b/src/util/virsocket.h @@ -22,6 +22,8 @@ int virSocketSendFD(int sock, int fd); int virSocketRecvFD(int sock, int fdflags); +int +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags); #ifdef WIN32 -- 2.31.1

Added qmp call to receive helper path. Also added monitor functions. For virQEMUCaps added field for helper path. Libvirt queries "query-helper-paths" from qmp and saves in qemuCaps. The qemu and the helper should be "sync" and the helper should return proper eBPF fd and maps with "interface" for current qemu. So, qemu returns where to find the helper through qmp, if the qemu supports "ebpf_rss_fds". Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; --- src/qemu/qemu_capabilities.c | 44 +++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 3 +++ src/qemu/qemu_monitor.c | 9 +++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 50 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 +++ 6 files changed, 112 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f16115226d..f62088d32f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -722,6 +722,9 @@ struct _virQEMUCaps { /* Capabilities which may differ depending on the accelerator. */ virQEMUCapsAccel kvm; virQEMUCapsAccel tcg; + + /* Helpers returned by qemu */ + char *helperEbpfRSS; }; struct virQEMUCapsSearchData { @@ -1994,6 +1997,8 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps) qemuCaps->sevCapabilities) < 0) goto error; + ret->helperEbpfRSS = g_strdup(qemuCaps->helperEbpfRSS); + return ret; error: @@ -2037,6 +2042,8 @@ void virQEMUCapsDispose(void *obj) virQEMUCapsAccelClear(&qemuCaps->kvm); virQEMUCapsAccelClear(&qemuCaps->tcg); + + g_free(qemuCaps->helperEbpfRSS); } void @@ -4418,6 +4425,8 @@ virQEMUCapsLoadCache(virArch hostArch, if (virXPathBoolean("boolean(./kvmSupportsSecureGuest)", ctxt) > 0) qemuCaps->kvmSupportsSecureGuest = true; + qemuCaps->helperEbpfRSS = virXPathString("string(./EbpfHelperPath)", ctxt); + if (skipInvalidation) qemuCaps->invalidation = false; @@ -4671,6 +4680,10 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) if (qemuCaps->kvmSupportsSecureGuest) virBufferAddLit(&buf, "<kvmSupportsSecureGuest/>\n"); + if (qemuCaps->helperEbpfRSS) { + virBufferAsprintf(&buf, "<EbpfHelperPath>%s</EbpfHelperPath>\n", qemuCaps->helperEbpfRSS); + } + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); @@ -5265,6 +5278,30 @@ virQEMUCapsGetVirtType(virQEMUCaps *qemuCaps) return type; } +static int +virQEMUCapsProbeQMPHelperPath(virQEMUCaps *qemuCaps, + qemuMonitor *mon) +{ + g_autoptr(GHashTable) helperList = NULL; + const char *entry = NULL; + + helperList = qemuMonitorGetHelperPath(mon); + + if (!helperList) { + return -1; + } + + /* TODO: parse all helpers? */ + entry = virHashLookup(helperList, "qemu-ebpf-rss-helper"); + if (!entry) { + return -1; + } + + qemuCaps->helperEbpfRSS = g_strdup(entry); + + return 0; +} + int virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps, qemuMonitor *mon) @@ -5345,6 +5382,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps, if (virQEMUCapsProbeQMPHostCPU(qemuCaps, accel, mon, type) < 0) return -1; + virQEMUCapsProbeQMPHelperPath(qemuCaps, mon); + return 0; } @@ -6459,3 +6498,8 @@ virQEMUCapsStripMachineAliases(virQEMUCaps *qemuCaps) virQEMUCapsStripMachineAliasesForVirtType(qemuCaps, VIR_DOMAIN_VIRT_KVM); virQEMUCapsStripMachineAliasesForVirtType(qemuCaps, VIR_DOMAIN_VIRT_QEMU); } + +const char *virQEMUCapsGetEBPFHelperPath(virQEMUCaps *qemuCaps) +{ + return qemuCaps->helperEbpfRSS; +} diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index c938b63e91..b25bdd4ec3 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -816,3 +816,6 @@ virQEMUCapsGetKVMSupportsSecureGuest(virQEMUCaps *qemuCaps) G_GNUC_NO_INLINE; virArch virQEMUCapsArchFromString(const char *arch); const char *virQEMUCapsArchToString(virArch arch); + +const char * +virQEMUCapsGetEBPFHelperPath(virQEMUCaps *qemuCaps); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6e2d8010c5..585fb2d6b4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4617,3 +4617,12 @@ qemuMonitorQueryDirtyRate(qemuMonitor *mon, return qemuMonitorJSONQueryDirtyRate(mon, info); } + + +GHashTable * +qemuMonitorGetHelperPath(qemuMonitor *mon) +{ + QEMU_CHECK_MONITOR_NULL(mon); + + return qemuMonitorJSONGetHelperPath(mon); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1491c1297c..0ee87d2c46 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1491,3 +1491,6 @@ struct _qemuMonitorDirtyRateInfo { int qemuMonitorQueryDirtyRate(qemuMonitor *mon, qemuMonitorDirtyRateInfo *info); + +GHashTable * +qemuMonitorGetHelperPath(qemuMonitor *mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ca2c3bb6cf..0579eee36a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9387,3 +9387,53 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon, return qemuMonitorJSONExtractDirtyRateInfo(data, info); } + +static int +qemuMonitorJSONGetHelperPathWorker(size_t pos G_GNUC_UNUSED, + virJSONValue *item, + void *opaque) +{ + const char *name = virJSONValueObjectGetString(item, "name"); + const char *path = virJSONValueObjectGetString(item, "path"); + GHashTable *pathsList = opaque; + + if (!name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("reply data was missing 'name'")); + return -1; + } + + if (virHashAddEntry(pathsList, name, g_strdup(path)) < 0) + return -1; + + return 0; +} + +GHashTable * +qemuMonitorJSONGetHelperPath(qemuMonitor *mon) +{ + g_autoptr(GHashTable) pathsList = virHashNew(g_free); + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-helper-paths", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return NULL; + + /* return empty hash */ + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) + return g_steal_pointer(&pathsList); + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + return NULL; + + if (virJSONValueArrayForeachSteal(virJSONValueObjectGetArray(reply, "return"), + qemuMonitorJSONGetHelperPathWorker, + pathsList) < 0) + return NULL; + + return g_steal_pointer(&pathsList); +} + diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 01a3ba25f1..80e47cf68f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -706,3 +706,6 @@ qemuMonitorJSONStartDirtyRateCalc(qemuMonitor *mon, int qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon, qemuMonitorDirtyRateInfo *info); + +GHashTable * +qemuMonitorJSONGetHelperPath(qemuMonitor *mon); -- 2.31.1

The helper called before NIC string creation. EBPF fds passed to the child process. The helper called if qemu supports "ebpf_rss_fds" and returns the helper path. If libvirt can't retrieve fds from the helper, qemu will be launched without "ebpf_rss_fds" virtio-net property. Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; --- src/qemu/qemu_command.c | 36 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_command.h | 2 ++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 67a396a513..2fffcee609 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3377,11 +3377,14 @@ qemuBuildNicDevStr(virDomainDef *def, virDomainNetDef *net, unsigned int bootindex, size_t vhostfdSize, + char **ebpfRSSfds, + size_t ebpfRSSnfds, virQEMUCaps *qemuCaps) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; bool usingVirtio = false; char macaddr[VIR_MAC_STRING_BUFLEN]; + size_t i = 0; if (virDomainNetIsVirtioModel(net)) { if (qemuBuildVirtioDevStr(&buf, "virtio-net", qemuCaps, @@ -3498,6 +3501,15 @@ qemuBuildNicDevStr(virDomainDef *def, if (net->driver.virtio.rss == VIR_TRISTATE_SWITCH_ON) { virBufferAsprintf(&buf, ",rss=%s", virTristateSwitchTypeToString(net->driver.virtio.rss)); + + if (ebpfRSSfds != NULL && ebpfRSSnfds) + { + virBufferAsprintf(&buf, ",ebpf_rss_fds="); + for (i = 0; i < (ebpfRSSnfds - 1); ++i) { + virBufferAsprintf(&buf, "%s:",ebpfRSSfds[i]); + } + virBufferAsprintf(&buf, "%s",ebpfRSSfds[ebpfRSSnfds - 1]); + } } if (net->driver.virtio.rss_hash_report == VIR_TRISTATE_SWITCH_ON) { @@ -8492,6 +8504,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, qemuSlirp *slirp; size_t i; g_autoptr(virJSONValue) hostnetprops = NULL; + char **ebpfRSSfdsName = NULL; + int ebpfRSSfds[16] = {}; + int ebpfRSSnfds = 0; if (!bootindex) @@ -8744,8 +8759,20 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, if (qemuCommandAddExtDevice(cmd, &net->info) < 0) goto cleanup; + if (net->driver.virtio.rss == VIR_TRISTATE_SWITCH_ON + && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_EBPF_RSS_FDS)) { + ebpfRSSnfds = qemuEbpfRssHelper(virQEMUCapsGetEBPFHelperPath(qemuCaps), ebpfRSSfds, 16); + if (ebpfRSSnfds > 0) { + ebpfRSSfdsName = g_new0(char *, ebpfRSSnfds); + for (i = 0; i < ebpfRSSnfds; ++i) { + ebpfRSSfdsName[i] = g_strdup_printf("%d", ebpfRSSfds[i]); + virCommandPassFD(cmd, ebpfRSSfds[i], VIR_COMMAND_PASS_FD_CLOSE_PARENT); + } + } + } + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, - net->driver.virtio.queues, qemuCaps))) + net->driver.virtio.queues, ebpfRSSfdsName, ebpfRSSnfds, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); } else if (!requireNicdev) { @@ -8785,6 +8812,13 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, } VIR_FREE(tapfdName); VIR_FREE(vhostfd); + for (i = 0; ret < 0 && ebpfRSSfds[i] && i < 16; i++) { + if (ret < 0) + VIR_FORCE_CLOSE(ebpfRSSfds[i]); + if (ebpfRSSfdsName) + VIR_FREE(ebpfRSSfdsName[i]); + } + VIR_FREE(ebpfRSSfdsName); VIR_FREE(tapfd); VIR_FORCE_CLOSE(vdpafd); return ret; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 188e63ea1f..cab0bb7770 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -107,6 +107,8 @@ char *qemuBuildNicDevStr(virDomainDef *def, virDomainNetDef *net, unsigned int bootindex, size_t vhostfdSize, + char **ebpf_rss_fds, + size_t nfds, virQEMUCaps *qemuCaps); char *qemuDeviceDriveHostAlias(virDomainDiskDef *disk); -- 2.31.1

Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; --- docs/formatdomain.rst | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 61ccd8895a..22ad19439f 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -5233,6 +5233,22 @@ following attributes are available for the ``"virtio"`` NIC driver: only for ``vhostuser`` type. :since:`Since 3.7.0 (QEMU and KVM only)` **In general you should leave this option alone, unless you are very certain you know what you are doing.** +``rss`` + The ``rss`` option enables in-qemu/ebpf RSS for virtio NIC. RSS works with + virtio and tap backends only. For ebpf RSS the "ebpf helper" is used. Helper + binary path received from qemu by qmp. The helper is't used if qemu doesn't + provide "ebpf_rss_fds" property or helper path. Without helper, virtio NIC + will be launched only with "rss" property. Qemu may load eBPF by itself if it + has CAP_SYS_ADMIN permissions. In other cases, "in-qemu" RSS is used. + **In general you should leave this option alone, unless you are very certain + you know what you are doing.** +``rss_hash_report`` + The ``rss_hash_report`` option enables in-qemu RSS hash report for virtio + NIC. Ebpf RSS doesn't support hash report yet. Usually enabled alongside with + ``rss``. Without ``rss`` option, the hash report doesn't affect steering + itself but provides vnet header with a calculated hash. + **In general you should leave this option alone, unless you are very certain + you know what you are doing.** virtio options For virtio interfaces, `Virtio-specific options <#elementsVirtio>`__ can also be set. ( :since:`Since 3.5.0` ) -- 2.31.1

On 7/28/21 10:17 AM, Andrew Melnychenko wrote: I think it's sufficient to check just for ebpfRSSnfds > 0 because it can't happen that these two variable disagree. This constant look magic. I'm this this ret < 0 check is duplicated. Also usually we write it like this: if (ret < 0) { for () ; /* other cleanup code goes here */ } Michal

On 7/28/21 10:17 AM, Andrew Melnychenko wrote: I haven't found this in QEMU. Looking into qemu-devel list it was proposed but apparently is not merged yet: https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg03536.html

On 7/28/21 10:17 AM, Andrew Melnychenko wrote: This needs to be ordered. The correct order is: # util/virsocket.h virSocketRecvFD; virSocketRecvMultipleFDs; virSocketSendFD; Please put a space before and after '/'. Like this: ret = fdSize / sizeof(int); This isn't needed really, because .. .. VIR_FORCE_CLOSE() doesn't change errno. But I wonder if this function is needed. I mean, we currently have virSocketRecvFD() and this new function looks very much like it. Would it be possible to turn virSocketRecvFD() into virSocketRecvMultipleFDs() and fix all (current) callers of virSocketRecvFD()? Alternatively, we can have virSocketRecvMultipleFDs() as you propose and then virSocketRecvFD() be just a thin wrapper over virSocketRecvMultipleFDs(), e.g. like this: virSocketRecvFD() { int fds[1]; virSocketRecvMultipleFDs(sock, fds, 1, fdflags); return fds[0]; } Or even better, have virSocketRecvFD() return FD via argument and its retval be 0/-1 (success/fail). My aim is to avoid having nearly the same code twice. Michal

On 7/28/21 10:17 AM, Andrew Melnychenko wrote: This needs to be updated because meanwhile we introduced comments to these enum items. Michal

On 8/20/21 2:57 PM, Michal Prívozník wrote: Ooops, hit send too early. What I wanted to say is that it's perfectly okay to send patches before QEMU part is merged. However, libvirt part won't be merged before QEMU. We did this mistake too many times before and learned our lesson. Michal

Hi, Yea, it's a good idea. On Fri, Aug 20, 2021 at 3:57 PM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: > On 7/28/21 10:17 AM, Andrew Melnychenko wrote: > > Similar to virSocketRecvFD() added virSocketRecvMultipleFDs(). > > This function returns multiple fds through unix socket. > > New function is required for "qemu-ebpf-rss-helper" program. > > The helper may pass few file descriptors - eBPF program and maps. > > > > Signed-off-by: Andrew Melnychenko <andrew(a)daynix.com&gt; > > --- > > src/libvirt_private.syms | 1 + > > src/util/virsocket.c | 83 ++++++++++++++++++++++++++++++++++++++++ > > src/util/virsocket.h | 2 + > > 3 files changed, 86 insertions(+) > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 43493ea76e..6987ff00c2 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -3226,6 +3226,7 @@ virSecureEraseString; > > # util/virsocket.h > > virSocketRecvFD; > > virSocketSendFD; > > +virSocketRecvMultipleFDs; > > This needs to be ordered. The correct order is: > > # util/virsocket.h > virSocketRecvFD; > virSocketRecvMultipleFDs; > virSocketSendFD; > > > > > > > > # util/virsocketaddr.h > > diff --git a/src/util/virsocket.c b/src/util/virsocket.c > > index b971da16e3..da8af42e72 100644 > > --- a/src/util/virsocket.c > > +++ b/src/util/virsocket.c > > @@ -486,6 +486,82 @@ virSocketRecvFD(int sock, int fdflags) > > > > return fd; > > } > > + > > + > > +/* virSocketRecvMultipleFDs receives few file descriptors through the > socket. > > + The flags are a bitmask, possibly including O_CLOEXEC (defined in > <fcntl.h>). > > + > > + Return the number of recived file descriptors on success, > > + or -1 with errno set in case of error. > > +*/ > > +int > > +virSocketRecvMultipleFDs(int sock, int *fds, size_t nfds, int fdflags) > > +{ > > + char byte = 0; > > + struct iovec iov; > > + struct msghdr msg; > > + int ret = -1; > > + ssize_t len; > > + struct cmsghdr *cmsg; > > + char buf[CMSG_SPACE(sizeof(int) * nfds)]; > > + int fdflags_recvmsg = fdflags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0; > > + int fdSize = -1; > > + int i = 0; > > + int saved_errno = 0; > > + > > + if ((fdflags & ~O_CLOEXEC) != 0) { > > + errno = EINVAL; > > + return -1; > > + } > > + > > + /* send at least one char */ > > + memset(&msg, 0, sizeof(msg)); > > + iov.iov_base = &byte; > > + iov.iov_len = 1; > > + msg.msg_iov = &iov; > > + msg.msg_iovlen = 1; > > + msg.msg_name = NULL; > > + msg.msg_namelen = 0; > > + > > + msg.msg_control = buf; > > + msg.msg_controllen = sizeof(buf); > > + > > + len = recvmsg(sock, &msg, fdflags_recvmsg); > > + if (len < 0) { > > + return -1; > > + } > > + > > + cmsg = CMSG_FIRSTHDR(&msg); > > + /* be paranoiac */ > > + if (len == 0 || cmsg == NULL || cmsg->cmsg_len < > CMSG_LEN(sizeof(int)) > > + || cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != > SCM_RIGHTS) { > > + /* fake errno: at end the file is not available */ > > + errno = len ? EACCES : ENOTCONN; > > + return -1; > > + } > > + > > + fdSize = cmsg->cmsg_len - CMSG_LEN(0); > > + memcpy(fds, CMSG_DATA(cmsg), fdSize); > > + ret = fdSize/sizeof(int); > > Please put a space before and after '/'. Like this: > > ret = fdSize / sizeof(int); > > > + > > + /* set close-on-exec flag */ > > + if (!MSG_CMSG_CLOEXEC && (fdflags & O_CLOEXEC)) { > > + for (i = 0; i < ret; ++i) { > > + if (virSetCloseExec(fds[i]) < 0) { > > + saved_errno = errno; > > This isn't needed really, because .. > > > + goto error; > > + } > > + } > > + } > > + > > + return ret; > > +error: > > + for (i = 0; i < ret; ++i) { > > + VIR_FORCE_CLOSE(fds[i]); > > .. VIR_FORCE_CLOSE() doesn't change errno. > > > + } > > + errno = saved_errno; > > + return -1; > > +} > > But I wonder if this function is needed. I mean, we currently have > virSocketRecvFD() and this new function looks very much like it. Would > it be possible to turn virSocketRecvFD() into virSocketRecvMultipleFDs() > and fix all (current) callers of virSocketRecvFD()? > > Alternatively, we can have virSocketRecvMultipleFDs() as you propose and > then virSocketRecvFD() be just a thin wrapper over > virSocketRecvMultipleFDs(), e.g. like this: > > Or even better, have virSocketRecvFD() return FD via argument and its > retval be 0/-1 (success/fail). > > My aim is to avoid having nearly the same code twice. > > Michal > >

Hi, Ok, I'll update in RFC v2. On Fri, Aug 20, 2021 at 3:57 PM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote:

Hi, No problem, those patches are RFC. If there would be issues with the libvirt interface - qemu patches should not be applied either. On Fri, Aug 20, 2021 at 3:59 PM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: