[PATCH 1/2] qemu: don't try to query QEMU about migration blockers during offline migration
The new code that queries QEMU about migration blockers was put at the top of qemuMigrationSrcIsAllowed(), but that function can also be called in the case of offline migration (ie when the domain is inactive / QEMU isn't running). This check should have been put inside the "if (!(flags & VIR_MIGRATE_OFFLINE))" conditional, so let's move it there. Fixes: 156e99f686690855be4e45d9b8b3194191a8bc31 Signed-off-by: Laine Stump --- src/qemu/qemu_migration.c | 34 ++ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5d1e5f987b..6fc5791f61 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1455,22 +1455,6 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, int nsnapshots; int pauseReason; size_t i; -bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps, - QEMU_CAPS_MIGRATION_BLOCKED_REASONS); - -/* Ask qemu if it have a migration blocker */ -if (blockedReasonsCap) { -g_auto(GStrv) blockers = NULL; -if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0) -return false; - -if (blockers && blockers[0]) { -g_autofree char *reasons = g_strjoinv("; ", blockers); -virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot migrate domain: %s"), reasons); -return false; -} -} /* perform these checks only when migrating to remote hosts */ if (remote) { @@ -1488,6 +1472,24 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, /* following checks don't make sense for offline migration */ if (!(flags & VIR_MIGRATE_OFFLINE)) { +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_MIGRATION_BLOCKED_REASONS); + +/* Ask qemu if it has a migration blocker */ +if (blockedReasonsCap) { +g_auto(GStrv) blockers = NULL; + +if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0) +return false; + +if (blockers && blockers[0]) { +g_autofree char *reasons = g_strjoinv("; ", blockers); +virReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain: %s"), reasons); +return false; +} +} + if (remote) { /* cancel migration if disk I/O error is emitted while migrating */ if (flags & VIR_MIGRATE_ABORT_ON_ERROR && -- 2.35.3
[PATCH 2/2] qemu: skip hardcoded hostdev migration check if QEMU can do it for us
libvirt currently will block migration for any vfio-assigned device unless it is a network device that is associated with a virtio-net failover device (ie. if the hostdev object has a teaming->type == VIR_DOMAIN_NET_TEAMING_TYPE_TRANSIENT). In the future there will be other vfio devices that can be migrated, so we don't want to rely on this hardcoded block. QEMU 6.0+ will anyway inform us of any devices that will block migration (as a part of qemuDomainGetMigrationBlockers()), so we only need to do the hardcoded check in the case of old QEMU that can't provide that information. Signed-off-by: Laine Stump --- src/qemu/qemu_migration.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 6fc5791f61..4ad5b7af39 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1488,6 +1488,14 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, _("cannot migrate domain: %s"), reasons); return false; } +} else { +/* checks here are for anything that doesn't need to be + * checked by libvirt if running QEMU that can be queried + * about migration blockers. + */ + +if (!qemuMigrationSrcIsAllowedHostdev(vm->def)) +return false; } if (remote) { @@ -1514,9 +1522,6 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, return false; } -if (!qemuMigrationSrcIsAllowedHostdev(vm->def)) -return false; - if (vm->def->cpu) { /* QEMU blocks migration and save with invariant TSC enabled * unless TSC frequency is explicitly set. -- 2.35.3
Re: [libvirt PATCH v4 2/4] qemu_monitor: add support for get qemu migration blockers
On 7/20/22 12:05 PM, Eugenio Pérez wrote: since qemu 6.0, if migration is blocked for some reason, 'query-migrate' will return an array of error strings describing the migration blockers. This can be used to check whether there are any devices blocking migration, etc. Enable qemu monitor to send this query. This will allow qemuMigrationSrcIsAllowed to dynamically ask for migration blockers, reducing duplication. I reworded this commit log a bit - see the branch I linked in my response to the cover letter and let me know if you approve. Signed-off-by: Eugenio Pérez Reviewed-by: Jiri Denemark --- v4: * Separate return type into its own line v3: * Squash some patches * Return ok in qemuMonitorJSONGetMigrationBlockers is there are no blockers. * Move note to function doc. --- src/qemu/qemu_monitor.c | 11 + src/qemu/qemu_monitor.h | 4 src/qemu/qemu_monitor_json.c | 44 src/qemu/qemu_monitor_json.h | 3 +++ 4 files changed, 62 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 109107eaae..e0939beecd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4486,3 +4486,14 @@ qemuMonitorMigrateRecover(qemuMonitor *mon, return qemuMonitorJSONMigrateRecover(mon, uri); } + +int +qemuMonitorGetMigrationBlockers(qemuMonitor *mon, +char ***blockers) We try to keep 2 blank lines between each function. I added in the extra blank lines before and after this function. +{ +VIR_DEBUG("blockers=%p", blockers); + +QEMU_CHECK_MONITOR(mon); + +return qemuMonitorJSONGetMigrationBlockers(mon, blockers); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cc1a0bc8c9..b82f198285 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1543,3 +1543,7 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorMigrateRecover(qemuMonitor *mon, const char *uri); + +int +qemuMonitorGetMigrationBlockers(qemuMonitor *mon, +char ***blockers); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 5e4a86e5ad..6b26dfcb54 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3338,6 +3338,50 @@ int qemuMonitorJSONMigrate(qemuMonitor *mon, return 0; } +/* + * Get the exposed migration blockers. + * + * This function assume qemu has the capability of request them. + * + * It returns a NULL terminated array on blockers if there are any, or it set + * it to NULL otherwise. + */ +int +qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon, +char ***blockers) +{ +g_autoptr(virJSONValue) cmd = NULL; +g_autoptr(virJSONValue) reply = NULL; +virJSONValue *data; +virJSONValue *jblockers; +size_t i; + +*blockers = NULL; +if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate", NULL))) +return -1; + +if (qemuMonitorJSONCommand(mon, cmd, ) < 0) +return -1; + +if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) +return -1; + +data = virJSONValueObjectGetObject(reply, "return"); + +if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons"))) +return 0; + +*blockers = g_new0(char *, virJSONValueArraySize(jblockers) + 1); +for (i = 0; i < virJSONValueArraySize(jblockers); i++) { +virJSONValue *jblocker = virJSONValueArrayGet(jblockers, i); +const char *blocker = virJSONValueGetString(jblocker); + +(*blockers)[i] = g_strdup(blocker); +} + +return 0; +} + int qemuMonitorJSONMigrateCancel(qemuMonitor *mon) { g_autoptr(virJSONValue) cmd = qemuMonitorJSONMakeCommand("migrate_cancel", NULL); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 2759566892..e4c65e250e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -184,6 +184,9 @@ qemuMonitorJSONMigrate(qemuMonitor *mon, unsigned int flags, const char *uri); int +qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon, +char ***blockers); +int qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon, bool *spice_migrated);
Re: [libvirt PATCH v4 4/4] qemu_migration: Do not forbid vDPA devices if can query blockers
On 7/20/22 12:05 PM, Eugenio Pérez wrote: vDPA devices will be migratable soon. Since they are not migratable before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking for migration blockers, let it hardcoded in that case. Otherwise, ask qemu about the explicit blocker. I reworded the commit log message, but otherwise this is all fine. Signed-off-by: Eugenio Pérez Reviewed-by: Jiri Denemark --- v3: Fix indentation --- src/qemu/qemu_migration.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2e3044289a..b554027da2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, int pauseReason; size_t i; int r; +bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_MIGRATION_BLOCKED_REASONS); /* Ask qemu if it have a migration blocker */ -if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) { +if (blockedReasonsCap) { g_auto(GStrv) blockers = NULL; r = qemuDomainGetMigrationBlockers(driver, vm, ); if (r != 0) @@ -1576,7 +1578,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, virDomainNetDef *net = vm->def->nets[i]; qemuSlirp *slirp; -if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) { +if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("vDPA devices cannot be migrated")); return false;
Re: [libvirt PATCH v4 3/4] qemu_migration: get migration blockers before hardcoded checks
On 7/20/22 12:05 PM, Eugenio Pérez wrote: since qemu 6.0, if migration is blocked for some reason, 'query-migrate' will return an array of error strings describing the migration blockers. This can be used to check whether there are any devices blocking migration, etc. Enable qemuMigrationSrcIsAllowed to query it. I reworded the commit log message a bit. Rather than repeating it here, I'll just point you to the branch I pushed to gitlab (link in my response to the cover letter). Signed-off-by: Eugenio Pérez Reviewed-by: Jiri Denemark --- v4: * Do not override qemuDomainGetMigrationBlockers error calling again virReportError. * Replace ", " with "; " in blockers separators. v3: * Report message with a colon. * Report all blockers instead of only the first. --- src/qemu/qemu_migration.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b12cb518ee..2e3044289a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def) return true; } +static int +qemuDomainGetMigrationBlockers(virQEMUDriver *driver, + virDomainObj *vm, + char ***blockers) +{ +qemuDomainObjPrivate *priv = vm->privateData; +int rc; + +qemuDomainObjEnterMonitor(driver, vm); +rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers); +qemuDomainObjExitMonitor(vm); + +return rc; +} Note that we've been trying to keep 2 blank lines between each function, but here you've added a new function in between those 2 blank lines, so there's only a single blank before and after. I added in the extra blanks. /** * qemuMigrationSrcIsAllowed: @@ -1439,6 +1453,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, int nsnapshots; int pauseReason; size_t i; +int r; + +/* Ask qemu if it have a migration blocker */ "has a migration blocker" +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) { +g_auto(GStrv) blockers = NULL; +r = qemuDomainGetMigrationBlockers(driver, vm, ); +if (r != 0) +return false; in libvirt we usually return -1 on failure, and check with "if (r < 0)". And since that is the only use of "r", we prefer to not have the extra stack variable cluttering things up, so: if (qemuDomainGetMigrationBlockers(driver, vm, ) < 0) return false; + +if (blockers && blockers[0]) { +g_autofree char *reasons = g_strjoinv("; ", blockers); +virReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain: %s"), reasons); +return false; +} +} /* perform these checks only when migrating to remote hosts */ if (remote) {
Re: [libvirt PATCH v4 0/4] Ask qemu about migration blockers
This all looks good except a couple small nits that I've noted in separate messages for the individual patches. I've already made these minor changes locally and pushed them here: https://gitlab.com/lainestump/libvirt/-/commits/REVIEW If it all looks okay to you, I'll push it as soon as I get up in the AM. If you don't like the changes I've made to the commit log messages, feel free to revise and re-send them :-) This is a pretty good turn around time for your first patch in libvirt. Thanks for the contribution! On 7/20/22 12:05 PM, Eugenio Pérez wrote: There are some hardcoded migration blockers in libvirt, like having a net vhost-vdpa device. While it's true that they cannot be migrated at the moment, there are plans to be able to migrate them soon They'll put a migration blockers in the cases where you cannot migrate them. Ask qemu about then before rejecting the migration. In case the question is not possible, assume they're not migratable. v4: * Do not override qemuDomainGetMigrationBlockers error calling again virReportError. * Replace ", " with "; " in blockers separators. v3: * Return ok in qemuMonitorJSONGetMigrationBlockers is there are no blockers. * Fix indentation * Report all blockers instead of only the first. * Squash some patches * Move note to function doc. * s/QEMU_MIGRATION_BLOCKED_REASONS/QEMU_CAPS_MIGRATION_BLOCKED_REASONS/ v2: * Ask qemu if it has some pending blockers before try the migration Eugenio Pérez (3): qemu_monitor: add support for get qemu migration blockers qemu_migration: get migration blockers before hardcoded checks qemu_migration: Do not forbid vDPA devices if can query blockers Jonathon Jongsma (1): qemu: introduce capability QEMU_CAPS_MIGRATION_BLOCKED_REASONS src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_migration.c | 34 +- src/qemu/qemu_monitor.c | 11 + src/qemu/qemu_monitor.h | 4 ++ src/qemu/qemu_monitor_json.c | 44 +++ src/qemu/qemu_monitor_json.h | 3 ++ .../caps_6.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../caps_6.2.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_6.2.0.ppc64.xml | 1 + .../caps_6.2.0.x86_64.xml | 1 + .../caps_7.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 1 + .../caps_7.0.0.x86_64.xml | 1 + .../caps_7.1.0.x86_64.xml | 1 + 18 files changed, 109 insertions(+), 1 deletion(-)
Re: [libvirt PATCH] qemu_migration: Delete vDPA check
On 7/19/22 2:38 PM, Eugenio Perez Martin wrote: On Tue, Jul 19, 2022 at 6:43 PM Laine Stump wrote: On 7/19/22 12:01 PM, Laine Stump wrote: On 7/19/22 11:09 AM, Eugenio Perez Martin wrote: On Tue, Jul 19, 2022 at 4:02 PM Laine Stump wrote: On 7/18/22 11:15 AM, Jiri Denemark wrote: On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote: On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark wrote: Which in ideal case would mean only a QMP command (such as hotplugging a non-migratable device) is the only way to add migration blocker. If this is true, than we're safe as libvirt does not allow such commands between qemuMigrationSrcIsAllowed and migration start. Ok, that rules out a few bad use cases. I can do a fast lookup to check if blockers can be added without the knowledge of libvirt. That said, is there a reason for not implementing the correct solution right away as a separate patch? I was not sure if libvirt already had another way to check, for example, if the vhost device didn't have VHOST_F_LOG_ALL feature. I'm not aware of such check, but even if it exists, checking for migration blockers looks like the right way of doing things anyway. Actually that's been on my todo list for a long time - for any qemu that supports the QMP command that checks for migratability, we should be calling this command rather than checking against our own internal list (which is really just an "informed guess") of what can't be migrated. This way we'll always get the right answer (or at least what QEMU believes to be the right answer :-)). Fixing it this way will also mean that migration of VFIO devices will just "magically" start working once a migration-supporting driver is written for the device, and the correct vfio driver is bound to the device (this latter item is also on my list). So if you're up for making the patch to call the QMP command, I'd be happy to review it! Thanks! Actually I'd need some guidance first, I'm not very used to libvirt code. As I understand I should create a function in qemu_agent.h/c, a getter similar to qemuAgentGetFSInfo. How can I get a qemuAgent from qemuMigrationSrcIsAllowed? I only have a virQEMUDriver there. qemu_agent.c is only for functions that require calling to the QEMU guest agent, which is a process running inside the guest. You just need to run a simple QMP command. There are some good examples of this in qemu_monitor_json.c For now it should be enough to delete vdpa hardcoded negation, and then other parts of libvirt can delete other hardcoded checks, isn't it? There's just a single function that checks for migratability (qemuMigrationSrcIsAllowed()). In theory *everything* in that function should be deprecated by just calling qemu to ask. In practice there may be / probably are things that qemu doesn't count as "can't migrate" that really should be counted that way. Certainly the VDPA and hostdev checks should be removable immediately though (although of course this should still be checked before pushing!) What I would do is this: 1) a patch that adds code to the qemu_capabilities to set a flag if the desired field in the "query-migrate" QMP command would be filled in by this qemu binary. Just to permanently document live discussions from IRC: jjongsma pointed us to a patch he wrote a year ago (but never pushed upstream) that implements (1): https://gitlab.com/jjongsma/libvirt/-/commit/4003b7047058a17465083178d6c0a0f62c900c74 Thanks! 2) a patch that adds a function to qemu_monitor_json.c to call that command and return migratable/not. Thinking about this more, I guess a function that returns the full text of "blocked-reason" would be more useful (that way it could be easily logged). I'm actually returned all the array in the form of `char ***` 3) a patch that adds a call to that function to qemuMigrationSrcIsAllowed(). What I cannot find an example of is how to get the qemuMonitor from the virQEMUDriver that qemuMigrationSrcIsAllowed has as the parameter. Hopefully I'm not leading you down a false path, but it looks like the call chain of: qemuDomainChangeMemoryRequestedSize() qemuMonitorChangeMemoryRequestedSize() qemuMonitorJSONChangeMemoryRequestedSize() contains all the bits you need, including the toplevel function that calls qemuDomainObj(Enter|Exit)Monitor() and grabs the qemuMonitor object from the domainObj's privateData before calling down to a wrapper function in qemu_monitor.c (that I think was originally there because there could be either an old-style shell-like command or a QMP command to do the same thing), and then from there down to the QMP/JSON function in qemu_monitor_json.c that actually calls the monitor. 4) additional patches that remove specific hardcoded checks *only if the new field in query-migrate is available (as indicated by the new capabilities flag) and returned a definite yes/no (othe
Re: [libvirt PATCH] qemu_migration: Delete vDPA check
On 7/19/22 12:01 PM, Laine Stump wrote: On 7/19/22 11:09 AM, Eugenio Perez Martin wrote: On Tue, Jul 19, 2022 at 4:02 PM Laine Stump wrote: On 7/18/22 11:15 AM, Jiri Denemark wrote: On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote: On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark wrote: Which in ideal case would mean only a QMP command (such as hotplugging a non-migratable device) is the only way to add migration blocker. If this is true, than we're safe as libvirt does not allow such commands between qemuMigrationSrcIsAllowed and migration start. Ok, that rules out a few bad use cases. I can do a fast lookup to check if blockers can be added without the knowledge of libvirt. That said, is there a reason for not implementing the correct solution right away as a separate patch? I was not sure if libvirt already had another way to check, for example, if the vhost device didn't have VHOST_F_LOG_ALL feature. I'm not aware of such check, but even if it exists, checking for migration blockers looks like the right way of doing things anyway. Actually that's been on my todo list for a long time - for any qemu that supports the QMP command that checks for migratability, we should be calling this command rather than checking against our own internal list (which is really just an "informed guess") of what can't be migrated. This way we'll always get the right answer (or at least what QEMU believes to be the right answer :-)). Fixing it this way will also mean that migration of VFIO devices will just "magically" start working once a migration-supporting driver is written for the device, and the correct vfio driver is bound to the device (this latter item is also on my list). So if you're up for making the patch to call the QMP command, I'd be happy to review it! Thanks! Actually I'd need some guidance first, I'm not very used to libvirt code. As I understand I should create a function in qemu_agent.h/c, a getter similar to qemuAgentGetFSInfo. How can I get a qemuAgent from qemuMigrationSrcIsAllowed? I only have a virQEMUDriver there. qemu_agent.c is only for functions that require calling to the QEMU guest agent, which is a process running inside the guest. You just need to run a simple QMP command. There are some good examples of this in qemu_monitor_json.c For now it should be enough to delete vdpa hardcoded negation, and then other parts of libvirt can delete other hardcoded checks, isn't it? There's just a single function that checks for migratability (qemuMigrationSrcIsAllowed()). In theory *everything* in that function should be deprecated by just calling qemu to ask. In practice there may be / probably are things that qemu doesn't count as "can't migrate" that really should be counted that way. Certainly the VDPA and hostdev checks should be removable immediately though (although of course this should still be checked before pushing!) What I would do is this: 1) a patch that adds code to the qemu_capabilities to set a flag if the desired field in the "query-migrate" QMP command would be filled in by this qemu binary. Just to permanently document live discussions from IRC: jjongsma pointed us to a patch he wrote a year ago (but never pushed upstream) that implements (1): https://gitlab.com/jjongsma/libvirt/-/commit/4003b7047058a17465083178d6c0a0f62c900c74 2) a patch that adds a function to qemu_monitor_json.c to call that command and return migratable/not. Thinking about this more, I guess a function that returns the full text of "blocked-reason" would be more useful (that way it could be easily logged). 3) a patch that adds a call to that function to qemuMigrationSrcIsAllowed(). 4) additional patches that remove specific hardcoded checks *only if the new field in query-migrate is available (as indicated by the new capabilities flag) and returned a definite yes/no (otherwise the checks still need to be done, to account for older qemu binaries that don't have the qmp command). I had thought there was a bugzilla somewhere that was tracking this for libvirt, but I can't find it.
Re: [libvirt PATCH] qemu_migration: Delete vDPA check
On 7/19/22 11:09 AM, Eugenio Perez Martin wrote: On Tue, Jul 19, 2022 at 4:02 PM Laine Stump wrote: On 7/18/22 11:15 AM, Jiri Denemark wrote: On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote: On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark wrote: Which in ideal case would mean only a QMP command (such as hotplugging a non-migratable device) is the only way to add migration blocker. If this is true, than we're safe as libvirt does not allow such commands between qemuMigrationSrcIsAllowed and migration start. Ok, that rules out a few bad use cases. I can do a fast lookup to check if blockers can be added without the knowledge of libvirt. That said, is there a reason for not implementing the correct solution right away as a separate patch? I was not sure if libvirt already had another way to check, for example, if the vhost device didn't have VHOST_F_LOG_ALL feature. I'm not aware of such check, but even if it exists, checking for migration blockers looks like the right way of doing things anyway. Actually that's been on my todo list for a long time - for any qemu that supports the QMP command that checks for migratability, we should be calling this command rather than checking against our own internal list (which is really just an "informed guess") of what can't be migrated. This way we'll always get the right answer (or at least what QEMU believes to be the right answer :-)). Fixing it this way will also mean that migration of VFIO devices will just "magically" start working once a migration-supporting driver is written for the device, and the correct vfio driver is bound to the device (this latter item is also on my list). So if you're up for making the patch to call the QMP command, I'd be happy to review it! Thanks! Actually I'd need some guidance first, I'm not very used to libvirt code. As I understand I should create a function in qemu_agent.h/c, a getter similar to qemuAgentGetFSInfo. How can I get a qemuAgent from qemuMigrationSrcIsAllowed? I only have a virQEMUDriver there. qemu_agent.c is only for functions that require calling to the QEMU guest agent, which is a process running inside the guest. You just need to run a simple QMP command. There are some good examples of this in qemu_monitor_json.c For now it should be enough to delete vdpa hardcoded negation, and then other parts of libvirt can delete other hardcoded checks, isn't it? There's just a single function that checks for migratability (qemuMigrationSrcIsAllowed()). In theory *everything* in that function should be deprecated by just calling qemu to ask. In practice there may be / probably are things that qemu doesn't count as "can't migrate" that really should be counted that way. Certainly the VDPA and hostdev checks should be removable immediately though (although of course this should still be checked before pushing!) What I would do is this: 1) a patch that adds code to the qemu_capabilities to set a flag if the desired field in the "query-migrate" QMP command would be filled in by this qemu binary. 2) a patch that adds a function to qemu_monitor_json.c to call that command and return migratable/not. 3) a patch that adds a call to that function to qemuMigrationSrcIsAllowed(). 4) additional patches that remove specific hardcoded checks *only if the new field in query-migrate is available (as indicated by the new capabilities flag) and returned a definite yes/no (otherwise the checks still need to be done, to account for older qemu binaries that don't have the qmp command). I had thought there was a bugzilla somewhere that was tracking this for libvirt, but I can't find it.
Re: [libvirt PATCH] qemu_migration: Delete vDPA check
On 7/18/22 11:15 AM, Jiri Denemark wrote: On Mon, Jul 18, 2022 at 10:40:56 +0200, Eugenio Perez Martin wrote: On Mon, Jul 18, 2022 at 10:25 AM Jiri Denemark wrote: Which in ideal case would mean only a QMP command (such as hotplugging a non-migratable device) is the only way to add migration blocker. If this is true, than we're safe as libvirt does not allow such commands between qemuMigrationSrcIsAllowed and migration start. Ok, that rules out a few bad use cases. I can do a fast lookup to check if blockers can be added without the knowledge of libvirt. That said, is there a reason for not implementing the correct solution right away as a separate patch? I was not sure if libvirt already had another way to check, for example, if the vhost device didn't have VHOST_F_LOG_ALL feature. I'm not aware of such check, but even if it exists, checking for migration blockers looks like the right way of doing things anyway. Actually that's been on my todo list for a long time - for any qemu that supports the QMP command that checks for migratability, we should be calling this command rather than checking against our own internal list (which is really just an "informed guess") of what can't be migrated. This way we'll always get the right answer (or at least what QEMU believes to be the right answer :-)). Fixing it this way will also mean that migration of VFIO devices will just "magically" start working once a migration-supporting driver is written for the device, and the correct vfio driver is bound to the device (this latter item is also on my list). So if you're up for making the patch to call the QMP command, I'd be happy to review it!
Re: [PATCH 1/2] virNetDevSaveNetConfig: Pass mode to virFileWriteStr()
On 6/13/22 9:18 AM, Michal Privoznik wrote: For some types of SRIOV interfaces we create a temporary file where the state of the interface is saved before we start modifying it. The file is used then to restore the original configuration when the interface is no longer associated with any guest. For writing the file virFileWriteStr() is used. However, it's given wrong argument: the last argument is supposed to be mode to create the file with but virNetDevSaveNetConfig() passes open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be writable and readable by root only (0600). Therefore, pass that mode instead of gibberish. Wow. This may be in competition for the longest living "how did this ever work?" bug in the code :-/ Since my name was on the git blame for the most recent change to this line, I had to figure out if it was really me that had misunderstood/misused virFileWriteStr() so grievously (wouldn't be the first or the last time). What I found was that this code had been moved around by multiple different people (including me) since originally being included in new code all the way back in commit cbd8227ee in June 2011. Anyway, Peter has already acked it, but still Reviewed-by: Laine Stump
Re: [PATCH] qemu_hotplug: Deny changing @rss and @rss_hash_report attributes of virtio vNICs
On 5/23/22 11:22 AM, Michal Privoznik wrote: We have virDomainUpdateDeviceFlags() API that allows changing of some attributes of a device whilst domain is still running (e.g. setting different QoS, link state change on vNICs). But only very limited set of attributes can be changed and we have to check whether user isn't trying to sneak in a change that's not allowed. Well, in case of a virtio vNIC we forgot to check for @rss and @rss_hash_report attributes of . Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2082540 Signed-off-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Laine Stump (too bad there's not an elegant automated way of pointing out when a new field is added that can't be updated at runtime) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 24df66cc9f..f795568299 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3662,7 +3662,9 @@ qemuDomainChangeNet(virQEMUDriver *driver, olddev->driver.virtio.guest.tso4 != newdev->driver.virtio.guest.tso4 || olddev->driver.virtio.guest.tso6 != newdev->driver.virtio.guest.tso6 || olddev->driver.virtio.guest.ecn != newdev->driver.virtio.guest.ecn || - olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo)) { + olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo || + olddev->driver.virtio.rss != newdev->driver.virtio.rss || + olddev->driver.virtio.rss_hash_report != newdev->driver.virtio.rss_hash_report)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify virtio network device driver attributes")); goto cleanup;
Re: [PATCH 0/4] network: firewalld: fix routed network
On 5/12/22 2:00 PM, Daniel P. Berrangé wrote: On Wed, May 11, 2022 at 11:41:51AM -0400, Eric Garver wrote: This series fixes routed networks when a newer firewalld (>= 1.0.0) is present [1]. Firewalld 1.0.0 included a change that disallows implicit forwarding between zones [2]. libvirt was relying on this behavior to allow routed networks to function. New firewalld policies are added. This is done to use common rules between NAT and routed networks. Policies have been supported since firewalld 0.9.0. For those following along, there's a helpful description of policies here, specifically explaining how its useful to the libvirt scenario: https://firewalld.org/2020/09/policy-objects-introduction ...and for some further context that is probably only documented in the discussions that we had with Eric and some other people back in 2018 or so: Once firewalld switches to its native-nftables backend, all of its own rules go into a separate nftables table, while libvirt's rules go into the iptables-compatibility table called "filter". In order for a packet to be accepted and forwarded, it must be accepted by *all* tables. (with iptables, both firewalld and libvirt use the "filter" table, and it is enough for the rules of one or the other to accept a packet). At the time libvirt added support for the firewalld nftables backend, there was no way to explicitly specify "allow forwarded traffic" in a zone, and if the zone was "default REJECT" then all forwarded traffic would be rejected. In order for our traffic to be accepted, we had to make the "libvirt zone" (which is itself a part of *firewalld's* rules, not libvirt's rules!) "default ACCEPT", and then use an at-the-time new feature of firewalld that allowed us to specify higher priority ACCEPT rules for the traffic we wanted accepted, then a lower priority "REJECT ALL" rule (which would reject all traffic on the *INPUT* chain, but not on the FORWARD chain), and then the "default ACCEPT" rule would implicitly add rules that accepted any forwarded traffic. Yes, in restrospect it sounds fragile. And at the time it sounded fragile as well. Unfortunately it was the only way to make things work. In the ensuing years, firewalld has added explicit support for accepting/rejecting traffic on the FORWARD and OUTPUT chains, but as a part of this, that implicit "default ACCEPT" of forwarded traffic has been removed. And *that* is what necessitates Eric's new zone/policy files! Whew!
Re: [PATCH 1/4] network: firewalld: convert to policies
On 5/12/22 12:53 PM, Eric Garver wrote: On Wed, May 11, 2022 at 05:15:25PM +0100, Daniel P. Berrangé wrote: On Wed, May 11, 2022 at 11:41:52AM -0400, Eric Garver wrote: Convert the existing behavior into policies. Has this split of .zone vs .policy been something firewalld always supported, or is it a "new" feature for some value of "new" ? Policies are new in firewalld-0.9.0. https://firewalld.org/2020/09/policy-objects-introduction Policies supplement zones. They do not split or replace them. Essentially wonder if this has any historical back compat implications for libvirt, given the platforms we target (2 most recent major releases of all distros, so RHEL >= 8 and equiv). The original zone definition requires firewalld >= 0.7.0. So the versions we need to worry about with this change are 0.7.z through 0.8.z. At least these distributions (probably non-exhaustive list) have a firewalld version in that range: Ubuntu: - focal (20.04 LTS) has 0.8.2 - this is 3 major releases ago, but 2 LTS releases ago Okay, so until we drop support for Ubuntu 20.04 I guess we need to maintain the existing "standalone" libvirt zone (i.e. doesn't use a policy file, and assumes the implied passage of inbound traffic due to the "default accept" behavior of the zone). We still have a (very outdated and now pointless! note to self: send a patch to remove it!) firewalld version check for a "firewalld < 0.6.0 with nftables backend", so we can easily add a check for firewalld < 0.9.0, use the standalone single libvirt zone in that case, and use separate libvirt-(nat|routed|isolated?) zones when >= 0.9.0. When Ubuntu 20.04 drops from our supported distros, then we can just remove that check along with the standalone "libvirt" zone. The only quirk here is that anyone who has copied the existing libvirt zone into /etc/firewalld/zones will be surprised when their modified zone is no longer used (as a result of us switching the default zone from "libvirt" to "libvirt-routed" or "libvirt-nat"). If only I'd had the foresight to install separately-named zones from the start :-/ (of course that would have *it's own* set of problems in the case of a zonefile update like this, so I guess I shouldn't beat myself up too much). Oh, but wait! I just realized - won't an older firewalld complain about a zonefile that references a policy even if no interfaces ever use that zone? If so, then the packaging for Ubuntu 20.04 may need to only install the old zone file, which would render all the version-check stuff outlined above to be pointless. An associated problem is that the files that create the ubuntu packages are not a part of libvirt's git tree, so making this all work would require updating the out-of-tree build config for a two year old distro (not that they would ever release an official new libvirt version for it, but if we say we support it, that implies that somebody somewhere might want to build an updated libvirt for it). (sigh. Why is nothing ever just simple and straightforward?...) -- The below distributions should be "good to go": RHEL/Fedora: - RHEL-8 and RHEL-9 have >= 0.9.0. - f34 and later have >= 0.9.0. Debian: - stable (11, bullseye) has 0.9.2. - oldstable (10, buster) has 0.6.3 - defaults to iptables backend [1] so even the original zone is not necessary Ubuntu: - jammy (22.04 LTS) has 1.1.1 - impish (21.10) has 0.9.3 repeat sigh. I will never not be annoyed by childish sounding version names :-/ version.numbers++ (/me hangs head in embarrassment at the Fedora "Beefy Miracle" debacle) SUSE: - 15 SP4 has 0.9.3 - 12 SP5 has 0.4.3.3 (too old to care) Note: I didn't investigate rolling release distributions, e.g. Arch, Gentoo I think by definition those *should* just have the latest version (or close) of every package, shouldn't they? [1]: https://salsa.debian.org/utopia-team/firewalld/-/blob/17fc3126d6eab159f6c703c7e100345fe3450f97/debian/patches/Switch-firewall-backend-from-nftables-back-to-iptables.patch This commit has no functional changes. Signed-off-by: Eric Garver --- src/network/libvirt-nat-out.policy | 12 src/network/libvirt-to-host.policy | 20 src/network/libvirt.zone | 23 +-- src/network/meson.build| 10 ++ 4 files changed, 47 insertions(+), 18 deletions(-) create mode 100644 src/network/libvirt-nat-out.policy create mode 100644 src/network/libvirt-to-host.policy diff --git a/src/network/libvirt-nat-out.policy b/src/network/libvirt-nat-out.policy new file mode 100644 index ..7d1cf6dfb4c4 --- /dev/null +++ b/src/network/libvirt-nat-out.policy @@ -0,0 +1,12 @@ + + + libvirt-nat-out + + +This policy is used to allow NAT virtual machine traffic to the +rest of the network. + + + + + diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy new
Re: [libvirt PATCH 0/2] nwfilter cleanups for legacy platforms
On 3/8/22 12:52 PM, Daniel P. Berrangé wrote: We have a couple of compatibility hacks to cope with changes in iptables userspace and kernel. These were very long ago so not relevant to our current build platforms. Removing them makes the code clearer. The tests have churn because we were never properly testing this aspect in the past Daniel P. Berrangé (2): nwfilter: drop support for legacy iptables match syntax nwfilter: drop support for legacy iptables conntrack direction Reviewed-by: Laine Stump for both.
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
Aha! the domain of qemu-de...@nongnu.org was incorrect in the original send (it was "nognu.org"), so none of this thread was making it to that list. I've corrected it in this message, but interested parties from qemu-devel will need to look on the libvir-list archives for the actual patch mails: https://listman.redhat.com/archives/libvir-list/2022-March/229089.html Anyone else who responds to any of the mail on this thread should fix the qemu-devel address accordingly. On 3/8/22 10:33 AM, Laine Stump wrote: On 3/8/22 1:39 AM, Ani Sinha wrote: Changelog: v2 - rebased the patch series to latest master. I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. My memory isn't completely clear, but I think there was also the issue that the option claims to enable ACPI hotplug when set to on, but instead what it actually does (in the Q35 case at least) is to enable native PCI hotplug when set to off (without actually disabling ACPI hotplug) and disable native PCI hotplug when set to on, or something like that. This ends up leaving it up to the guest OS to decide which type of hotplug to use, meaning its decision could override what's in the libvirt config, thus confusing everyone. Again, I probably have the details mixed up, but it was something like this. I asked mst about this this morning, and he suggested something that you've already done - Cc'ing the series to qemu-devel and the relevant maintainers so we can have a discussion with all involved parties about their opinions on whether we really should expose this existing option in libvirt, or if we should instead have two new options that are more orthogonal about enabling/disabling the two types of hotplug, so that libvirt config can more accurately represent what is being presented to the guest rather than a "best guess" of what we think the guest is going to do with what is presented. (Michael did also say that, with the current flurry of bug reports for the QEMU rc's, this discusion may not happen until closer to release when the bug reports die down. I know this doesn't mesh with your desire to "push now to allow for testing" (which in general would be a good thing if we were certain that we wanted the option like this and were just expecting some minor bugs that could be fixed), but my opinion is that 1) it's possible for anyone interested to test the functionality using , and 2) we should avoid turning libvirt git into a revolving door of experiments. The only practical difference between using and having a dedicated option is that the use of causes the domain to be tainted, and the XML is a bit more complicated. But since the people we're talking about here will already have built their own libvirt binaries, the tainted status of any guests is irrelevant and the extra complexity of using is probably trivial to them :-). Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of h
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On 3/8/22 1:39 AM, Ani Sinha wrote: Changelog: v2 - rebased the patch series to latest master. I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. My memory isn't completely clear, but I think there was also the issue that the option claims to enable ACPI hotplug when set to on, but instead what it actually does (in the Q35 case at least) is to enable native PCI hotplug when set to off (without actually disabling ACPI hotplug) and disable native PCI hotplug when set to on, or something like that. This ends up leaving it up to the guest OS to decide which type of hotplug to use, meaning its decision could override what's in the libvirt config, thus confusing everyone. Again, I probably have the details mixed up, but it was something like this. I asked mst about this this morning, and he suggested something that you've already done - Cc'ing the series to qemu-devel and the relevant maintainers so we can have a discussion with all involved parties about their opinions on whether we really should expose this existing option in libvirt, or if we should instead have two new options that are more orthogonal about enabling/disabling the two types of hotplug, so that libvirt config can more accurately represent what is being presented to the guest rather than a "best guess" of what we think the guest is going to do with what is presented. (Michael did also say that, with the current flurry of bug reports for the QEMU rc's, this discusion may not happen until closer to release when the bug reports die down. I know this doesn't mesh with your desire to "push now to allow for testing" (which in general would be a good thing if we were certain that we wanted the option like this and were just expecting some minor bugs that could be fixed), but my opinion is that 1) it's possible for anyone interested to test the functionality using , and 2) we should avoid turning libvirt git into a revolving door of experiments. The only practical difference between using and having a dedicated option is that the use of causes the domain to be tainted, and the XML is a bit more complicated. But since the people we're talking about here will already have built their own libvirt binaries, the tainted status of any guests is irrelevant and the extra complexity of using is probably trivial to them :-). Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of hot-plugging switches with PCIe Native hotplug since ACPI PCI Hot-plug only works with cold-plugged bridges. If the user wants to explicitely use this feature, they can disable ACPI PCI Hot-plug with: --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug instead of PCIe Native. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2]
Re: [PATCH] libxl: Turn on user aliases
On 3/2/22 3:29 AM, Peter Krempa wrote: On Tue, Mar 01, 2022 at 09:40:30 -0700, Jim Fehlig wrote: On 2/17/22 04:56, Michal Privoznik wrote: When I implemented user aliases I've invented this virDomainDefFeatures flag so that individual drivers can signal support for user provided aliases. The reasoning was that a device alias might be part of guest ABI, or used in a different way then in QEMU. Well, neither applies to the libxl driver, so it's safe to allow user aliases there. I suppose it is safe, but does it make sense since aliases are not used by the driver in any way, and not supported by libxl? In that case at least the libvirt side user-visible aspect of it does make sense. Users can pick a stable alias for their device if they care about that for any reason. ...for example, in the qemu driver, the alias can be used as an index to match when unplugging or updating a device; in the case of an , in the past you could match the device to unplug/update by looking at the MAC address or PCI address, but the MAC address isn't necessarily unique, and PCI address is usually determined by libvirt, so the management application may not know it. But the management application can just explicitly provide an alias in the XML when the domain is defined, and then later use that as the key when unplugging/updating. Matching on alias when searching for an was added for use by the QEMU driver back in commit 114e3b4232, but fortunately this search is done by virDomainNetFindIdx(), which is also used by the libxl interface update and unplug functions, so libxl will get this functionality for free once Michal's patch is pushed. (I haven't looked at whether or not other types of devices are using alias in this manner either in the qemu or libxl drivers, but I assume at least some of them are) In the qemu impl we decided to propagate it to the hypervisor, but that should be considered an implementation detail as the IDs of objects (mostly) don't have guest visible meaning.
Re: [libvirt PATCH 5/5] nwfilter: make some gentech driver methods static
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: The virNWFilterTechDriverForName & virNWFilterUpdateInstantiateFilter methods are only used within the same source file, so don't need to be exported. Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump (again, I didn't even look to the source to verify; the fact that it still compiles proves that you're right. Hooray for machines doing (part of) our work for us!)
Re: [libvirt PATCH 4/5] nwfilter: remove decl of virNWFilterCreateVarHashmap
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: This method doesn't exist since commit d1a7c08eb145d8b9d9c4a268f43b1590049a Author: Daniel P. Berrangé Date: Thu Apr 26 12:26:51 2018 +0100 nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr Signed-off-by: Daniel P. Berrangé The proof is in the successful compile without it :-) Reviewed-by: Laine Stump
Re: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: Now that the virNWFilterBinding APIs are using the nwfilter update lock directly, there is no need for the virt drivers to do it themselves. I'm always nervous when the ordering of locks is changed, and that's what is happening with the combination of this patch and the previous patch. Before it was always the NWFilterLock that was acquired first, and then the domain object is retrieved (which involves getting the domain-list lock while getting a ref to the domain object). But since holding of the domain-list lock is localized to that short period (and certainly never across a call to the NWFilter binding API) I'm fairly certain this (along with the previous patch) is safe from creating any new deadlocks. Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump --- src/lxc/lxc_driver.c | 6 -- src/qemu/qemu_driver.c| 18 -- src/qemu/qemu_migration.c | 3 --- src/qemu/qemu_process.c | 4 4 files changed, 31 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257ae..4185a61267 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); -virNWFilterReadLockFilterUpdates(); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, cleanup: virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; -virNWFilterReadLockFilterUpdates(); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, cleanup: virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); -virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b74b0375a7..e4e1cd7bdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; -virNWFilterReadLockFilterUpdates(); - if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); -virNWFilterUnlockFilterUpdates(); return dom; } @@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; -virNWFilterReadLockFilterUpdates(); - fd = qemuSaveImageOpen(driver, NULL, path, , , (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, , false, false); @@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (vm && ret < 0) qemuDomainRemoveInactiveJob(driver, vm); virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_FORCE_BOOT | VIR_DOMAIN_START_RESET_NVRAM, -1); -virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) cleanup: virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, virDomainObj *vm = NULL; int ret = -1; -virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); -virNWFilterReadLockFilterUpdates(); - cfg = virQEMUDriverGetConfig(driver); if (!(vm = qemuDomainObjFromDomai
Re: [libvirt PATCH 2/5] nwfilter: hold filter update lock when creating/deleting bindings
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: The nwfilter update lock is historically acquired by the virt drivers in order to achieve serialization between nwfilter define/undefine, and instantiation/teardown of filters. When running in the modular daemons, however, the mutex that the virt drivers are locking is in a completely different process from the mutex that the nwfilter driver is locking. Serialization is lost and thus call from the virt driver to virNWFilterBindingCreateXML can deadlock with a concurrent call to the virNWFilterDefineXML method. The solution is surprisingly easy, the update lock simply needs acquiring in the virNWFilterBindingCreateXML method and virNWFilterBindingUndefine method instead of in the virt drivers. The only semantic difference here is that when a virtual machine has multiple NICs, the instantiation and teardown of filters is no longer serialized for the whole VM, but rather for each NIC. This should not be a problem since the virt drivers already need to cope with tearing down a partially created VM where only some of the NICs are setup. Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump (I considered this patch together with 3/5 - see my comment there of why I think the re-ordering of the locking is safe) --- src/nwfilter/nwfilter_driver.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 08f138dd79..3ce8fce7f9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -760,11 +760,14 @@ nwfilterBindingCreateXML(virConnectPtr conn, if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) goto cleanup; +virNWFilterReadLockFilterUpdates(); if (virNWFilterInstantiateFilter(driver, def) < 0) { +virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjListRemove(driver->bindings, obj); g_clear_pointer(, virObjectUnref); goto cleanup; } +virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjSave(obj, driver->bindingDir); cleanup: @@ -801,7 +804,9 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding) if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) goto cleanup; +virNWFilterReadLockFilterUpdates(); virNWFilterTeardownFilter(def); +virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjDelete(obj, driver->bindingDir); virNWFilterBindingObjListRemove(driver->bindings, obj);
Re: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: The virNWFilterLockIface method is only called from one place, the learnIPAddressThread method. nwfilter_gentech_driver.c:virNWFilterDoInstantiate() and _virNWFilterTeardownFilter() also call virNWFilterLockIface. And there is this call chain from learnIPAddressThread() that ends up at one or the other of those: learnIPAddressThread() virNWFilterInstantiateFilterLate() virNWFilterInstantiateFilterUpdate() virNWFilterDoInstantiate() < _virNWFilterTeardownFilter() < But of course just prior to calling virNWFilterINstantiateFilterLate(), learnIPAddressThread() calls: virNWFilterUnlockIface(req->binding->portdevname); which matches the previous virNWFilterLockIface(), so there shouldn't be a problem for the learnIPAddressThread at least. I'm not sure about the threads of other calls to virNWFilterDoInstantiate/TeardownFilter though, haven't tried to decipher them yet. This is the entry point for the learning thread on the interface in question. As such there is never any recursive locking on this mutex from the same thread. This appears to have been the case since the code was first introduced. Thus a plain mutex is sufficient. Signed-off-by: Daniel P. Berrangé --- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index fb552bd1e6..4840b0539f 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -151,7 +151,7 @@ virNWFilterLockIface(const char *ifname) if (!ifaceLock) { ifaceLock = g_new0(virNWFilterIfaceLock, 1); -if (virMutexInitRecursive(>lock) < 0) { +if (virMutexInit(>lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); g_free(ifaceLock);
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/3/22 10:52 AM, Michal Prívozník wrote: On 2/3/22 15:45, Laine Stump wrote: On 2/2/22 1:04 PM, Michal Prívozník wrote: Laine, any thoughts? I somehow thought this had already been pushed, so I was surprised when it showed up again :-/ I think the only issue I had before was that I thought the new unit tests were more testing the test setup than the actual code, but Dan convinced me otherwise. So I'm fine with this change. Cool, pushed now. A newly discovered (but pre-existing, so non-consequential to these patches) problem associated with vlans is that we don't differentiate between "set vlan 0" and "don't set any vlan", which I hadn't even considered before, until this BZ came up: https://bugzilla.redhat.com/2035726 (The reporter is trying to use to "unset" the vlan tag for an interface) Ah, but the bug report is for openvswitch port profile, so that's doubly unrelated :-) Yes and now. The original report was about openvswitch ports, but then the reporter also tried things with hostdev SRIOV devices and macvtap passthrough devices (with varying results). I'm sure that whatever is done to fix it will be deeply intertwined and complicated by the current topic (since the whole point of these patches is that smartnics want/need to just leave the entire vlan tag thing alone) :-) Additionally, Dmitrii's change might make it possible to easily/simply fix the case of updating vlan tag for existing macvtap passthrough and hostdev SRIOV VFs (since we can now set the vlan tag without doing anything else). So for once there is an unintended *good* side effect!
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/2/22 1:04 PM, Michal Prívozník wrote: Laine, any thoughts? I somehow thought this had already been pushed, so I was surprised when it showed up again :-/ I think the only issue I had before was that I thought the new unit tests were more testing the test setup than the actual code, but Dan convinced me otherwise. So I'm fine with this change. A newly discovered (but pre-existing, so non-consequential to these patches) problem associated with vlans is that we don't differentiate between "set vlan 0" and "don't set any vlan", which I hadn't even considered before, until this BZ came up: https://bugzilla.redhat.com/2035726 (The reporter is trying to use to "unset" the vlan tag for an interface)
Re: [libvirt PATCH 0/4] move virParseVersionString to virstring.c
On 1/28/22 3:58 PM, Ján Tomko wrote: And clean up some includes while doing it. Ján Tomko (4): maint: add required includes util: virParseVersionString: move to virstring.c virParseVersionString: rename to virStringParseVersion maint: remove unnecessary virutil.h includes Reviewed-by: Laine Stump but beware the slippery slope (e.g. - if you're going to move this one, then what about virIndexToDiskName and virDiskNameToIndex - they just operate on a string too...). Do we really want such specific functions in a file made for generally useful string functions? (Maybe we do, I don't have a strong opinion one way or the other, just thought I'd bring it up) src/bhyve/bhyve_driver.c | 2 +- src/ch/ch_conf.c | 2 +- src/esx/esx_vi.c | 9 ++--- src/libvirt_private.syms | 2 +- src/lxc/lxc_driver.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 6 +-- src/openvz/openvz_conf.c | 3 +- src/util/virdnsmasq.c | 3 +- src/util/virfirewalld.c | 4 +- src/util/virstring.c | 48 +++ src/util/virstring.h | 4 ++ src/util/virutil.c| 46 -- src/util/virutil.h| 3 -- src/vbox/vbox_common.c| 2 +- src/vmware/vmware_conf.c | 3 +- src/vz/vz_utils.c | 2 +- tests/testutilsqemu.c | 3 +- tests/utiltest.c | 2 +- tools/virt-host-validate-common.c | 2 +- 19 files changed, 73 insertions(+), 75 deletions(-)
Re: [libvirt PATCH] conf: network: remove hostname validation
On 1/27/22 4:33 PM, Ján Tomko wrote: We used to validate that the first character of the hostname is a letter. Later, RFC1123 relaxed the requirements to allow a number as well. Drop the validation completely, since we do not care about the following characters, and neither does dnsmasq (even if it's a comma, which is a delimiter in the hosts file). Was there some discussion somewhere that prompted this patch (and thus invalidates the opinion I'm about to spout)? The only email I could find about it was the email of the "reverted" patch itself (sent by Peter on behalf of the author, with r-b given in the same email). My opinion is that if a current RFC restricts the first letter of a hostname, then we should validate that too, *especially* if dnsmasq doesn't; who knows what entity beyond dnsmasq will barf on it in some way, and the closer to the source the non-compliance is reported, the easier it will be to fix. (Additionally, it's easy to remove extra validation, but much more difficult to add it back later if we decide it shouldn't have been removed) Reverts: 673b74be5fda928da5e9f3c2cfbf6c1cb1eda0c6 Signed-off-by: Ján Tomko --- src/conf/network_conf.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c769bbaeb5..8f50e22be5 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -548,12 +548,6 @@ virNetworkDHCPHostDefParseXML(const char *networkName, } name = virXMLPropString(node, "name"); -if (name && !(g_ascii_isalpha(name[0]) || g_ascii_isdigit(name[0]))) { -virReportError(VIR_ERR_XML_ERROR, - _("Cannot use host name '%s' in network '%s'"), - name, networkName); -return -1; -} ip = virXMLPropString(node, "ip"); if (ip && (virSocketAddrParse(, ip, AF_UNSPEC) < 0)) {
Re: [PATCH] domain_cgroup: Don't put semicolon at the end of VIR_GET_LIMIT_PARAMETER macro
On 1/28/22 6:15 AM, Michal Privoznik wrote: In domain_cgroup.c there's VIR_GET_LIMIT_PARAMETER macro which has a semicolon at the end of its declaration. Well, remove it so that the places where macro is used have to put the semicolon explicitly. This helps with automatic reformatting (at least in vim). Signed-off-by: Michal Privoznik Reviewed-by: Laine Stump
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On 1/14/22 3:29 PM, Ján Tomko wrote: On a Friday in 2022, Laine Stump wrote: Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. Well, you need to search for the word i instead of the letter i. grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \ vim searches for the word under the cursor with '*' by default Surely other search tools have some equivalent. This forced me to go look for it in emacs, and after 28 years, I've learned about isearch-forward-symbol-at-point, which is by default bound to [alt-s .]. But that's just another different keystroke I have to remember. Much easier if I can just use an expansion of the ctl-s (incremental search) that I already know and use for pretty much all searching within a single file. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-)) II thank you. JJano KKind of you, LLaine
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On 1/14/22 10:56 AM, Ján Tomko wrote: On a Friday in 2022, Tim Wiederhake wrote: This was not mentioned before. Signed-off-by: Tim Wiederhake --- docs/coding-style.rst | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: + int x; + int y; + + BAD: + int x, y; + Please use longer variable names and initialize some too, to illustrate it better, e.g.: int count = 0, nnodes; Personally I don't mind: size_t i, j; that much - even though removing one does cause churn, they are simple to read. I also don't mind combining simple things like that, but am willing to go full-isolated just for consistency's sake. Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-))
Re: .conf file setting(s) for packet filtering backend(s)
On 1/4/22 7:57 AM, Daniel P. Berrangé wrote: On Sun, Jan 02, 2022 at 09:41:37PM -0500, Laine Stump wrote: I'm currently working on switching the backend of the network driver from using iptables to using nftables. Due to some functionality that is not available with nftables (the rule that fixes up the checksum of DHCP packets which, btw, is only relevant for *very* old guests, e.g. RHEL5), this needs to be opt-in via a config file setting. In the meantime, in order to make this doable in a reasonable amount of time, I am *not* converting the nwfilter driver right away, and when I do it will need its own config file setting for opt-in. I've never before looked at the code for the .conf file settings at all. I had assumed there would be some sort of "pull" API, where code in the drivers could call, e.g. virConfGetString("filter_backend") and it would return the config setting to the caller. But when I look at it, I see that all daemons use the same daemonConfigLoadFile() called from remote_daemon.c:main() (which is shared by all the daemons) and the daemonConfig object that is created to hold the config settings that are read is only visible within main() - the only way that a config setting is used is by main() "pushing" it out to a static variable somewhere else where it is later retrieved by the interested party, e.g. the way that main() calls daemonSetupNetDevOpenvswitch(config), which then sets the static virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c. I'd consider the OVS approach to be a bad example Global state needing configurable parameters for stuff in util/ should generally be considered a design flaw. Okay, so then the setting of the host uuid is also a bad example (set into a static in util/viruuid.c), as is all the config for logging (set in statics in util/virlog.c by calling a function in util/virdaemon.c) :-) (Of course, those are things that *are* conceivably needed by all daemons, not just a subset of them, so I guess it's different, but still if you want to be pedantic, they do fit your description) Global state should be exclusively in the drivers, It just occurred to me that two different things are being discussed in parallel here without really thinking about it (at least by me) - daemon config, and driver config. In the past there was a single daemon, with its config in libvirtd.conf, and many drivers, each potentially having its own separate .conf file (but in practice there is only qemu.conf, lxc.conf, and libxl.conf - no .conf files for other drivers that I can see). Now with split daemons, each driver has its own daemon, and each daemon has its own .conf file (virtqemud.conf, virtlxc.conf, virtnetworkd.conf, ...) while drivers continue to have (or not have) their own separate conf file (qemu.conf, lxc.conf, libxl.conf). When the daemon split happened, I made the (incorrect) leap that the new virt*d.conf files were a convenient place for driver-specific config. Instead, I guess the virt*d.conf files should only be used for config related to operation of the daemon process, how it's connected to, logging of its error messages, etc., but shouldn't have any config specific to the driver that happens to be running in that daemon; for driver-specific things there should be a *.conf file (qemu.conf, and now it seems I will need network.conf) which is read by the driver itself. (not sure what should be done with ovs_timeout, which is, as you point out, in the wrong place) That seems like a lot of files, but I guess as long as it's got a (well documented) logic to it, it shouldn't be any worse than having fewer files each of a greater length :-) Anyway, rather than looking at what happens when virtnetworkd.conf is read and adding a new knob there, I really should be looking at qemu_conf.c and using that as an example to add parsing of a new network.conf (which doesn't currently exist) to the network driver (and later nwfilter.conf to the nwfilter driver) and then the desired values passed into the util APIs explicitly. Well, that *is* being done with ovs_timeout. It's just that the API being called is setting a static in the util/virnetdevopenvswitch.c (just as is done with logging config and host uuid), and then later used implicitly by other functions in the same file, rather than sending it as an arg to each API call that needs it. My guess is that since the setting is used for both qemu and lxc, the author figured putting a single instance of the config in libvirtd.conf would "make life simpler". ie ovs_timeout should have been in qemu.conf (any other drivers' config files if appropriate). Somehow I had always considered qemu.conf to be specifically for things related to starting the qemu process *only* (and not necessarily pertaining to the entire qemu driver), although even with that interpretation I guess ovs_timeout could be considered to be in that gro
Re: [PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy
On 1/4/22 11:59 AM, Michal Privoznik wrote: In a few places (e.g. device attach/detach/update) we are given a device XML, parse it but then need a copy of parsed data so that the original can be passed to function handling the request over inactive XML and the copy is then passed to function handling the operation over live XML. Note, both functions consume passed device on success, hence the need for copy. The problem is in combination of how the copy is obtained and where is passed. The copy is done by calling virDomainDeviceDefCopy() which does only inactive copy, i.e. no live information is copied over (e.g. no aliases). Then, this copy (inactive XML effectively) is passed to function handling live part of the operation (e.g. qemuDomainUpdateDeviceLive()) and the definition containing all the juicy, live bits is passed to function handling inactive part of the operation (e.g. qemuDomainUpdateDeviceConfig()). This is rather suboptimal, and XML copies should be passed to their respective functions. s/suboptimal/incorrect/ :-) It's funny that the comment at the top of virDomainDeviceDefCopy explicitly says that it's a copy of the inactive/config object, and that every single place that function is called, its result is being put into the live domain :-P At first I was concerned that there were no matching chunks for qemuDomainAttachDeviceFlags(), but then I took a look and realized that function parses the XML twice instead of parsing it once and making a copy (and I suppose I should mention the moment of cringe when, while looking at that function, I came across some piece of ugly treachery I had added to make sure the MAC addresses match between the copies. It seems like this cringe moment happens every time I open a file these days...) Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2036895 Signed-off-by: Michal Privoznik Reviewed-by: Laine Stump --- src/lxc/lxc_driver.c | 10 +- src/qemu/qemu_driver.c | 8 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fe583ccb76..7bc39120ee 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4308,17 +4308,17 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, false) < 0) goto endjob; -if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) +if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev_copy)) < 0) goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { -if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL, +if (virDomainDefCompatibleDevice(vm->def, dev, NULL, VIR_DOMAIN_DEVICE_ACTION_ATTACH, true) < 0) goto endjob; -if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev_copy)) < 0) +if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be @@ -4475,12 +4475,12 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob; -if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0) +if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev_copy)) < 0) goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { -if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0) +if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8537a4260..b1255da9f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8020,7 +8020,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, /* virDomainDefCompatibleDevice call is delayed until we know the * device we're going to update. */ -if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, priv->qemuCaps, +if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev_copy, priv->qemuCaps, parse_flags, driver->xmlopt)) < 0) goto endjob; @@ -8029,7 +8029,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { /* virDomainDefCompatibleDevice call is delayed until we know the * device we're going to update. */ -if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0) +if ((ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force)) < 0) goto endjob; qemuDomainSaveStatus(vm); @@ -8100,7 +8
.conf file setting(s) for packet filtering backend(s)
(this probably will make no sense to anyone who hasn't spent time looking at daemonConfig*, in which case you can go ahead and hit Delete now. At any rate I'm just tossing this out into the void to see if anyone has any ideas/opinions, so in *any* case feel free to hit delete!) Happy New Year! and time for another bit of confused ramblings trying to figure out how to do something that ends up being non-confused and straightforward. I'm currently working on switching the backend of the network driver from using iptables to using nftables. Due to some functionality that is not available with nftables (the rule that fixes up the checksum of DHCP packets which, btw, is only relevant for *very* old guests, e.g. RHEL5), this needs to be opt-in via a config file setting. In the meantime, in order to make this doable in a reasonable amount of time, I am *not* converting the nwfilter driver right away, and when I do it will need its own config file setting for opt-in. I've never before looked at the code for the .conf file settings at all. I had assumed there would be some sort of "pull" API, where code in the drivers could call, e.g. virConfGetString("filter_backend") and it would return the config setting to the caller. But when I look at it, I see that all daemons use the same daemonConfigLoadFile() called from remote_daemon.c:main() (which is shared by all the daemons) and the daemonConfig object that is created to hold the config settings that are read is only visible within main() - the only way that a config setting is used is by main() "pushing" it out to a static variable somewhere else where it is later retrieved by the interested party, e.g. the way that main() calls daemonSetupNetDevOpenvswitch(config), which then sets the static virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c. (NB: util/virnetdevopenvswitch.c is linked into every deamon, so even for the daemons that don't use it, calls to virnetdevopenvswitch.c functions still compile properly (and calling them is harmless), so virNetDevOpenvswitchTimeout is set even for daemons that never call openvswitch APIs). If I could count on all builds using split daemons (i.e. separate virtnetworkd and virtnwfilterd) then I could add a similar API in virfirewall.c that remote_daemon.c:main() could use to set "filter_backend" into a static in virfirewall.c (which is used by both drivers) and everything would just happily work: virtnetworkd.conf: filter_backend = nftables virtnwfilterd.conf filter_backend = iptables However, I need to also deal with the possibility that the nwfilter and network drivers are in the same unified libvirtd binary, and in that case both drivers would have the same virfirewall.c:filter_backend setting, thus making it impossible to use the iptables backend for the nwfilter driver and nftables backend for the network driver. For that case I would need separate settings in the config for each driver, e.g. libvirtd.conf: network_filter_backend = nftables nwfilter_backend = iptables and then those would need to be stored off somewhere different for each driver, then they would use it to set the backend for each virFirewall object as it is created. Organizationally, it would make the most sense for these settings (and the API to set them) to be located in the drivers that use them (so, for example, network_filter_backend could live in network/bridge_driver_linux.c and nwfilter_backend could live in nwfilter/nwfilter_driver.c). But that would mean that remote_daemon.c:main() would need to directly call functions in those files, which is a no-no (because, in the case of split daemons, you either have one or the other at build time, but never both). So should I perhaps declare the nftables backend for nwfilter to be a lost cause until everyone moves to split daemons, add a "filter_backend" setting that is directly set in virfirewall.c (by remote_daemon.c:main()), and then provide some sort of override in virFirewallNew so calls from the nwfilter driver can say "ignore the filter_backend setting and use iptables"? Or should we make the virConf APIs beefier, and add facilities to save off the entire daemonConfig object and make its contents available via something like virConfGetString("network_filter_backend")? But if I did that, it would mean two differently-named config entries, and it would certainly be nice if I didn't have to introduce daemon-specific names like this that would need to be carried over from libvirtd.conf into virtnwfilterd.conf and virtnetworkd.conf (where differing names would no longer be required). I suppose I could go "full MS" and introduce the concept of sections to the conf file, so libvirtd.conf could have something like this: [network] filter_backend = nftables [nwfilter] filter_backend = iptables but that seems like a lot of work for something that will be obsolete
Re: [libvirt PATCH 0/3] tests: virtimetest: Skip more tests near year's end
On 1/1/22 2:07 PM, Andrea Bolognani wrote: Failed pipeline: https://gitlab.com/libvirt/libvirt/-/pipelines/439750804 Fixed pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/439847251 Andrea Bolognani (3): tests: virtimetest: Move comment tests: virtimetest: Skip more tests near year's end Ugh, I had managed to repress the memories of this, and now it's come back :-O tests: virtimetest: Mention GLib version containing fix tests/virtimetest.c | 58 ++--- 1 file changed, 28 insertions(+), 30 deletions(-) Reviewed-by: Laine Stump *if* you can explain to me why (at 11:55PM local time on Jan 1) all 28 of virtimetest cases were run by "ninja -C build test", with test 27 & 28 failing, *but* when I just ran build/tests/virtimetest directly, it succeeded - running it under gdb I found that isNearYearEnd() properly returned true, so the final 8 tests aren't run (as you would expect). Not only that, but when I ran "ninja -C build test" at 12:01AM Jan 2, test 28 failed; it also failed when I directly ran virtimetest. For the first problem - is it possible that ninja is setting the timezone to something different during the build? My machine is in EST (UTC -5). For the 2nd problem - maybe we need to widen the definition of "isNearYearEnd()"? If I make the following change then the final tests are skipped by both ninja -C build test and virtimetest directly when the date is 12;14AM Jan 2. Oh, wait. That solves the 1st problem too :-) diff --git a/tests/virtimetest.c b/tests/virtimetest.c index 5d0c0717ca..383a4cfa59 100644 --- a/tests/virtimetest.c +++ b/tests/virtimetest.c @@ -104,7 +104,7 @@ isNearYearEnd(void) g_autoptr(GDateTime) now = g_date_time_new_now_local(); return ((g_date_time_get_month(now) == 1 && - g_date_time_get_day_of_month(now) == 1) || + g_date_time_get_day_of_month(now) <= 2) || (g_date_time_get_month(now) == 12 && g_date_time_get_day_of_month(now) == 31)); } (I was just kidding about the "*if*" above - you have my Reviewed-by: in any case. But unless you want to spend the time figuring out why my tests failed on Jan 1/early Jan 2, I'm going to send the above diff as a patch)
Re: [libvirt PATCH 00/17] Bump minimum dnsmasq version
On 12/14/21 2:09 PM, Ján Tomko wrote: This bumps the minimum dnsmasq version to the point where we do not need capability probing, reducing it to a version check (which I will be happy to remove on request). Unless I missed something, this also means we no longer need to spawn radvd manually. The code doesn't lie! If removing the bits that were only true for older dnsmasq removed the lines that ran radvd, then it's true. (I recall that support for RA was added to dnsmasq fairly soon after the original ipv6 support was added, and radvd was left in libvirt only because there were so many downstreams that still had an older dnsmasq). Note that DNSMASQ_CAPS_BINDTODEVICE was the indication of a downstream mitigation of a CVE that should no longer be needed if we have --bind-dynamic [...] 17 files changed, 83 insertions(+), 569 deletions(-) Nice!!! After the minor fixes I noted in 03/17 and 08/17 Reviewed-by: Laine Stump /me ponders what I should idly suggest be removed next...
Re: [libvirt PATCH 08/17] network: assume DNSMASQ_CAPS_RA_PARAM
On 12/14/21 2:09 PM, Ján Tomko wrote: Introduced by: "Introduced by dnsmasq commit:" commit c4cd95df68b573b63d234ecdb675228657d65353 Author: Simon Kelley CommitDate: 2013-10-10 20:58:11 +0100 Add --ra-param and remove --force-fast-ra git describe: v2.67rc3-3-gc4cd95d contains: v2.67rc4~12 Signed-off-by: Ján Tomko --- src/network/bridge_driver.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index dffe4e1574..a4535b1b49 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1197,10 +1197,8 @@ networkDnsmasqConfContents(virNetworkObj *obj, if (def->forward.type == VIR_NETWORK_FORWARD_NONE) { virBufferAddLit(, "dhcp-option=3\n" "no-resolv\n"); -if (dnsmasqCapsGet(caps, DNSMASQ_CAPS_RA_PARAM)) { -/* interface=* (any), interval=0 (default), lifetime=0 (seconds) */ -virBufferAddLit(, "ra-param=*,0,0\n"); -} +/* interface=* (any), interval=0 (default), lifetime=0 (seconds) */ +virBufferAddLit(, "ra-param=*,0,0\n"); } if (wantDNS) {
Re: [libvirt PATCH 03/17] util: dnsmasq: mandate at least version 2.67
On 12/14/21 2:09 PM, Ján Tomko wrote: All the capabilities should be supported in 2.67. Make this the minimum version, since even the oldest distros we support have moved on: Debian 8: 2.72 CentOS 7: 2.76 Ubuntu 18.04: 2.79 Signed-off-by: Ján Tomko --- src/util/virdnsmasq.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 90a1ea35b6..efe65174f8 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -49,6 +49,9 @@ VIR_LOG_INIT("util.dnsmasq"); #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" #define DNSMASQ_ADDNHOSTSFILE_SUFFIX "addnhosts" +#define DNSMASQ_MIN_MAJOR 2 +#define DNSMASQ_MIN_MINOR 67 + static void dhcphostFreeContent(dnsmasqDhcpHost *host) { @@ -627,6 +630,16 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) if (virParseVersionString(p, >version, true) < 0) goto error; +if (caps->version / 100 < DNSMASQ_MIN_MAJOR || +caps->version % 100 < DNSMASQ_MIN_MINOR) { I think you actually want something like: if (caps->version < DNSMASQ_MIN_MAJOR * 100 + DNSMASQ_MIN_MINOR * 1000) (or if you wanted to avoid giving this file the knowledge of how version numbers are represented internally, you could #define DNSMASQ_MIN_VERSION "2.67", then use virParseVersionString() to parse that into an unsigned long, and then compare that result. That seems like overkill though) +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("dnsmasq version >= %u.%u required but %lu.%lu found"), + DNSMASQ_MIN_MAJOR, DNSMASQ_MIN_MINOR, + caps->version / 100, + caps->version % 100); +goto error; +} + if (strstr(buf, "--bind-dynamic")) dnsmasqCapsSet(caps, DNSMASQ_CAPS_BIND_DYNAMIC);
Re: [libvirt PATCH 09/17] util: dnsmasq: delete assumed capability flags
On 12/14/21 2:09 PM, Ján Tomko wrote: Signed-off-by: Ján Tomko --- src/util/virdnsmasq.c | 22 ++ src/util/virdnsmasq.h | 4 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index efe65174f8..016d9d64a8 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -640,27 +640,9 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) goto error; } -if (strstr(buf, "--bind-dynamic")) -dnsmasqCapsSet(caps, DNSMASQ_CAPS_BIND_DYNAMIC); - -/* if this string is a part of the --version output, dnsmasq - * has been patched to use SO_BINDTODEVICE when listening, - * so that it will only accept requests that arrived on the - * listening interface(s) - */ -if (strstr(buf, "--bind-interfaces with SO_BINDTODEVICE")) -dnsmasqCapsSet(caps, DNSMASQ_CAPS_BINDTODEVICE); - -if (strstr(buf, "--ra-param")) -dnsmasqCapsSet(caps, DNSMASQ_CAPS_RA_PARAM); - -VIR_INFO("dnsmasq version is %d.%d, --bind-dynamic is %spresent, " - "SO_BINDTODEVICE is %sin use, --ra-param is %spresent", +VIR_INFO("dnsmasq version is %d.%d", (int)caps->version / 100, - (int)(caps->version % 100) / 1000, - dnsmasqCapsGet(caps, DNSMASQ_CAPS_BIND_DYNAMIC) ? "" : "NOT ", - dnsmasqCapsGet(caps, DNSMASQ_CAPS_BINDTODEVICE) ? "" : "NOT ", - dnsmasqCapsGet(caps, DNSMASQ_CAPS_RA_PARAM) ? "" : "NOT "); + (int)(caps->version % 100) / 1000); One would hope that nobody is actually looking for these strings in a script anywhere :-/ (To clarify - I think it's fine to remove).
Re: [libvirt PATCH 2/5] util: dnsmasq: refactor CapsRefresh
On 12/13/21 1:58 PM, Ján Tomko wrote: Use two variables with automatic cleanup instead of reusing one. Remove the pointless cleanup label. Signed-off-by: Ján Tomko --- src/util/virdnsmasq.c | 37 - 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 2dd9a20377..b62e353ceb 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -666,9 +666,9 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) static int dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) { -int ret = -1; struct stat sb; -virCommand *cmd = NULL; +g_autoptr(virCommand) vercmd = NULL; +g_autoptr(virCommand) helpcmd = NULL; g_autofree char *help = NULL; g_autofree char *version = NULL; g_autofree char *complete = NULL; @@ -692,31 +692,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) if (!virFileIsExecutable(caps->binaryPath)) { virReportSystemError(errno, _("dnsmasq binary %s is not executable"), caps->binaryPath); -goto cleanup; +return -1; } -cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); -virCommandSetOutputBuffer(cmd, ); -virCommandAddEnvPassCommon(cmd); -virCommandClearCaps(cmd); -if (virCommandRun(cmd, NULL) < 0) -goto cleanup; -virCommandFree(cmd); +vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); +virCommandSetOutputBuffer(vercmd, ); +virCommandAddEnvPassCommon(vercmd); +virCommandClearCaps(vercmd); +if (virCommandRun(vercmd, NULL) < 0) +return -1; Hmmm. Every time I run across this code, I wonder if we should keep it or just remove it completely - the "newest" feature we're checking for was added to dnsmasq in version 2.67, which was released in late 2013. So all these extra executions of dnsmasq to get the version# and parse the help output are just producing the same results for everyone. On the other hand, it's possible some new feature could be added to dnsmasq in the future that we would want to check for, and that would be easier to add if the basic structure of the code was still here. I'm not sure how likely that is at this point though - dnsmasq (and libvirt's use of dnsmasq) is fairly mature at this point, so keeping the code is just creating more maintenance burden for nothing...
[libvirt PATCH 02/12] util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix
This function formats an address + prefix as, e.g. 192.168.122.0/24, which is useful in places other than iptables. Move it to virsocketaddr.c and make it public so that others can use it. While moving, the bit that masks off the host bits of the address is made optional, so that the function is more generally useful. Signed-off-by: Laine Stump --- src/libvirt_private.syms | 1 + src/util/viriptables.c | 41 + src/util/virsocketaddr.c | 44 src/util/virsocketaddr.h | 3 +++ 4 files changed, 53 insertions(+), 36 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ff6f71054e..72b38a970d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3269,6 +3269,7 @@ virSocketAddrCheckNetmask; virSocketAddrEqual; virSocketAddrFormat; virSocketAddrFormatFull; +virSocketAddrFormatWithPrefix; virSocketAddrGetIPPrefix; virSocketAddrGetNumNetmaskBits; virSocketAddrGetPath; diff --git a/src/util/viriptables.c b/src/util/viriptables.c index ac949efba7..78d979cfe8 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -353,37 +353,6 @@ iptablesRemoveUdpOutput(virFirewall *fw, } -static char *iptablesFormatNetwork(virSocketAddr *netaddr, - unsigned int prefix) -{ -virSocketAddr network; -g_autofree char *netstr = NULL; -char *ret; - -if (!(VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET) || - VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET6))) { -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only IPv4 or IPv6 addresses can be used with iptables")); -return NULL; -} - -if (virSocketAddrMaskByPrefix(netaddr, prefix, ) < 0) { -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failure to mask address")); -return NULL; -} - -netstr = virSocketAddrFormat(); - -if (!netstr) -return NULL; - -ret = g_strdup_printf("%s/%d", netstr, prefix); - -return ret; -} - - /* Allow all traffic coming from the bridge, with a valid network address * to proceed to WAN */ @@ -399,7 +368,7 @@ iptablesForwardAllowOut(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; -if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) +if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -487,7 +456,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; -if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) +if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -577,7 +546,7 @@ iptablesForwardAllowIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; -if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) +if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) @@ -829,7 +798,7 @@ iptablesForwardMasquerade(virFirewall *fw, virFirewallLayer layer = af == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; -if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) +if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(>start, af)) { @@ -972,7 +941,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; -if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) +if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) return -1; if (physdev && physdev[0]) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 94cbfc6264..430e43f2eb 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -511,6 +511,50 @@ virSocketAddrFormatFull(const virSocketAddr *addr, } +/* + * virSocketAddrFormatWithPrefix: + * @addr: an initialized virSocketAddr * + * @prefix: an IP network prefix (0-32 if IPv4, 0-128 if IPv6) + * @masked: true to mask off the host bits of the address + * + * Returns a string representation of the IP network described by + * @netaddr/@prefix. If @masked is true, the address is masked to + * remove the host bits according to prefix. So, for example, sending + * f(1.2.3.4, 24, true) would return "1.2.3.0/24", but f(1.2.3.4, 24, + * false) would return "1.2.3.4/24". + * + * returns
[libvirt PATCH 09/12] util: move and rename virFirewallBackendSynchronize()
This function doesn't have anything to do with manipulating virFirewall objects, but rather should be called in response to dbus events about the firewalld service. Move this function into virfirewalld.c, and rename it to virFirewallDSynchronize(). Signed-off-by: Laine Stump --- src/libvirt_private.syms | 2 +- src/util/virfirewall.c | 43 src/util/virfirewall.h | 2 -- src/util/virfirewalld.c | 43 src/util/virfirewalld.h | 2 ++ src/util/viriptables.c | 3 ++- 6 files changed, 48 insertions(+), 47 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 72b38a970d..23385ec7a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2308,7 +2308,6 @@ virFileCacheSetPriv; # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; -virFirewallBackendSynchronize; virFirewallFree; virFirewallNew; virFirewallRemoveRule; @@ -2329,6 +2328,7 @@ virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; virFirewallDIsRegistered; +virFirewallDSynchronize; virFirewallDZoneExists; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 2fc9f94729..f3172e5c96 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -611,49 +611,6 @@ virFirewallApplyRuleFirewallD(virFirewallRule *rule, } -void -virFirewallBackendSynchronize(void) -{ -const char *arg = "-V"; -g_autofree char *output = NULL; -int firewallDRegistered = virFirewallDIsRegistered(); - -/* - * virFirewallBackendSynchronize() should be called after - * receiving an ownership-change event or reload event for - * firewalld from dbus, prior to performing any operations on the - * default table "filter". - * - * Our iptables filter rules are added to (private chains within) - * the default table named "filter", which is flushed by firewalld - * any time it is restarted or reloads its rules. libvirt watches - * for notifications that firewalld has been restarted / its rules - * reloaded, and then reloads the libvirt rules. But it's possible - * for libvirt to be notified that firewalld has restarted prior - * to firewalld completing initialization, and when that race - * happens, firewalld can potentially flush out rules that libvirt - * has just added! - * - * To prevent this, we send a simple command ("iptables -V") via - * firewalld's passthrough iptables API, and wait until it's - * finished before sending our own directly-executed iptables - * commands. This assures that firewalld has fully initialized and - * caught up with its internal queue of iptables commands, and - * won't stomp all over the new rules we subsequently add. - * - */ - -VIR_DEBUG("Firewalld is registered ? %d", firewallDRegistered); - -if (firewallDRegistered < 0) -return; /* firewalld (or dbus?) not functional, don't sync */ - -ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, - (char **), 1, true, )); -VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); -} - - static int virFirewallApplyRule(virFirewall *firewall, virFirewallRule *rule, diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 169d99fe2b..7448825dbc 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -109,6 +109,4 @@ void virFirewallStartRollback(virFirewall *firewall, int virFirewallApply(virFirewall *firewall); -void virFirewallBackendSynchronize(void); - G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFirewall, virFirewallFree); diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 3178bf4b3d..4795bf7925 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -368,3 +368,46 @@ virFirewallDInterfaceSetZone(const char *iface, "changeZoneOfInterface", message); } + + +void +virFirewallDSynchronize(void) +{ +const char *arg = "-V"; +g_autofree char *output = NULL; +int firewallDRegistered = virFirewallDIsRegistered(); + +/* + * virFirewallDSynchronize() should be called after receiving an + * ownership-change event or reload event for firewalld from dbus, + * prior to performing any operations on the default table + * "filter". + * + * Our iptables filter rules are added to (private chains within) + * the default table named "filter", which is flushed by firewalld + * any time it is restarted or reloads its rules. libvirt watches + * for notifications that firewalld has been restarted / its rules + * reloaded, and then reloads the libvirt rules. But it's possible + * for libvirt to be notified that firewalld has restarted prior +
[libvirt PATCH 08/12] util: simplify virFirewallBackendSynchronize()
This function doesn't need to check for a backend - synchronization with firewalld should always be done whenever firewalld is registered and available, not just when the firewalld backend is selected. Signed-off-by: Laine Stump --- src/util/virfirewall.c | 54 ++ src/util/viriptables.c | 6 ++--- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bb14a367d9..2fc9f94729 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -616,27 +616,41 @@ virFirewallBackendSynchronize(void) { const char *arg = "-V"; g_autofree char *output = NULL; +int firewallDRegistered = virFirewallDIsRegistered(); + +/* + * virFirewallBackendSynchronize() should be called after + * receiving an ownership-change event or reload event for + * firewalld from dbus, prior to performing any operations on the + * default table "filter". + * + * Our iptables filter rules are added to (private chains within) + * the default table named "filter", which is flushed by firewalld + * any time it is restarted or reloads its rules. libvirt watches + * for notifications that firewalld has been restarted / its rules + * reloaded, and then reloads the libvirt rules. But it's possible + * for libvirt to be notified that firewalld has restarted prior + * to firewalld completing initialization, and when that race + * happens, firewalld can potentially flush out rules that libvirt + * has just added! + * + * To prevent this, we send a simple command ("iptables -V") via + * firewalld's passthrough iptables API, and wait until it's + * finished before sending our own directly-executed iptables + * commands. This assures that firewalld has fully initialized and + * caught up with its internal queue of iptables commands, and + * won't stomp all over the new rules we subsequently add. + * + */ -switch (currentBackend) { -case VIR_FIREWALL_BACKEND_DIRECT: -/* nobody to synchronize with */ -break; -case VIR_FIREWALL_BACKEND_FIREWALLD: -/* Send a simple rule via firewalld's passthrough iptables - * command so that we'll be sure firewalld has fully - * initialized and caught up with its internal queue of - * iptables commands. Waiting for this will prevent our own - * directly-executed iptables commands from being run while - * firewalld is still initializing. - */ -ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, - (char **), 1, true, )); -VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); -break; -case VIR_FIREWALL_BACKEND_AUTOMATIC: -case VIR_FIREWALL_BACKEND_LAST: -break; -} +VIR_DEBUG("Firewalld is registered ? %d", firewallDRegistered); + +if (firewallDRegistered < 0) +return; /* firewalld (or dbus?) not functional, don't sync */ + +ignore_value(virFirewallDApplyRule(VIR_FIREWALL_LAYER_IPV4, + (char **), 1, true, )); +VIR_DEBUG("Result of 'iptables -V' via firewalld: %s", NULLSTR(output)); } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index d2bc10a652..34ce9cd018 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -138,10 +138,10 @@ iptablesSetupPrivateChains(virFirewallLayer layer) }; size_t i; -/* When the backend is firewalld, we need to make sure that +/* When firewalld.service is active, we need to make sure that * firewalld has been fully started and completed its - * initialization, otherwise firewalld might delete our rules soon - * after we add them! + * initialization, otherwise it might delete our rules soon after + * we add them! */ virFirewallBackendSynchronize(); -- 2.33.1
[libvirt PATCH 11/12] util: remove currentBackend from virfirewall.c
Since the currentBackend (direct vs. firewalld) setting is no longer used for anything, we don't need to set it (either explicitly from tests, or implicitly during init), and can completely remove it. Signed-off-by: Laine Stump --- src/libvirt_private.syms | 1 - src/util/virfirewall.c | 50 ++-- src/util/virfirewallpriv.h | 37 --- tests/networkxml2firewalltest.c | 8 + tests/nwfilterebiptablestest.c | 7 - tests/nwfilterxml2firewalltest.c | 8 + tests/virfirewalltest.c | 7 ++--- 7 files changed, 6 insertions(+), 112 deletions(-) delete mode 100644 src/util/virfirewallpriv.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 23385ec7a1..bb90659365 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2316,7 +2316,6 @@ virFirewallRuleAddArgFormat; virFirewallRuleAddArgList; virFirewallRuleAddArgSet; virFirewallRuleGetArgCount; -virFirewallSetBackend; virFirewallStartRollback; virFirewallStartTransaction; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 1e6c667ee1..98d78857df 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -22,8 +22,7 @@ #include -#define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -#include "virfirewallpriv.h" +#include "virfirewall.h" #include "virfirewalld.h" #include "viralloc.h" #include "virerror.h" @@ -81,61 +80,16 @@ struct _virFirewall { size_t currentGroup; }; -static virFirewallBackend currentBackend = VIR_FIREWALL_BACKEND_AUTOMATIC; static virMutex ruleLock = VIR_MUTEX_INITIALIZER; -static int -virFirewallValidateBackend(virFirewallBackend backend); - static int virFirewallOnceInit(void) { -return virFirewallValidateBackend(currentBackend); -} - -VIR_ONCE_GLOBAL_INIT(virFirewall); - -static int -virFirewallValidateBackend(virFirewallBackend backend) -{ -if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || -backend == VIR_FIREWALL_BACKEND_FIREWALLD) { -int rv = virFirewallDIsRegistered(); - -VIR_DEBUG("Firewalld is registered ? %d", rv); - -if (rv == -1) -return -1; - -if (rv == -2) { -if (backend == VIR_FIREWALL_BACKEND_FIREWALLD) { -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld backend requested, but service is not running")); -return -1; -} else { -VIR_DEBUG("firewalld service not running, using direct backend"); -backend = VIR_FIREWALL_BACKEND_DIRECT; -} -} else { -VIR_DEBUG("firewalld service running, using firewalld backend"); -backend = VIR_FIREWALL_BACKEND_FIREWALLD; -} -} - -currentBackend = backend; return 0; } -int -virFirewallSetBackend(virFirewallBackend backend) -{ -currentBackend = backend; - -if (virFirewallInitialize() < 0) -return -1; +VIR_ONCE_GLOBAL_INIT(virFirewall); -return virFirewallValidateBackend(backend); -} static virFirewallGroup * virFirewallGroupNew(void) diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h deleted file mode 100644 index b846f8799c..00 --- a/src/util/virfirewallpriv.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * virfirewallpriv.h: integration with firewalls private APIs - * - * Copyright (C) 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - */ - -#ifndef LIBVIRT_VIRFIREWALLPRIV_H_ALLOW -# error "virfirewallpriv.h may only be included by virfirewall.c or test suites" -#endif /* LIBVIRT_VIRFIREWALLPRIV_H_ALLOW */ - -#pragma once - -#include "virfirewall.h" - -typedef enum { -VIR_FIREWALL_BACKEND_AUTOMATIC, -VIR_FIREWALL_BACKEND_DIRECT, -VIR_FIREWALL_BACKEND_FIREWALLD, - -VIR_FIREWALL_BACKEND_LAST, -} virFirewallBackend; - -int virFirewallSetBackend(virFirewallBackend backend); diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 68a82e60d6..11be85e06f 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -31,9 +31,7 @@ # include "network/bridge_driver_platform.h" # includ
[libvirt PATCH 07/12] util: eliminate pointless switch in virFirewallApplyRule
Since commit b19863640 both useful cases of the switch statement in this function have made the same call (and the other/default case is just an error that can never happen). Eliminate the switch to help eliminate use of currentBackend. Signed-off-by: Laine Stump --- src/util/virfirewall.c | 25 + 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 1a546335f6..bb14a367d9 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -653,31 +653,8 @@ virFirewallApplyRule(virFirewall *firewall, if (rule->ignoreErrors) ignoreErrors = rule->ignoreErrors; -switch (currentBackend) { -case VIR_FIREWALL_BACKEND_DIRECT: -if (virFirewallApplyRuleDirect(rule, ignoreErrors, ) < 0) -return -1; -break; -case VIR_FIREWALL_BACKEND_FIREWALLD: -/* Since we are using raw iptables rules, there is no - * advantage to going through firewalld, so instead just add - * them directly rather that via dbus calls to firewalld. This - * has the useful side effect of eliminating extra unwanted - * warning messages in the system logs when trying to delete - * rules that don't exist (which is something that happens - * often when libvirtd is started, and *always* when firewalld - * is restarted) - */ -if (virFirewallApplyRuleDirect(rule, ignoreErrors, ) < 0) -return -1; -break; - -case VIR_FIREWALL_BACKEND_AUTOMATIC: -case VIR_FIREWALL_BACKEND_LAST: -default: -virReportEnumRangeError(virFirewallBackend, currentBackend); +if (virFirewallApplyRuleDirect(rule, ignoreErrors, ) < 0) return -1; -} if (rule->queryCB && output) { if (!(lines = g_strsplit(output, "\n", -1))) -- 2.33.1
[libvirt PATCH 12/12] util: remove virFirewallOnceInit()
There is no longer anything to initialize at binary startup time. Signed-off-by: Laine Stump --- src/util/virfirewall.c | 16 +--- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 98d78857df..70092f2ef6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -82,15 +82,6 @@ struct _virFirewall { static virMutex ruleLock = VIR_MUTEX_INITIALIZER; -static int -virFirewallOnceInit(void) -{ -return 0; -} - -VIR_ONCE_GLOBAL_INIT(virFirewall); - - static virFirewallGroup * virFirewallGroupNew(void) { @@ -110,12 +101,7 @@ virFirewallGroupNew(void) */ virFirewall *virFirewallNew(void) { -virFirewall *firewall; - -if (virFirewallInitialize() < 0) -return NULL; - -firewall = g_new0(virFirewall, 1); +virFirewall *firewall = g_new0(virFirewall, 1); return firewall; } -- 2.33.1
[libvirt PATCH 10/12] util: remove check for iptables binary during virFirewallInit
It's unclear exactly why this check exists; possibly a parallel to a long-removed check for the firewall-cmd binary (added to viriptables.c with the initial support for firewalld in commit bf156385a03 in 2012, and long since removed), or possibly because virFirewallOnceInit() was intended to be called at daemon startup, and it seemed like a good idea to just log this error once when trying to determine whether to use firewalld, or direct iptables commands, and then not waste time building commands that could never be executed. The odd thing is that it would sometimes result in logging an error when it couldn't find a binary that wasn't needed anyway (e.g., if all the rules were iptables rules, but ebtables and/or ip6tables weren't also installed). If we just remove this check, then virCommandRun() will end up logging an error and failing if the needed binary isn't found when we try to execute it, which seems like it should just as good (or at least good enough, especially since we eventually want to get rid of iptables completely). So let's remove it! Signed-off-by: Laine Stump --- src/util/virfirewall.c | 25 - 1 file changed, 25 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index f3172e5c96..1e6c667ee1 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -98,23 +98,6 @@ VIR_ONCE_GLOBAL_INIT(virFirewall); static int virFirewallValidateBackend(virFirewallBackend backend) { -const char *commands[] = { -IPTABLES, IP6TABLES, EBTABLES -}; -size_t i; - -for (i = 0; i < G_N_ELEMENTS(commands); i++) { -g_autofree char *path = virFindFileInPath(commands[i]); - -if (!path) { -virReportSystemError(errno, - _("%s not available, firewall backend will not function"), - commands[i]); -return -1; -} -} -VIR_DEBUG("found iptables/ip6tables/ebtables"); - if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || backend == VIR_FIREWALL_BACKEND_FIREWALLD) { int rv = virFirewallDIsRegistered(); @@ -694,14 +677,6 @@ virFirewallApply(virFirewall *firewall) virMutexLock(); -if (currentBackend == VIR_FIREWALL_BACKEND_AUTOMATIC) { -/* a specific backend should have been set when the firewall - * object was created. If not, it means none was found. - */ -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to initialize a valid firewall backend")); -goto cleanup; -} if (!firewall || firewall->err) { int err = EINVAL; -- 2.33.1
[libvirt PATCH 03/12] util: rename iptables operators to something less generic
Rather than calling these "ADD" and "REMOVE", which could be confused with some other random items with the same names, make them more specific by prepending "VIR_NETFILTER_" (because they will also be used by the nftables backend) and rename them to match the iptables/nftables operators they signify, i.e. INSERT and DELETE, just to eliminate confusion (in particular, in case someone ever decides that we need to also use the nftables "add" operator, which appends a rule to a chain rather than inserting it at the beginning of the chain). Signed-off-by: Laine Stump --- src/util/viriptables.c | 97 +++--- 1 file changed, 53 insertions(+), 44 deletions(-) diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 78d979cfe8..d2bc10a652 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -43,8 +43,8 @@ VIR_LOG_INIT("util.iptables"); #define VIR_FROM_THIS VIR_FROM_NONE enum { -ADD = 0, -REMOVE +VIR_NETFILTER_INSERT = 0, +VIR_NETFILTER_DELETE }; typedef struct { @@ -175,7 +175,7 @@ iptablesInput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_INP", "--in-interface", iface, "--protocol", tcp ? "tcp" : "udp", @@ -199,7 +199,7 @@ iptablesOutput(virFirewall *fw, virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_OUT", "--out-interface", iface, "--protocol", tcp ? "tcp" : "udp", @@ -223,7 +223,7 @@ iptablesAddTcpInput(virFirewall *fw, const char *iface, int port) { -iptablesInput(fw, layer, iface, port, ADD, 1); +iptablesInput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 1); } /** @@ -241,7 +241,7 @@ iptablesRemoveTcpInput(virFirewall *fw, const char *iface, int port) { -iptablesInput(fw, layer, iface, port, REMOVE, 1); +iptablesInput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 1); } /** @@ -259,7 +259,7 @@ iptablesAddUdpInput(virFirewall *fw, const char *iface, int port) { -iptablesInput(fw, layer, iface, port, ADD, 0); +iptablesInput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 0); } /** @@ -277,7 +277,7 @@ iptablesRemoveUdpInput(virFirewall *fw, const char *iface, int port) { -iptablesInput(fw, layer, iface, port, REMOVE, 0); +iptablesInput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 0); } /** @@ -295,7 +295,7 @@ iptablesAddTcpOutput(virFirewall *fw, const char *iface, int port) { -iptablesOutput(fw, layer, iface, port, ADD, 1); +iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 1); } /** @@ -313,7 +313,7 @@ iptablesRemoveTcpOutput(virFirewall *fw, const char *iface, int port) { -iptablesOutput(fw, layer, iface, port, REMOVE, 1); +iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 1); } /** @@ -331,7 +331,7 @@ iptablesAddUdpOutput(virFirewall *fw, const char *iface, int port) { -iptablesOutput(fw, layer, iface, port, ADD, 0); +iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_INSERT, 0); } /** @@ -349,7 +349,7 @@ iptablesRemoveUdpOutput(virFirewall *fw, const char *iface, int port) { -iptablesOutput(fw, layer, iface, port, REMOVE, 0); +iptablesOutput(fw, layer, iface, port, VIR_NETFILTER_DELETE, 0); } @@ -374,7 +374,7 @@ iptablesForwardAllowOut(virFirewall *fw, if (physdev && physdev[0]) virFirewallAddRule(fw, layer, "--table", "filter", - action == ADD ? "--insert" : "--delete", + action == VIR_NETFILTER_INSERT ? "--insert" : "--delete", "LIBVIRT_FWO", "--source", networkstr, "--in-interface", iface, @@ -384,7 +384,7 @@ iptablesForwardAllow
[libvirt PATCH 06/12] tests: document why virgdbus must be mocked in networkxml2firewalltest.c
It isn't intuitive (to me) that a test just converting xml text into iptables commands should need to call dbus, so rather than forcing the next person to look through the commit logs and/or run the test under gdb to understand why this is needed, just add a short comment in the source. Signed-off-by: Laine Stump --- tests/networkxml2firewalltest.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index e4f86bc3fc..68a82e60d6 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -187,6 +187,12 @@ mymain(void) return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } +/* NB: virgdbus must be mocked because this test calls + * networkAddFirewallRules(), which will always call + * virFirewallDIsRegistered(), which calls + * virGDBusIsServiceRegistered(). + */ + VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virgdbus"), VIR_TEST_MOCK("virfirewall")) -- 2.33.1
[libvirt PATCH 05/12] tests: remove unnecessary ret variables and cleanup labels
Several functions were simplified to remove the only cleanup code at the cleanup label, making it unnecessary. Signed-off-by: Laine Stump --- tests/virfirewalltest.c | 92 ++--- 1 file changed, 31 insertions(+), 61 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index e6c41d89fa..724d3081f1 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -64,7 +64,6 @@ testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); -int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -87,19 +86,17 @@ testFirewallSingleGroup(const void *opaque G_GNUC_UNUSED) "--jump", "REJECT", NULL); if (virFirewallApply(fw) < 0) -goto cleanup; +return -1; actual = virBufferCurrentContent(); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); -goto cleanup; +return -1; } -ret = 0; - cleanup: -return ret; +return 0; } @@ -108,7 +105,6 @@ testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); -int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -137,19 +133,17 @@ testFirewallRemoveRule(const void *opaque G_GNUC_UNUSED) virFirewallRuleAddArgList(fw, fwrule, "--jump", "REJECT", NULL); if (virFirewallApply(fw) < 0) -goto cleanup; +return -1; actual = virBufferCurrentContent(); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); -goto cleanup; +return -1; } -ret = 0; - cleanup: -return ret; +return 0; } @@ -158,7 +152,6 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); -int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -194,19 +187,17 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) -goto cleanup; +return -1; actual = virBufferCurrentContent(); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); -goto cleanup; +return -1; } -ret = 0; - cleanup: -return ret; +return 0; } static void @@ -236,7 +227,6 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); -int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -272,19 +262,17 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) -goto cleanup; +return -1; actual = virBufferCurrentContent(); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); -goto cleanup; +return -1; } -ret = 0; - cleanup: -return ret; +return 0; } @@ -293,7 +281,6 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) fw = virFirewallNew(); -int ret = -1; const char *actual = NULL; const char *expected = IPTABLES " -w -A INPUT --source 192.168.122.1 --jump ACCEPT\n" @@ -328,19 +315,17 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) if (virFirewallApply(fw) < 0) -goto cleanup; +return -1; actual = virBufferCurrentContent(); if (STRNEQ_NULLABLE(expected, actual)) { fprintf(stderr, "Unexpected command execution\n"); virTestDifference(stderr, expected, actual); -goto cleanup; +return -1; } -ret = 0; - cleanup: -return ret; +return 0; } @@ -349,7 +334,6 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) { g_auto(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER; g_autoptr(virFirewall) f
[libvirt PATCH 04/12] tests: remove firewalld backend tests from virfirewalltest.c
When libvirt added support for firewalld, all iptables/ebtables rules were added via the firewalld "passthrough" API when firewalld was enabled (the "firewalld backend"), or run directly by libvirt when firewalld was disabled (the so-called "direct backend"). virfirewalltest.c dutifully ran each test twice, once with the each backend enabled. But commit b19863640d changed the code to *always* directly run iptables/ebtables commands, and never use the firewalld passthrough API, effectively making the direct and firewalld backends identical, except that when libvirt receives notice that firewalld has restarted or reloaded its rules, the firewalld backend sends an extra "iptables -V" command via firewalld's passthrough API (and waits for a response) prior to running all the rest of the iptables commands directly; this assures that a newly-restarted firewalld has finished its work on the filter tables before libvirt starts messing with it. (Because this code is only executed in response to an event from dbus, it isn't tested in the unit tests). In spite of this, we still go through all the virfirewall tests twice though - once for the direct backend, and once for the firewalld backend, even though these take the same codepath. In commit b19863640d I had left this double-testing in thinking that someday we might go back to actually doing something useful with the firewalld backend in the course of adding support for native nftables, but I've now realized that for the case of nftables we will be *even more* divorced from firewalld, so there is really no point in keeping this code around any longer. (It's likely/probable that the tests will be done twice again in the future, but it will be enough different that it is better to remove this code and re-implement from scratch when adding the nftables backend, rather than trying to directly modify the existing code and end up with something even more confusing). This patch eliminates all the test duplication in virfirewalltest.c, including mocking dbus, which is unnecessary since none of the tests use dbus (for now we ensure that by explicitly setting the virfirewall backend to DIRECT before any of the tests have run. Eventually the concept of a "firewalld backend" will disappear completely, but that's for another patch.) Signed-off-by: Laine Stump --- tests/virfirewalltest.c | 293 +++- 1 file changed, 20 insertions(+), 273 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index c6f4ca05e2..e6c41d89fa 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -35,10 +35,6 @@ # define VIR_FROM_THIS VIR_FROM_FIREWALL -static bool fwDisabled = true; -static virBuffer *fwBuf; -static bool fwError; - # define TEST_FILTER_TABLE_LIST \ "Chain INPUT (policy ACCEPT)\n" \ "target prot opt source destination\n" \ @@ -62,124 +58,9 @@ static bool fwError; "Chain POSTROUTING (policy ACCEPT)\n" \ "target prot opt source destination\n" -VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, - GVariant *, - GDBusConnection *, connection, - const gchar *, bus_name, - const gchar *, object_path, - const gchar *, interface_name, - const gchar *, method_name, - GVariant *, parameters, - const GVariantType *, reply_type, - GDBusCallFlags, flags, - gint, timeout_msec, - GCancellable *, cancellable, - GError **, error) -{ -GVariant *reply = NULL; -g_autoptr(GVariant) params = parameters; - -if (params) -g_variant_ref_sink(params); - -VIR_MOCK_REAL_INIT(g_dbus_connection_call_sync); - -if (STREQ(bus_name, "org.freedesktop.DBus") && -STREQ(method_name, "ListNames")) { -GVariantBuilder builder; - -g_variant_builder_init(, G_VARIANT_TYPE("(as)")); -g_variant_builder_open(, G_VARIANT_TYPE("as")); - -g_variant_builder_add(, "s", "org.foo.bar.wizz"); - -if (!fwDisabled) -g_variant_builder_add(, "s", VIR_FIREWALL_FIREWALLD_SERVICE); - -g_variant_builder_close(); - -reply = g_variant_builder_end(); -} else if (STREQ(bus_name, VIR_FIREWALL_FIREWALLD_SERVICE) && - STREQ(method_name, "passthrough")) { -g_autoptr(GVariantIter) iter = NULL; -static const size_t maxargs = 5; -g_auto(GStrv) args = NULL; -size_t nargs = 0; -char *type = NULL; -char *item = NULL; -bool isAdd = false; -bool doError = false; - -g_variant_get(params
[libvirt PATCH 00/12] Clean up cruft in firewall/iptables code (in preparation for nftables)
These patches make no functional change, they just remove a bunch of cruft that accumulated over the years and is no longer needed. This is all in advance of adding support for native nftable support, but there is nothing nftables-specific being added here; I just wanted to get these cleanups out of way now so that the eventual nftables support patchset is smaller and less complicated. (NB: the concept of a "firewall backend" is being removed here, implying that it will no longer exist. This is not true, but the way that it will exist in the future will be different (per-firewall object rather than per-process) so almost all of the existing code won't be applicable anyway.) Laine Stump (12): network: eliminate code that uses default iptables chains util: rename/move iptablesFormatNetwork to virSocketAddrFormatWithPrefix util: rename iptables operators to something less generic tests: remove firewalld backend tests from virfirewalltest.c tests: remove unnecessary ret variables and cleanup labels tests: document why virgdbus must be mocked in networkxml2firewalltest.c util: eliminate pointless switch in virFirewallApplyRule util: simplify virFirewallBackendSynchronize() util: move and rename virFirewallBackendSynchronize() util: remove check for iptables binary during virFirewallInit util: remove currentBackend from virfirewall.c util: remove virFirewallOnceInit() src/libvirt_private.syms | 5 +- src/network/bridge_driver_linux.c | 37 +-- src/util/virfirewall.c| 143 +-- src/util/virfirewall.h| 2 - src/util/virfirewalld.c | 43 src/util/virfirewalld.h | 2 + src/util/virfirewallpriv.h| 37 --- src/util/viriptables.c| 207 +++- src/util/viriptables.h| 2 - src/util/virsocketaddr.c | 44 src/util/virsocketaddr.h | 3 + tests/networkxml2firewalltest.c | 14 +- tests/nwfilterebiptablestest.c| 7 - tests/nwfilterxml2firewalltest.c | 8 +- tests/virfirewalltest.c | 390 -- 15 files changed, 247 insertions(+), 697 deletions(-) delete mode 100644 src/util/virfirewallpriv.h -- 2.33.1
[libvirt PATCH 01/12] network: eliminate code that uses default iptables chains
The network driver has put all its rules into private chains (created by libvirt) since commit 7431b3eb9a, which was included in libvirt-5.1.0. When the conversion was made, code was included that would attempt to delete existing rules in the default chains, to make it possible to upgrade libvirt without restarting the host OS. Almost 3 years has passed, and it is doubtful that anyone will be attempting to upgrade directly from a pre-5.1.0 libvirt to something as new as 8.0.0 (possibly with the exception of upgrading the entire OS to a new release, which would include also rebooting), so it is now safe to remove this code. Signed-off-by: Laine Stump --- src/libvirt_private.syms | 1 - src/network/bridge_driver_linux.c | 37 ++- src/util/viriptables.c| 104 -- src/util/viriptables.h| 2 - 4 files changed, 49 insertions(+), 95 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7be5b51100..ff6f71054e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2501,7 +2501,6 @@ iptablesRemoveTcpInput; iptablesRemoveTcpOutput; iptablesRemoveUdpInput; iptablesRemoveUdpOutput; -iptablesSetDeletePrivate; iptablesSetupPrivateChains; diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index d2eab33e5f..1c8be7103a 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -37,7 +37,7 @@ VIR_LOG_INIT("network.bridge_driver_linux"); static virOnceControl createdOnce; static bool chainInitDone; /* true iff networkSetupPrivateChains was ever called */ -static bool createdChains; /* true iff networkSetupPrivateChains created chains during most recent call */ + static virErrorPtr errInitV4; static virErrorPtr errInitV6; @@ -50,7 +50,6 @@ static void networkSetupPrivateChains(void) VIR_DEBUG("Setting up global firewall chains"); -createdChains = false; virFreeError(errInitV4); errInitV4 = NULL; virFreeError(errInitV6); @@ -63,12 +62,10 @@ static void networkSetupPrivateChains(void) errInitV4 = virSaveLastError(); virResetLastError(); } else { -if (rc) { +if (rc) VIR_DEBUG("Created global IPv4 chains"); -createdChains = true; -} else { +else VIR_DEBUG("Global IPv4 chains already exist"); -} } rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6); @@ -78,12 +75,10 @@ static void networkSetupPrivateChains(void) errInitV6 = virSaveLastError(); virResetLastError(); } else { -if (rc) { +if (rc) VIR_DEBUG("Created global IPv6 chains"); -createdChains = true; -} else { +else VIR_DEBUG("Global IPv6 chains already exist"); -} } chainInitDone = true; @@ -145,7 +140,7 @@ networkHasRunningNetworksWithFW(virNetworkDriverState *driver) void networkPreReloadFirewallRules(virNetworkDriverState *driver, - bool startup, + bool startup G_GNUC_UNUSED, bool force) { /* @@ -183,31 +178,13 @@ networkPreReloadFirewallRules(virNetworkDriverState *driver, } ignore_value(virOnce(, networkSetupPrivateChains)); - -/* - * If this is initial startup, and we just created the - * top level private chains we either - * - * - upgraded from old libvirt - * - freshly booted from clean state - * - * In the first case we must delete the old rules from - * the built-in chains, instead of our new private chains. - * In the second case it doesn't matter, since no existing - * rules will be present. Thus we can safely just tell it - * to always delete from the builin chain - */ -if (startup && createdChains) { -VIR_DEBUG("Requesting cleanup of legacy firewall rules"); -iptablesSetDeletePrivate(false); -} } } void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED) { -iptablesSetDeletePrivate(true); + } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 721e1eeae7..ac949efba7 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -47,8 +47,6 @@ enum { REMOVE }; -static bool deletePrivate = true; - typedef struct { const char *parent; const char *child; @@ -162,17 +160,9 @@ iptablesSetupPrivateChains(virFirewallLayer layer) } -void -iptablesSetDeletePrivate(bool pvt) -{ -deletePrivate = pvt; -} - - static void iptablesInput(virFirewall *fw, virFirewallLayer layer, - bool pvt, const char *iface, int port, int action,
[libvirt PATCH v2 1/2] util: fix erroneous requirement for phys_port_id to get ifname of a VF
Commit 795e9e05c3 (libvirt-7.7.0) refactored the code in virpci.c and virnetdev.c that gathered lists of the Virtual Functions (VF) of an SRIOV Physical Function (PF) to simplify the code. Unfortunately the simplification made the assumption, in the new function virPCIGetVirtualFunctionsFull(), that a VF's netdev interface name should only be retrieved if the PF had a valid phys_port_id. That is an incorrect assumption - only a small handful of (now previous-generation) Mellanox SRIOV cards actually use phys_port_id (this is for an odd design where there are multiple physical network ports on a single PCI address); all other SRIOV cards (including new Mellanox cards) have a file in sysfs called phys_port_id, but it can't be read, and so the pfPhysPortID string is NULL. The result of this logic error is that virtual networks that are a pool of VFs to be used for macvtap connections will be unable to start, giving an errror like this: VF 0 of SRIOV PF enp130s0f0 couldn't be added to the interface pool because it isn't bound to a network driver - possibly in use elsewhere This error message is misinformed - the caller of virNetDevGetVirtualFunctionsFull() only *thinks* that the VF isn't bound to a network driver because it doesn't see a netdev name for the VF in the list. But that's only because virNetDevGetVirtualFunctionsFull() didn't even try to get the names! We do need a way for virPCIGetVirtualFunctionsFull() to sometimes retrieve the netdev names and sometimes not. One way of doing that would be to send down the netdev name of the PF whenever we also want to know the netdev names of the VFs, but send a NULL when we don't. This can conveniently be done by just *replacing* pfPhysPortID in the arglist with pfNetDevName - pfPhysPortID is determined by simply calling virNetDevGetPhysPortID(pfNetDevName) so we can just make that call down in virPCIGetVirtualFunctionsFull() (when needed). This solves the regression introduced by commit 795e9e05c3, and also nicely sets us up to (in a subsequent commit) move the call to virNetDevGetPhysPortID() down one layer further to virPCIGetNetName(), where it really belongs! Resolves: https://bugzilla.redhat.com/2025432 Fixes: 795e9e05c3b6b9ef3abe6f6078a6373a136ec23b Signed-off-by: Laine Stump --- src/util/virnetdev.c | 6 +- src/util/virpci.c| 16 ++-- src/util/virpci.h| 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 58f7360a0f..861b426c58 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1223,15 +1223,11 @@ virNetDevGetVirtualFunctions(const char *pfname, virPCIVirtualFunctionList **vfs) { g_autofree char *pf_sysfs_device_link = NULL; -g_autofree char *pfPhysPortID = NULL; - -if (virNetDevGetPhysPortID(pfname, ) < 0) -return -1; if (virNetDevSysfsFile(_sysfs_device_link, pfname, "device") < 0) return -1; -if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, vfs, pfPhysPortID) < 0) +if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, vfs, pfname) < 0) return -1; return 0; diff --git a/src/util/virpci.c b/src/util/virpci.c index 2d12e28004..f7afcb6e78 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2340,8 +2340,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, * virPCIGetVirtualFunctionsFull: * @sysfs_path: path to physical function sysfs entry * @vfs: filled with the virtual function data - * @pfPhysPortID: Optional physical port id. If provided the network interface - *name of the VFs is queried too. + * @pfNetDevName: Optional netdev name of this PF. If provided, the netdev + *names of the VFs are queried too. * * * Returns virtual functions of a physical function. @@ -2349,7 +2349,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, int virPCIGetVirtualFunctionsFull(const char *sysfs_path, virPCIVirtualFunctionList **vfs, - const char *pfPhysPortID) + const char *pfNetDevName) { g_autofree char *totalvfs_file = NULL; g_autofree char *totalvfs_str = NULL; @@ -2390,8 +2390,12 @@ virPCIGetVirtualFunctionsFull(const char *sysfs_path, return -1; } -if (pfPhysPortID) { -if (virPCIGetNetName(device_link, 0, pfPhysPortID, ) < 0) { +if (pfNetDevName) { +g_autofree char *pfPhysPortID = NULL; + +if (virNetDevGetPhysPortID(pfNetDevName, ) < 0 || +virPCIGetNetName(device_link, 0, pfPhysPortID, ) < 0) { + g_free(fnc.addr); return -1; } @@ -2712,7 +2716,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path G_GNUC_UNUSED, int virPCIGetVirtualFunctionsFull(const char *sysfs_path G_GNUC_UNUSED,
[libvirt PATCH v2 0/2] Fix failure to find VF netdev names during virtual network start
The first patch resolves https://bugzilla.redhat.com/2025432, the 2nd simplifies lower level code in the same manner. V1 is here: https://listman.redhat.com/archives/libvir-list/2021-December/msg0.html Change in V2: Rather than adding an extra bool to the arglist of virPCIGetVirtualFunctionsFull() (what I did in V1), just switch to sending the name of the netdev whose phys_port_id we want to match (called physPortNetDevName) (which will always be non-NULL in the cases where we want) rather than the phys_port_id itself (physPortID) (which may or may not be NULL). This way we don't need an extra arg (we can just check for physPortNetDevName != NULL), and the lower level function can call virNetDevGetPhysPortID() as needed. Also added a similar 2nd patch that pushes the call to virNetDevGetPhysPortID() down even further, into virPCIGetNetName(). This simplifies the callers, and concentrates all calls to virNetDevGetPhysPortID() into a single function (virPCIGetNetName(), duh). Laine Stump (2): util: fix erroneous requirement for phys_port_id to get ifname of a VF util: call virNetDevGetPhysPortID() in less places src/util/virnetdev.c | 24 +++- src/util/virpci.c| 52 ++-- src/util/virpci.h| 4 ++-- 3 files changed, 36 insertions(+), 44 deletions(-) -- 2.33.1
[libvirt PATCH v2 2/2] util: call virNetDevGetPhysPortID() in less places
Whenever virPCIGetNetName() is called, it is either called with physPortID = NULL, or with it set by the caller calling virNetDevGetPhysPortID() soon before virPCIGetNetName(). The physPortID is then used *only* in virPCIGetNetName(). Rather than replicating that same call to virNetDevGetPhysPortID() in all the callers of virPCIGetNetName(), lets just have all those callers send the NetDevName whose physPortID they want down to virPCIGetNetName(), and let virPCIGetNetName() call virNetDevGetPhysPortID(). Signed-off-by: Laine Stump --- src/util/virnetdev.c | 18 ++--- src/util/virpci.c| 48 +--- src/util/virpci.h| 2 +- 3 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 861b426c58..d93b2c6a83 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1297,18 +1297,12 @@ int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) { g_autofree char *physfn_sysfs_path = NULL; -g_autofree char *vfPhysPortID = NULL; - -if (virNetDevGetPhysPortID(ifname, ) < 0) -return -1; if (virNetDevSysfsDeviceFile(_sysfs_path, ifname, "physfn") < 0) return -1; -if (virPCIGetNetName(physfn_sysfs_path, 0, - vfPhysPortID, pfname) < 0) { +if (virPCIGetNetName(physfn_sysfs_path, 0, ifname, pfname) < 0) return -1; -} return 0; } @@ -1336,14 +1330,6 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname) { g_autofree char *virtfnName = NULL; g_autofree char *virtfnSysfsPath = NULL; -g_autofree char *pfPhysPortID = NULL; - -/* a VF may have multiple "ports", each one having its own netdev, - * and each netdev having a different phys_port_id. Be sure we get - * the VF netdev with a phys_port_id matchine that of pfname - */ -if (virNetDevGetPhysPortID(pfname, ) < 0) -return -1; virtfnName = g_strdup_printf("virtfn%d", vf); @@ -1360,7 +1346,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname) * isn't bound to a netdev driver, it won't have a netdev name, * and vfname will be NULL). */ -return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname); +return virPCIGetNetName(virtfnSysfsPath, 0, pfname, vfname); } diff --git a/src/util/virpci.c b/src/util/virpci.c index f7afcb6e78..0d476cd8b4 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2390,15 +2390,10 @@ virPCIGetVirtualFunctionsFull(const char *sysfs_path, return -1; } -if (pfNetDevName) { -g_autofree char *pfPhysPortID = NULL; - -if (virNetDevGetPhysPortID(pfNetDevName, ) < 0 || -virPCIGetNetName(device_link, 0, pfPhysPortID, ) < 0) { - -g_free(fnc.addr); -return -1; -} +if (pfNetDevName && +virPCIGetNetName(device_link, 0, pfNetDevName, ) < 0) { +g_free(fnc.addr); +return -1; } VIR_APPEND_ELEMENT(list->functions, list->nfunctions, fnc); @@ -2474,8 +2469,20 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddress *addr, * @device_link_sysfs_path: sysfs path to the PCI device * @idx: used to choose which netdev when there are several * (ignored if physPortID is set or physPortName is available) - * @physPortID: match this string in the netdev's phys_port_id - * (or NULL to ignore and use phys_port_name or idx instead) + + * @physPortNetDevName: if non-null, attempt to learn the phys_port_id + * of the netdev interface named + * @physPortNetDevName, and find a netdev for + * this PCI device that has the same + * phys_port_id. if @physPortNetDevName is NULL, + * or has no phys_port_id, then use + * phys_port_name or idx to determine which + * netdev to return. (NB: as of today, only mlx + * drivers/cards can have multiple phys_ports for + * a single PCI device; on all other devices + * there is only a single choice of netdev, and + * phys_port_id, phys_port_name, and idx are + * unavailable/unused) * @netname: used to return the name of the netdev * (set to NULL (but returns success) if there is no netdev) * @@ -2484,9 +2491,10 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddress *addr, int virPCIGetNetName(const char *device_link_sysfs_path, size_t idx, - const char *physPortID, + const char *physPortNetDevName, char **netname) { +g_autofree char *physPortID = NULL; g_autofree char *pcidev_sysfs_net_path = NULL; g_autofree cha
[PATCH] util: fix erroneous requirement for phys_port_id to get ifname of a VF
Commit 795e9e05c3 (libvirt-7.7.0) refactored the code in virpci.c and virnetdev.c that gathered lists of the Virtual Functions (VF) of an SRIOV Physical Function (PF) to simplify the code. Unfortunately the simplification made the assumption that a VF's netdev interface name should only be retrieved if the PF had a valid phys_port_id. That is an incorrect assumption - only a small handful of (now previous-generation) Mellanox SRIOV cards actually use phys_port_id (this is for an odd design where there are multiple physical network ports on a single PCI address); all other SRIOV cards (including new Mellanox cards) have a file in sysfs called phys_port_id, but it can't be read, and so the pfPhysPortID string is NULL. The result of this logic error is that virtual networks that are a pool of VFs to be used for macvtap connections will be unable to start, giving an errror like this: VF 0 of SRIOV PF enp130s0f0 couldn't be added to the interface pool because it isn't bound to a network driver - possibly in use elsewhere The simple/quick solution to this is to not imply that "pfPhysPortID == NULL" is the same as "don't fill in the VF's netdev ifname". Instead, add a bool getIfName to the args of virNetDevGetVirtualFunctionsFull() so that we can still get the ifname filled in when pfPhysPortID is NULL. Resolves: https://bugzilla.redhat.com/2025432 Fixes: 795e9e05c3b6b9ef3abe6f6078a6373a136ec23b Signed-off-by: Laine Stump --- On one hand this is a regression with an apparently simple fix (that has been tested to solve the problem), and it's always good to fix regressions before a release rather than after. On the other hand it has been broken since August, and nobody complained until last week (and that was a QE tester, not an actual end-user), so it seems the bug is in functionality that isn't used much in the field (or at least no downstream with a used of the functionality has made a release since then that was installed by said user). I've grown increasingly wary of pushing anything just before a release, especially when it modifies the args of a function that has multiple definitions for different platforms (although CI is supposed to be thorough enough to catch those types of problems these days). So I'm all for pushing this right after the release, rather than before. But if anyone sees a reason for doing otherwise, we can talk about it in about 10 hours when I sit down at the keyboard again :-). P.S. I'm planning to make a followup that eliminates the pfPhysPortID arg completely, but wanted the bugfix to be as stripped down as possible. src/util/virnetdev.c | 2 +- src/util/virpci.c| 20 src/util/virpci.h| 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 58f7360a0f..300d7e4f45 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1231,7 +1231,7 @@ virNetDevGetVirtualFunctions(const char *pfname, if (virNetDevSysfsFile(_sysfs_device_link, pfname, "device") < 0) return -1; -if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, vfs, pfPhysPortID) < 0) +if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, true, vfs, pfPhysPortID) < 0) return -1; return 0; diff --git a/src/util/virpci.c b/src/util/virpci.c index 2d12e28004..b7b987dd63 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2263,7 +2263,7 @@ int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIVirtualFunctionList **vfs) { -return virPCIGetVirtualFunctionsFull(sysfs_path, vfs, NULL); +return virPCIGetVirtualFunctionsFull(sysfs_path, false, vfs, NULL); } @@ -2339,15 +2339,18 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, /** * virPCIGetVirtualFunctionsFull: * @sysfs_path: path to physical function sysfs entry + * @getIfName: true if the ifname of the VF should also be filled in, + * false to only fill in the PCI address. * @vfs: filled with the virtual function data - * @pfPhysPortID: Optional physical port id. If provided the network interface - *name of the VFs is queried too. + * @pfPhysPortID: Optional physical port id. if non-null it will be used + *when determining the ifname. * * * Returns virtual functions of a physical function. */ int virPCIGetVirtualFunctionsFull(const char *sysfs_path, + bool getIfName, virPCIVirtualFunctionList **vfs, const char *pfPhysPortID) { @@ -2390,11 +2393,10 @@ virPCIGetVirtualFunctionsFull(const char *sysfs_path, return -1; } -if (pfPhysPortID) { -if (virPCIGetNetName(device_link, 0, pfPhysPortID, ) < 0) { -g_free(fnc.addr); -return -1; -} +if (getIfName && +vi
Re: [libvirt PATCH v3 1/1] Ignore EPERM on attempts to clear VF VLAN ID
On 11/15/21 1:59 PM, Dmitrii Shcherbakov wrote: [...] diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c index aadbeb1ef4..bdaa94e83c 100644 --- a/tests/virnetdevtest.c +++ b/tests/virnetdevtest.c Maybe I'm just looking at it too superficially, but it seems like these test cases are testing the test jig itself more than any of the library code (the one exception is that testVirNetDevSetVfConfig() checks that the real virNetDevSetVfConfig() returns success when setting the vlan failed after setting the MAC succeeded). I'm not saying that anything better could be accomplished without mocking netlink itself, just wonder if it's worth all this extra bulk for the small amount of testing of actual library code that's accomplished. @@ -18,11 +18,17 @@ #include +#include "internal.h" #include "testutils.h" +#include "virnetlink.h" + +#define LIBVIRT_VIRNETDEVPRIV_H_ALLOW + #ifdef __linux__ -# include "virnetdev.h" +# include "virmock.h" +# include "virnetdevpriv.h" # define VIR_FROM_THIS VIR_FROM_NONE @@ -59,6 +65,211 @@ testVirNetDevGetLinkInfo(const void *opaque) return 0; } +int +(*real_virNetDevSendVfSetLinkRequest)(const char *ifname, int vfInfoType, + const void *payload, const size_t payloadLen); + +int +(*real_virNetDevSetVfMac)(const char *ifname, int vf, const virMacAddr *macaddr, bool *allowRetry); + +int +(*real_virNetDevSetVfVlan)(const char *ifname, int vf, int vlanid); + +static void +init_syms(void) +{ +VIR_MOCK_REAL_INIT(virNetDevSendVfSetLinkRequest); +VIR_MOCK_REAL_INIT(virNetDevSetVfMac); +VIR_MOCK_REAL_INIT(virNetDevSetVfVlan); +} + +int +virNetDevSetVfMac(const char *ifname, int vf, + const virMacAddr *macaddr, + bool *allowRetry) +{ +init_syms(); + +if (STREQ_NULLABLE(ifname, "fakeiface-macerror")) { +return -1; +} else if (STREQ_NULLABLE(ifname, "fakeiface-altmacerror")) { +return -2; +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) { +return -1; +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) { +return -1; +} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) { +return 0; +} +return real_virNetDevSetVfMac(ifname, vf, macaddr, allowRetry); +} + +int +virNetDevSetVfVlan(const char *ifname, int vf, int vlanid) +{ +init_syms(); + +if (STREQ_NULLABLE(ifname, "fakeiface-macerror-vlanerror")) { +return -1; +} else if (STREQ_NULLABLE(ifname, "fakeiface-macerror-novlanerror")) { +return 0; +} else if (STREQ_NULLABLE(ifname, "fakeiface-nomacerror-novlanerror")) { +return 0; +} +return real_virNetDevSetVfVlan(ifname, vf, vlanid); +} + +int +virNetDevSendVfSetLinkRequest(const char *ifname, int vfInfoType, + const void *payload, const size_t payloadLen) +{ +init_syms(); + +if (STREQ_NULLABLE(ifname, "fakeiface-eperm")) { +return -EPERM; +} else if (STREQ_NULLABLE(ifname, "fakeiface-eagain")) { +return -EAGAIN; +} else if (STREQ_NULLABLE(ifname, "fakeiface-einval")) { +return -EINVAL; +} else if (STREQ_NULLABLE(ifname, "fakeiface-ok")) { +return 0; +} +return real_virNetDevSendVfSetLinkRequest(ifname, vfInfoType, payload, payloadLen); +} + +static int +testVirNetDevSetVfMac(const void *opaque G_GNUC_UNUSED) +{ +struct testCase { +const char *ifname; +const int vf_num; +const virMacAddr macaddr; +bool allow_retry; +const int rc; +}; +size_t i = 0; +int rc = 0; +struct testCase testCases[] = { +{ .ifname = "fakeiface-ok", .vf_num = 1, + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = 0 }, +{ .ifname = "fakeiface-ok", .vf_num = 2, + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = 0 }, +{ .ifname = "fakeiface-ok", .vf_num = 3, + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = 0 }, +{ .ifname = "fakeiface-ok", .vf_num = 4, + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = true, .rc = 0 }, +{ .ifname = "fakeiface-eperm", .vf_num = 5, + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -1 }, +{ .ifname = "fakeiface-einval", .vf_num = 6, + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = false, .rc = -1 }, +{ .ifname = "fakeiface-einval", .vf_num = 7, + .macaddr = { .addr = { 0, 0, 0, 0, 0, 0 } }, .allow_retry = true, .rc = -2 }, +{ .ifname = "fakeiface-einval", .vf_num = 8, + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry = false, .rc = -1 }, +{ .ifname = "fakeiface-einval", .vf_num = 9, + .macaddr = { .addr = { 0, 0, 0, 7, 7, 7 } }, .allow_retry =
Re: [PATCH 1/1] bridge_driver: use ovs-vsctl to setup and clean Qos when
On 11/8/21 5:18 AM, Michal Prívozník wrote: On 11/3/21 4:11 AM, jx8zjs wrote: From: zhangjl02 Fix bug 1826168: bridge type network with ovs bridge can start with Qos setting which do not take any effect Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1826168 Signed-off-by: zhangjl02 --- src/network/bridge_driver.c | 65 +++-- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 498c45d0a7..d0627848cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2305,6 +2305,15 @@ networkAddRouteToBridge(virNetworkObj *obj, } +static int +networkDefIsOvsBridge(virNetworkDef *def) This @def should have been const too. And function returns a bool, effectively. +{ +const virNetDevVPortProfile *vport = def->virtPortProfile; +return vport && +vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; +} + + static int networkStartNetworkVirtual(virNetworkDriverState *driver, virNetworkObj *obj) @@ -2320,6 +2329,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, bool dnsmasqStarted = false; bool devOnline = false; bool firewalRulesAdded = false; +bool ovsType = networkDefIsOvsBridge(def); /* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(def) < 0) @@ -2439,15 +2449,29 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (v6present && networkStartRadvd(driver, obj) < 0) goto error; -if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) -goto error; +if (ovsType) { +if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, +def->uuid, +true) < 0) +goto error; +} else { +if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) +goto error; +} I don't think this should be here. At this point, the bridge was created using netlink (definitely NOT ovs-vsctl add-br). Therefore, virNetDevOpenvswitchInterfaceSetQos() must fail because it won't be able to find any ports/queues/... Beyond that, networks of the type that would end up calling networkStartNetworkVirtual (forward modes 'route' and "nat') don't support using OVS bridges anyway, so this code in networkStartNetworkVirtual and networkShutdownNetworkVirtual will never be executed anyway. return 0; error: virErrorPreserveLast(_err); -if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); +if (ovsType) { +if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) +VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); +} else { +if (def->bandwidth) { +virNetDevBandwidthClear(def->bridge); +} +} Similarly, this shouldn't be here either. if (dnsmasqStarted) { pid_t dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); @@ -2536,13 +2560,21 @@ static int networkStartNetworkBridge(virNetworkObj *obj) { virNetworkDef *def = virNetworkObjGetDef(obj); +bool ovsType = networkDefIsOvsBridge(def); /* put anything here that needs to be done each time a network of * type BRIDGE, is started. On failure, undo anything you've done, * and return -1. On success return 0. */ -if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) -goto error; +if (ovsType) { +if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, +def->uuid, +true) < 0) +goto error; +} else { +if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) +goto error; +} if (networkStartHandleMACTableManagerMode(obj) < 0) goto error; @@ -2550,8 +2582,15 @@ networkStartNetworkBridge(virNetworkObj *obj) return 0; error: -if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); +if (ovsType) { +if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) +VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); +} else { +if (def->bandwidth) { +virNetDevBandwidthClear(def->bridge); +} +} return -1; } @@ -2565,9 +2604,15 @@ networkShutdownNetworkBridge(virNetworkObj *obj G_GNUC_UNUSED) * type BRIDGE is shutdown. On failure, undo anything you've done, * and return -1. On success return 0. */ -if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); +if
Re: [libvirt PATCH] node_device: Fix memory leak in udevProcessMediatedDevice
On 10/25/21 11:32 AM, Jiri Denemark wrote: One of the paths returned -1 directly without going through the cleanup section. Signed-off-by: Jiri Denemark Reviewed-by: Laine Stump
Re: [PATCH] cosmetic: remove unused function return value
On 10/24/21 1:40 AM, Ani Sinha wrote: qemuBuildPMPCIRootHotplugCommandLine() returns 0 unconditionally. There is no failure scenario at present. So clean up the code by removing integer return from the function and also remove the failure check conditional from the function call. Also fix indentation for the above function call while at it. Signed-off-by: Ani Sinha Reviewed-by: Laine Stump and pushed. --- src/qemu/qemu_command.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6df50ec73..093d8ae62c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3177,9 +3177,9 @@ qemuBuildSkipController(const virDomainControllerDef *controller, return false; } -static int +static void qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd, - const virDomainControllerDef *controller) + const virDomainControllerDef *controller) { if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && @@ -3189,7 +3189,7 @@ qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd, virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s", virTristateSwitchTypeToString(controller->opts.pciopts.hotplug)); } -return 0; +return; } static int @@ -3207,8 +3207,7 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, if (cont->type != type) continue; -if (qemuBuildPMPCIRootHotplugCommandLine(cmd, cont)) -continue; +qemuBuildPMPCIRootHotplugCommandLine(cmd, cont); if (qemuBuildSkipController(cont, def)) continue;
Re: [PATCH] i440fx/hotplug: Fix error message format to conform to spec
On 10/20/21 11:30 PM, Ani Sinha wrote: Error messages must conform to spec as specified here: https://www.libvirt.org/coding-style.html#error-message-format This change makes some error messages conform to the spec above. Fixes: 8eadf82fb5 ("conf: introduce option to enable/disable pci hotplug on pci-root controller") Signed-off-by: Ani Sinha Reviewed-by: Laine Stump and pushed. --- src/qemu/qemu_validate.c| 6 +++--- .../pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err| 2 +- .../pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 3045e4b64b..f93b697265 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3947,7 +3947,7 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("setting the %s property on a '%s' device is not supported by this QEMU binary"), + _("setting the '%s' property on a '%s' device is not supported by this QEMU binary"), "hotplug", "pci-root"); return -1; } @@ -3956,8 +3956,8 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("setting the hotplug property on a '%s' device is not supported by this QEMU binary"), - modelName); + _("setting the '%s' property on a '%s' device is not supported by this QEMU binary"), + "hotplug", modelName); return -1; } break; diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err index b507f1f8bc..55ec41c476 100644 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err @@ -1 +1 @@ -unsupported configuration: setting the hotplug property on a 'pci-root' device is not supported by this QEMU binary +unsupported configuration: setting the 'hotplug' property on a 'pci-root' device is not supported by this QEMU binary diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err index b507f1f8bc..55ec41c476 100644 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err @@ -1 +1 @@ -unsupported configuration: setting the hotplug property on a 'pci-root' device is not supported by this QEMU binary +unsupported configuration: setting the 'hotplug' property on a 'pci-root' device is not supported by this QEMU binary
Re: [libvirt PATCH 02/10] Revert "qemuxml2xmltest: Convert all acpi-hotplug control related tests to DO_TEST_CAPS_LATEST"
On 10/21/21 1:06 PM, Ján Tomko wrote: On a Thursday in 2021, Laine Stump wrote: This reverts commit da896d440c7267e0b4575e4a3f2780bebf3fbfca. Signed-off-by: Laine Stump --- ...i-hotplug-bridge-disable.x86_64-latest.xml | 36 - .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 1 + ...pi-hotplug-bridge-enable.x86_64-latest.xml | 36 - .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 1 + ...cpi-root-hotplug-disable.x86_64-latest.xml | 33 .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + ...acpi-root-hotplug-enable.x86_64-latest.xml | 33 .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + ...i-hotplug-bridge-disable.x86_64-latest.xml | 53 --- .../q35-acpi-hotplug-bridge-disable.xml | 1 + ...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 --- .../q35-acpi-hotplug-bridge-enable.xml | 1 + tests/qemuxml2xmltest.c | 24 ++--- 13 files changed, 24 insertions(+), 250 deletions(-) delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml delete mode 100644 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b0a1212a54..d6effbdac6 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -424,12 +424,24 @@ mymain(void) DO_TEST_NOCAPS("input-usbtablet"); DO_TEST_NOCAPS("misc-acpi"); DO_TEST("misc-disable-s3", QEMU_CAPS_PIIX_DISABLE_S3); - DO_TEST_CAPS_LATEST("pc-i440fx-acpi-root-hotplug-disable"); - DO_TEST_CAPS_LATEST("pc-i440fx-acpi-root-hotplug-enable"); - DO_TEST_CAPS_LATEST("pc-i440fx-acpi-hotplug-bridge-disable"); - DO_TEST_CAPS_LATEST("pc-i440fx-acpi-hotplug-bridge-enable"); - DO_TEST_CAPS_LATEST("q35-acpi-hotplug-bridge-disable"); - DO_TEST_CAPS_LATEST("q35-acpi-hotplug-bridge-enable"); + DO_TEST("pc-i440fx-acpi-root-hotplug-disable", + QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG); + DO_TEST("pc-i440fx-acpi-root-hotplug-enable", + QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG); Please only do a partial revert here and leave the i440fx root hotplug tests using the _LATEST form, since you're only reverting bridge hotplug tests by the end of the series. Huh, I thought I caught all of those (maybe I "re-un-re-reverted" it in another patch?). Thanks for catching it! I fixed it, but I'm waiting until Monday to push these. (The only conflict will be in context in the second-to-last patch. Well, and the NEWS addition, because I pushed a patch there in the meantime) Jano + DO_TEST("pc-i440fx-acpi-hotplug-bridge-disable", + QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE); + DO_TEST("pc-i440fx-acpi-hotplug-bridge-enable", + QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE);
[libvirt PATCH 10/10] Revert "qemu: capablities: detect acpi-pci-hotplug-with-bridge-support"
This reverts commit 58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5. Conflict: * src/qemu/qemu_capabilities.[ch] Because other new cap flags had been added since the original commit, reformatting was necessary to follow the "groups of five" pattern. * tests.qemucapabilitiesdata/caps_6.2.0.x86_64.xml This file was added after the original commit that we are reverting, so had to be manually edited to remove the two capabilities. Signed-off-by: Laine Stump --- src/qemu/qemu_capabilities.c | 8 ++-- src/qemu/qemu_capabilities.h | 6 ++ tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 - tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 2 -- tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 2 -- 15 files changed, 4 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c5ebf5d156..96ecf27032 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -644,12 +644,10 @@ VIR_ENUM_IMPL(virQEMUCaps, "virtio-mem-pci", /* QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI */ "memory-backend-file.reserve", /* QEMU_CAPS_MEMORY_BACKEND_RESERVE */ "piix4.acpi-root-pci-hotplug", /* QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */ - "piix4.acpi-hotplug-bridge", /* QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE */ - "ich9.acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ - - /* 415 */ "netdev.json", /* QEMU_CAPS_NETDEV_JSON */ "chardev.json", /* QEMU_CAPS_CHARDEV_JSON */ + + /* 415 */ "device.json", /* QEMU_CAPS_DEVICE_JSON */ "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */ ); @@ -1470,7 +1468,6 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, { "acpi-root-pci-hotplug", QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, NULL }, -{ "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { @@ -1521,7 +1518,6 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioGpu[] = { static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = { { "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL }, -{ "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ac8a94a0af..796714438e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -624,12 +624,10 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */ QEMU_CAPS_MEMORY_BACKEND_RESERVE, /* -object memory-backend-*.reserve= */ QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, /* -M pc PIIX4_PM.acpi-root-pci-hotplug */ -QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, /* -M pc PIIX4_PM.acpi-pci-hotplug-with-bridge-support */ -QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ - -/* 415 */ QEMU_CAPS_NETDEV_JSON, /* -netdev accepts JSON */ QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */ + +/* 415 */ QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */ QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */ diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml index 6544b78730..559bf16766 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml @@ -167,7 +167,6 @@ - 2011000 0 43100288 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index c66a140f8d..745110142f 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/t
[libvirt PATCH 08/10] Revert "qemu: command: add support for acpi-bridge-hotplug feature"
This reverts commit bef0f0d8be6baa1d9359be208b53d6b8a37ddc95. Conflicts: tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args * this file had been renamed from its original, then renamed back, which understandably confused git. It's being completely removed here anyway, so the contents don't matter. tests/qemuxml2argvtest.c * change in context around removed chunk Signed-off-by: Laine Stump --- src/qemu/qemu_command.c | 20 --- .../aarch64-acpi-hotplug-bridge-disable.err | 1 - ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 - .../pc-i440fx-acpi-hotplug-bridge-disable.err | 1 - .../q35-acpi-hotplug-bridge-disable.args | 33 --- .../q35-acpi-hotplug-bridge-disable.err | 1 - tests/qemuxml2argvtest.c | 16 - 7 files changed, 103 deletions(-) delete mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 989ddcadb8..55d26d9af6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6415,7 +6415,6 @@ qemuBuildPMCommandLine(virCommand *cmd, qemuDomainObjPrivate *priv) { virQEMUCaps *qemuCaps = priv->qemuCaps; -int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG]; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { /* with new qemu we always want '-no-shutdown' on startup and we set @@ -6461,25 +6460,6 @@ qemuBuildPMCommandLine(virCommand *cmd, pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); } -if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) { -const char *pm_object = NULL; - -if (!qemuDomainIsQ35(def) && -virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) -pm_object = "PIIX4_PM"; - -if (qemuDomainIsQ35(def) && -virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) -pm_object = "ICH9-LPC"; - -if (pm_object != NULL) { -virCommandAddArg(cmd, "-global"); -virCommandAddArgFormat(cmd, "%s.acpi-pci-hotplug-with-bridge-support=%s", - pm_object, - virTristateSwitchTypeToString(acpihp_br)); -} -} - return 0; } diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err deleted file mode 100644 index 9f0a88b826..00 --- a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err +++ /dev/null @@ -1 +0,0 @@ -unsupported configuration: acpi-bridge-hotplug is not available for architecture 'aarch64' diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args deleted file mode 100644 index a1e5dc85c7..00 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args +++ /dev/null @@ -1,31 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-i440fx \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ -XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ -XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-x86_64 \ --name guest=i440fx,debug-threads=on \ --S \ --object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \ --machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \ --m 1024 \ --realtime mlock=off \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid 56f5055c-1b8d-490c-844a-ad646a1c \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --no-acpi \ --global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \ --boot strict=on \ --usb \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ --msg timestamp=on diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err deleted file mode 100644 index 8c09a3cd76..00 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err +++ /dev/null @@ -1 +0,0 @@ -unsupported configuration: acpi-bridge-hotplug is not available with this QEMU binary diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args b/tests/qem
[libvirt PATCH 09/10] Revert "conf: introduce support for acpi-bridge-hotplug feature"
This reverts commit 7300ccc9b3eddb38306868534e7fc2d505a0a13c. Signed-off-by: Laine Stump --- docs/formatdomain.rst | 29 -- docs/schemas/domaincommon.rng | 15 src/conf/domain_conf.c| 89 +-- src/conf/domain_conf.h| 9 -- src/qemu/qemu_validate.c | 46 -- .../aarch64-acpi-hotplug-bridge-disable.xml | 33 --- .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 33 --- .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 33 --- .../q35-acpi-hotplug-bridge-disable.xml | 47 -- .../q35-acpi-hotplug-bridge-enable.xml| 47 -- .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 1 - .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 1 - .../q35-acpi-hotplug-bridge-disable.xml | 1 - .../q35-acpi-hotplug-bridge-enable.xml| 1 - tests/qemuxml2xmltest.c | 14 --- 15 files changed, 1 insertion(+), 398 deletions(-) delete mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml delete mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml delete mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml delete mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml delete mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 9bf59936e5..58768f7e5e 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1847,9 +1847,6 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. - - - @@ -1945,32 +1942,6 @@ are: passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode - optional string sync_pt or share_pt :since:`6.3.0` === == === == -``pci`` - Various PCI bus related features of the hypervisor. - - = === == - Feature Description Value Since - = === == - acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`7.9.0` - = === == - - Note: pc machine types (i440fx) have ACPI hotplug enabled by - default on cold plugged bridges (bridges that were present in the - VM's domain definition before it was started). Disabling ACPI - hotplug leaves only SHPC hotplug enabled; many OSes don't - support SHPC hotplug, so this may have the effect of completely - disabling hotplug. - - On q35 machinetypes earlier than pc-q35-6.1 (regardless of the QEMU - binary version), ACPI hotplug for cold plugged bridges is disabled - by default, and native PCIe hotplug is enabled instead. Enabling - ACPI hotplug will disable native PCIe hotplug. - - Starting with the pc-q35-6.1 machinetype, ACPI hotplug is enabled - on cold plugged bridges by default while native PCIe hotplug is - disabled. In this case, disabling ACPI hotplug will re-enable PCIe - native hotplug. - ``pmu`` Depending on the ``state`` attribute (values ``on``, ``off``, default ``on``) enable or disable the performance monitoring unit for the guest. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 26990c4d6d..f71e375a33 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6169,9 +6169,6 @@ - - - @@ -6403,18 +6400,6 @@ - - - - - - - - - - - - diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15228d1e38..2c794626f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -172,7 +172,6 @@ VIR_ENUM_IMPL(virDomainFeature, "cfpc", "sbbc",
[libvirt PATCH 06/10] Revert "qemuValidateDomainDefPCIFeature: un-break error messages"
This reverts commit 7d074c56830c5d435f87667299cc102650dbbb4f. Signed-off-by: Laine Stump --- src/qemu/qemu_validate.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index d3b9691db5..0cb4542efd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -194,7 +194,8 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def, case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG: if (!ARCH_IS_X86(def->os.arch)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("acpi-bridge-hotplug is not available for architecture '%s'"), + _("acpi-bridge-hotplug is not available " + "for architecture '%s'"), virArchToString(def->os.arch)); return -1; } @@ -202,7 +203,8 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def, !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("acpi-bridge-hotplug is not available with this QEMU binary")); + _("acpi-bridge-hotplug is not available " + "with this QEMU binary")); return -1; } break; -- 2.31.1
[libvirt PATCH 07/10] Revert "NEWS: document new acpi pci hotplug config option"
This reverts commit 5ee4f3e1d4f173f7e1b64b745ab9ef5dc8c8f393. Signed-off-by: Laine Stump --- NEWS.rst | 8 1 file changed, 8 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index f3b9e5f0cb..bbed7a8228 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -32,14 +32,6 @@ v7.9.0 (unreleased) controller. The default behavior is unchanged (hotplug is allowed). - * Add a new global feature for controlling ACPI PCI hotplug on bridges - -A new config option under -sub-element have been added to allow users enable/disable ACPI based PCI -hotplug for cold plugged bridges (that is, bridges that were present in the -domain definition when the guest was first started).This setting is only -applicable for x86 guests using qemu driver. - * **Improvements** * Use of JSON syntax with ``-device`` with upcoming QEMU-6.2 -- 2.31.1
[libvirt PATCH 03/10] Revert "qemuxml2argvtest: Add '-enable' variants for ACPI-hotplug related cases"
This reverts commit 64146031058906804b3c5f519570fadc9ff4df48. Conflicts: tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.args tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args These files are unrelated to the functionality we need to remove, so they weren't removed, and the associated test cases weren't removed from qemuxml2argvtest.c Signed-off-by: Laine Stump --- ...i-hotplug-bridge-enable.x86_64-latest.args | 34 - ...cpi-hotplug-bridge-enable.x86_64-6.0.0.err | 1 - ...i-hotplug-bridge-enable.x86_64-latest.args | 37 --- tests/qemuxml2argvtest.c | 3 -- 4 files changed, 75 deletions(-) delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args deleted file mode 100644 index 77ef82f25b..00 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args +++ /dev/null @@ -1,34 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-i440fx \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ -XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ -XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ -/usr/bin/qemu-system-x86_64 \ --name guest=i440fx,debug-threads=on \ --S \ --object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}' \ --machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \ --cpu qemu64 \ --m 1024 \ --object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \ --overcommit mem-lock=off \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid 56f5055c-1b8d-490c-844a-ad646a1c \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --no-acpi \ --global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on \ --boot strict=on \ --device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ --audiodev id=audio1,driver=none \ --device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ --sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ --msg timestamp=on diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err deleted file mode 100644 index 8c09a3cd76..00 --- a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err +++ /dev/null @@ -1 +0,0 @@ -unsupported configuration: acpi-bridge-hotplug is not available with this QEMU binary diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args deleted file mode 100644 index 4ff07ad3b8..00 --- a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args +++ /dev/null @@ -1,37 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-q35 \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-q35/.local/share \ -XDG_CACHE_HOME=/tmp/lib/domain--1-q35/.cache \ -XDG_CONFIG_HOME=/tmp/lib/domain--1-q35/.config \ -/usr/bin/qemu-system-x86_64 \ --name guest=q35,debug-threads=on \ --S \ --object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-q35/master-key.aes"}' \ --machine pc-q35-2.5,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \ --cpu qemu64 \ --m 1024 \ --object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \ --overcommit mem-lock=off \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid 56f5055c-1b8d-490c-844a-ad646a1c \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --no-acpi \ --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=on \ --boot strict=on \ --device '{"driver":"i82801b11-bri
[libvirt PATCH 04/10] Revert "qemuxml2argvtest: Use real-caps testing for 'acpi-hotplug-bridge-disable'"
This reverts commit 2d20f0bb05175d18513220bccd1dbaa207cc4c6a. Conflicts: tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args the test output of these files was regenerated because the tests were changed upstream to use JSON on the commandline at a later commit than the commit being reverted here (where they were changed to use latest caps, but the patches to use JSON on the commandline hadn't been committed yet). Signed-off-by: Laine Stump --- ...> aarch64-acpi-hotplug-bridge-disable.err} | 0 .../aarch64-acpi-hotplug-bridge-disable.xml | 22 ++- ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 ...pc-i440fx-acpi-hotplug-bridge-disable.err} | 0 ...-hotplug-bridge-disable.x86_64-latest.args | 34 - .../q35-acpi-hotplug-bridge-disable.args | 33 + .../q35-acpi-hotplug-bridge-disable.err | 1 + ...-hotplug-bridge-disable.x86_64-latest.args | 37 --- tests/qemuxml2argvtest.c | 17 +++-- 9 files changed, 99 insertions(+), 76 deletions(-) rename tests/qemuxml2argvdata/{aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err => aarch64-acpi-hotplug-bridge-disable.err} (100%) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args rename tests/qemuxml2argvdata/{q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err => pc-i440fx-acpi-hotplug-bridge-disable.err} (100%) delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err similarity index 100% rename from tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err rename to tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml index 47107e93f3..0d5f945bd7 100644 --- a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml +++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml @@ -1,13 +1,33 @@ - test + i440fx + 56f5055c-1b8d-490c-844a-ad646a1c 1048576 + 1048576 1 hvm + + + destroy + restart + destroy + +/usr/bin/qemu-system-aarch64 + + + + + + + + + + + diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args new file mode 100644 index 00..a1e5dc85c7 --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-i440fx \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=i440fx,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \ +-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \ +-m 1024 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 56f5055c-1b8d-490c-844a-ad646a1c \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \ +-boot strict=on \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err similarity index 100% rename from tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err rename to tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args deleted file mode 100644 index 8a3e91c95e..00 --- a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args +++ /dev/null @@ -1,34 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-i440fx \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.lo
[libvirt PATCH 02/10] Revert "qemuxml2xmltest: Convert all acpi-hotplug control related tests to DO_TEST_CAPS_LATEST"
This reverts commit da896d440c7267e0b4575e4a3f2780bebf3fbfca. Signed-off-by: Laine Stump --- ...i-hotplug-bridge-disable.x86_64-latest.xml | 36 - .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 1 + ...pi-hotplug-bridge-enable.x86_64-latest.xml | 36 - .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 1 + ...cpi-root-hotplug-disable.x86_64-latest.xml | 33 .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + ...acpi-root-hotplug-enable.x86_64-latest.xml | 33 .../pc-i440fx-acpi-root-hotplug-enable.xml| 1 + ...i-hotplug-bridge-disable.x86_64-latest.xml | 53 --- .../q35-acpi-hotplug-bridge-disable.xml | 1 + ...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 --- .../q35-acpi-hotplug-bridge-enable.xml| 1 + tests/qemuxml2xmltest.c | 24 ++--- 13 files changed, 24 insertions(+), 250 deletions(-) delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml delete mode 100644 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml deleted file mode 100644 index 1b63ff9539..00 --- a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml +++ /dev/null @@ -1,36 +0,0 @@ - - i440fx - 56f5055c-1b8d-490c-844a-ad646a1c - 1048576 - 1048576 - 1 - -hvm - - - - - - - - -qemu64 - - - destroy - restart - destroy - -/usr/bin/qemu-system-x86_64 - - - - - - - - - - - - diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml new file mode 12 index 00..8154897401 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml deleted file mode 100644 index f7b2cbb9ce..00 --- a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml +++ /dev/null @@ -1,36 +0,0 @@ - - i440fx - 56f5055c-1b8d-490c-844a-ad646a1c - 1048576 - 1048576 - 1 - -hvm - - - - - - - - -qemu64 - - - destroy - restart - destroy - -/usr/bin/qemu-system-x86_64 - - - - - - - - - - - - diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml new file mode 12 index 00..6b9e5492f8 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml deleted file mode 100644 index 5a78fe638d..00 --- a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-latest.xml +++ /dev/null @@ -1,33 +0,0 @@ - - i440fx - 56f5055c-1b8d-490c-844a-ad646a1c - 1048576 - 1048576 - 1 - -hvm - - - -qemu64 - - - destroy - restart - destroy - -/usr/bin/qemu-system-x86_64 - - - - - - - - - - - - - - diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml new file mode 12 index 00..0570e40273 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi
[libvirt PATCH 05/10] Revert "qemuValidateDomainDefPCIFeature: Fix validation logic"
This reverts commit bdc3e8f47be108fa552b72a6d913528869e61097. Signed-off-by: Laine Stump --- src/qemu/qemu_validate.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 1ffc261c58..d3b9691db5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -179,6 +179,9 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def, int feature) { size_t i; +bool q35Dom = qemuDomainIsQ35(def); +bool q35cap = q35Dom && virQEMUCapsGet(qemuCaps, + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE); if (def->features[feature] == VIR_TRISTATE_SWITCH_ABSENT) return 0; @@ -195,9 +198,9 @@ qemuValidateDomainDefPCIFeature(const virDomainDef *def, virArchToString(def->os.arch)); return -1; } - -if ((qemuDomainIsQ35(def) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) || -(!qemuDomainIsQ35(def) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE))) { +if (!q35cap && +!virQEMUCapsGet(qemuCaps, +QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("acpi-bridge-hotplug is not available with this QEMU binary")); return -1; -- 2.31.1
[libvirt PATCH 00/10] Revert
osen in that case. (Presumably this change prevents the "confusion" that is seen with the existing behavior of the option). So essentially, the choice of whether to use ACPI is controlled by enabling/disabling native PCIe hotplug (which *kind of* goes against the libvirt philosophy of "the XML describes the virtual machine that is presented to the guest"; instead it becomes "the XML describes how the guest will behave"). Another possibility would be to completely scrap (well, deprecate and later remove) the current QEMU commandline switch in favor of one or more switches that behave differently but result in the desired behavior. In either case, I think the best course of action for libvirt is to revert the current , wait until QEMU settles down with a new workable set of switches, and then re-do libvirt support based on that. Laine Stump (10): Revert "qemu: capabilities: Remove QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE" Revert "qemuxml2xmltest: Convert all acpi-hotplug control related tests to DO_TEST_CAPS_LATEST" Revert "qemuxml2argvtest: Add '-enable' variants for ACPI-hotplug related cases" Revert "qemuxml2argvtest: Use real-caps testing for 'acpi-hotplug-bridge-disable'" Revert "qemuValidateDomainDefPCIFeature: Fix validation logic" Revert "qemuValidateDomainDefPCIFeature: un-break error messages" Revert "NEWS: document new acpi pci hotplug config option" Revert "qemu: command: add support for acpi-bridge-hotplug feature" Revert "conf: introduce support for acpi-bridge-hotplug feature" Revert "qemu: capablities: detect acpi-pci-hotplug-with-bridge-support" NEWS.rst | 8 -- docs/formatdomain.rst | 29 -- docs/schemas/domaincommon.rng | 15 src/conf/domain_conf.c| 89 +-- src/conf/domain_conf.h| 9 -- src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_capabilities.h | 3 +- src/qemu/qemu_command.c | 19 src/qemu/qemu_validate.c | 41 - .../caps_6.1.0.x86_64.xml | 1 - .../caps_6.2.0.x86_64.xml | 1 - ...-hotplug-bridge-disable.aarch64-latest.err | 1 - .../aarch64-acpi-hotplug-bridge-disable.xml | 13 --- ...-hotplug-bridge-disable.x86_64-latest.args | 34 --- .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 33 --- ...i-hotplug-bridge-enable.x86_64-latest.args | 34 --- .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 33 --- ...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 - ...-hotplug-bridge-disable.x86_64-latest.args | 37 .../q35-acpi-hotplug-bridge-disable.xml | 47 -- ...cpi-hotplug-bridge-enable.x86_64-6.0.0.err | 1 - ...i-hotplug-bridge-enable.x86_64-latest.args | 37 .../q35-acpi-hotplug-bridge-enable.xml| 47 -- tests/qemuxml2argvtest.c | 10 --- ...i-hotplug-bridge-disable.x86_64-latest.xml | 36 ...pi-hotplug-bridge-enable.x86_64-latest.xml | 36 ...cpi-root-hotplug-disable.x86_64-latest.xml | 33 --- .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + ...acpi-root-hotplug-enable.x86_64-latest.xml | 33 --- .../pc-i440fx-acpi-root-hotplug-enable.xml| 1 + ...i-hotplug-bridge-disable.x86_64-latest.xml | 53 --- ...pi-hotplug-bridge-enable.x86_64-latest.xml | 53 --- tests/qemuxml2xmltest.c | 10 +-- 33 files changed, 9 insertions(+), 794 deletions(-) delete mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err delete mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.args delete mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-6.0.0.err delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.args delete mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml delete mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-
[libvirt PATCH 01/10] Revert "qemu: capabilities: Remove QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE"
This reverts commit 618e8665db2e4c1a8e9a227045b99b48f6110c06. This is the first in a series of 10 commits that revert (in reverse order) the changes to add the switch to libvirt domain XML, which unfortunately needs to be removed due to QEMU developers discovering a flaw with the design of the QEMU commandline switch used to implement the libvirt switch that will likely result in a new and different method of selecting hotplug modes. Because the libvirt switch has not been in any official releases of libvirt, we are still able to remove it completely, rather than deprecating it. The original commits began with commit 58ba0f6a3d7342fba29edbbf2bb9cb5497c870e5. The other original commit IDs are documented in each revert commit. Signed-off-by: Laine Stump --- src/qemu/qemu_capabilities.c | 4 +++- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_command.c | 3 ++- src/qemu/qemu_validate.c | 4 ++-- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 + 17 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cddd39924d..c5ebf5d156 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -644,10 +644,11 @@ VIR_ENUM_IMPL(virQEMUCaps, "virtio-mem-pci", /* QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI */ "memory-backend-file.reserve", /* QEMU_CAPS_MEMORY_BACKEND_RESERVE */ "piix4.acpi-root-pci-hotplug", /* QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */ + "piix4.acpi-hotplug-bridge", /* QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE */ "ich9.acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ - "netdev.json", /* QEMU_CAPS_NETDEV_JSON */ /* 415 */ + "netdev.json", /* QEMU_CAPS_NETDEV_JSON */ "chardev.json", /* QEMU_CAPS_CHARDEV_JSON */ "device.json", /* QEMU_CAPS_DEVICE_JSON */ "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */ @@ -1469,6 +1470,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, { "acpi-root-pci-hotplug", QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, NULL }, +{ "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index bb53d9ae46..ac8a94a0af 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -624,10 +624,11 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI, /* -device virtio-mem-pci */ QEMU_CAPS_MEMORY_BACKEND_RESERVE, /* -object memory-backend-*.reserve= */ QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG, /* -M pc PIIX4_PM.acpi-root-pci-hotplug */ +QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE, /* -M pc PIIX4_PM.acpi-pci-hotplug-with-bridge-support */ QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ -QEMU_CAPS_NETDEV_JSON, /* -netdev accepts JSON */ /* 415 */ +QEMU_CAPS_NETDEV_JSON, /* -netdev accepts JSON */ QEMU_CAPS_CHARDEV_JSON, /* -chardev accepts JSON */ QEMU_CAPS_DEVICE_JSON, /* -device accepts JSON */ QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6df50ec73..989ddcadb8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6464,7 +6464,8 @@ qemuBuildPMCommandLine(virCommand *cmd, if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) { const char *pm_object = NULL; -if (!qemuDomainIsQ35(def)) +if (!qemuDomainIsQ35(def) && +virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE)) pm_object = "PIIX4_PM"; if (qemuDomainIsQ35(def) && diff --git a/
Re: [PATCH] acpi/hotplug: Fix error message format to conform to spec
On 10/18/21 12:31 AM, Ani Sinha wrote: Error messages must conform to spec as specified here: https://www.libvirt.org/coding-style.html#error-message-format This change encloses format specifiers in quotes and unbreaks error messages. Fixes: 8eadf82fb5 ("conf: introduce option to enable/disable pci hotplug on pci-root controller") Fixes: 7300ccc9b3 ("conf: introduce support for acpi-bridge-hotplug feature") Signed-off-by: Ani Sinha Hi Ani, Thanks for following up on your contributions even after they've been pushed :-) The parts of this patch that are relevent to the pci-root hotplug look fine and can be pushed. But the QEMU people have discovered problems with the "acpi-pci-hotplug-with-bridge-support" switch used by libvirt's switch that is forcing them to rethink not just their implementation/design. Basically the way that the QEMU switch works (setting it off will enable native and disable ACPI hotplug, while setting it on will disable native and enable ACPI), while making logical sense from the outside, is "confusing" some guests - that's as much detail as I have, but it means that most likely that QEMU switch will be scrapped and one or more new (and possibly different) switches will be added in their place; it all seems to be up in the air right now. Anyway, since we don't want to have code in libvirt for a QEMU switch that didn't work correctly in the beginning and was then replaced, and since it's highly likely that the new QEMU hotplug-selection method will work differently (and anyway isn't known now), our best course of action seems to be reverting all the acpi-bridge-hotplug patches for now - this is still possible since there hasn't been an official release since they were added. I've already made the revert patches (for the original 4 of the feature, plus 6 patches from pkrempa that fixed up test cases and such) and will be posting them in the morning with a (hopefully) slightly better explanation. I hope you don't find this too discouraging - just keep in mind that the reason for reverting isn't your code, but rather is because the QEMU feature your code is using turns out to not work as advertised, and if the libvirt code that is using it makes it into a release, then we will have to support it essentially forever. (NB: it's also completely possible (but looks to be very unlikely) that QEMU will figure out a way to solve their problems without modifying this switch, and we'll be able to simply re-push the reverted commits; we can't be sure of that right now though). More in the morning... --- src/conf/domain_conf.c | 6 ++ src/qemu/qemu_validate.c| 6 +++--- .../pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err| 2 +- .../pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err | 2 +- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6fcf86ba58..d5de07d13d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17557,8 +17557,7 @@ virDomainFeaturesPCIDefParse(virDomainDef *def, feature = virDomainPCITypeFromString((const char *)node->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported PCI feature: %s"), - node->name); + _("unsupported PCI feature: '%s'"), node->name); return -1; } @@ -21833,8 +21832,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG: if (src->pci_features[i] != dst->pci_features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("State of PCI feature '%s' differs: " - "source: '%s', destination: '%s'"), + _("State of PCI feature '%s' differs: source: '%s', destination: '%s'"), virDomainPCITypeToString(i), virTristateSwitchTypeToString(src->pci_features[i]), virTristateSwitchTypeToString(dst->pci_features[i])); diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 3045e4b64b..f93b697265 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3947,7 +3947,7 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("setting the %s property on a '%s' device is not supported by this QEMU binary"), + _("setting the '%s' property on a '%s' device is not
Re: [PATCH v7 0/4] introduce support for acpi-bridge-hotplug feature
Reviewed-by: Laine Stump I'm going to push this later today after I put it through CI (and get back from a mandatory 4 hour drive). On 10/8/21 2:42 AM, Ani Sinha wrote: changelog: v7: Laine's suggestions from v6 incorporated. rebased the patchset to latest master. v6: rebased to latest. capabilities have been renamed as per suggestions that were made here: https://listman.redhat.com/archives/libvir-list/2021-October/msg00061.html v5: rebased the patchset with the latest master. v4: split the original series into two - pci-root controller specific one (https://www.mail-archive.com/libvir-list@redhat.com/msg221645.html) and this one specific to pci bridges. The conf xml has been introduced as per suggestion by Berrange here: https://patchew.org/Libvirt/20210912032631.2853520-1-...@anisinha.ca Changes has been introduced to parse and validate the xml accordingly as well as to add backend qemu commandline option. v3: reorganized the patches as per Laine's suggestion. Added more details in commit messages. Added conf description in formatdomain.rst. Added changelog for next release. v2: fixed bugs and added additional missing unit tests. v1: initial implementation. Had some bugs and missed some unit tests This change introduces a new libvirt sub-element under that can be used to configure all pci related features. Currently the only sub-sub element supported by this sub-element is 'acpi-bridge-hotplug' as shown below: The above option is only available for qemu driver and that too for x86 guests only. It is a global option. 'acpi-bridge-hotplug' option enables or disables ACPI hotplug support for cold-plugged pci bridges. Examples of bridges include PCI-PCI bridge (pci-bridge controller) or PCIe-PCI bridges for pc machines and pcie-root-port controller for q35 machines. Being global option, no other bridge specific option are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the following changes in qemu repo: 9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled") For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu repo: (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) are outlined in (b). There are use cases where users would still want to use native hotplug (see notes). Therefore, this config option enables users to choose either ACPI based hotplug or native hotplug for bridges (for example for pcie root port controller in q35 machines). Notes: One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users. Ani Sinha (4): qemu: capablities: detect presence of acpi-pci-hotplug-with-bridge-support conf: introduce support for acpi-bridge-hotplug feature qemu: command: add support for acpi-bridge-hotplug feature NEWS: add new acpi pci hotplug config option in the release note for next release NEWS.rst | 8 ++ docs/formatdomain.rst | 16 docs/schemas/domaincommon.rng | 15 src/conf/domain_conf.c| 89 ++- src/conf/domain_conf.h| 9 ++ src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 20 + src/qemu/qemu_validate.c | 46 ++ .../caps_2.11.0.x86_64.xml| 1 + .../caps_2.12.0.x86_64.xml| 1 + .../caps_3.0.0.x86_64.xml | 1 + .../caps_3.1.0.x86_64.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.x86_64.xml
Re: [PATCH v6 2/4] conf: introduce support for acpi-bridge-hotplug feature
On 10/5/21 1:51 AM, Ani Sinha wrote: This change introduces a new libvirt sub-element under that can be used to configure all pci related features. Currently the only sub-sub element supported by this sub-element is 'acpi-bridge-hotplug' as shown below: The above option is only available for qemu driver and that too for x86 guests only. It is a global option. 'acpi-bridge-hotplug' option enables or disables ACPI hotplug support for cold-plugged pci bridges. Examples of bridges include PCI-PCI bridge (pci-bridge controller) or PCIe-PCI bridges for pc machines and pcie-root-port controller for q35 machines. Being global option, no other bridge specific option are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the following changes in qemu repo: 9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled") For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu repo: (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) are outlined in (b). There are use cases where users would still want to use native hotplug. Therefore, this config option enables users to choose either ACPI based hotplug or native hotplug for bridges (for example for pcie root port controller in q35 machines). Qemu capability validation checks have also been added along with related unit tests to exercise the new conf option. Signed-off-by: Ani Sinha --- docs/formatdomain.rst | 11 +++ docs/schemas/domaincommon.rng | 15 src/conf/domain_conf.c| 89 ++- src/conf/domain_conf.h| 9 ++ src/qemu/qemu_validate.c | 46 ++ .../aarch64-acpi-hotplug-bridge-disable.xml | 33 +++ .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 33 +++ .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 33 +++ .../q35-acpi-hotplug-bridge-disable.xml | 47 ++ .../q35-acpi-hotplug-bridge-enable.xml| 47 ++ .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 1 + .../pc-i440fx-acpi-hotplug-bridge-enable.xml | 1 + .../q35-acpi-hotplug-bridge-disable.xml | 1 + .../q35-acpi-hotplug-bridge-enable.xml| 1 + tests/qemuxml2xmltest.c | 14 +++ 15 files changed, 380 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml create mode 12 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index a02802a954..8d916eefa6 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1847,6 +1847,9 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. + + + @@ -1942,6 +1945,14 @@ are: passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode - optional string sync_pt or share_pt :since:`6.3.0` === == === == +``pci`` + Various PCI bus related features of the hypervisor. + === + Feature Description Value Since + === + acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI bridges (pc) and pcie-root-ports (q35) on, off :since:`7.8.0 (QEMU 6.1.0)` Weren't the qemu properties there in a much earlier version of QEMU? (Oh, and also it needs to say 7.9.0 instead of 7.8.0) Somewhere we need to put the extra info that on 440fx machinetypes ACPI is on by default, and disabling it means that only SHPC
Re: [PATCH v6 4/4] NEWS: add new acpi pci hotplug config option in the release note for next release
On 10/5/21 1:51 AM, Ani Sinha wrote: Added the following new libvirt conf option to the release note to indicate their availability with the next release: Signed-off-by: Ani Sinha --- NEWS.rst | 7 +++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index caa61f0646..25de621c91 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -32,6 +32,13 @@ v7.9.0 (unreleased) controller. The default behavior is unchanged (hotplug is allowed). + * Add a new global feature for controlling ACPI PCI hotplug on bridges + +A new config option under +sub-element have been added to allow users control BIOS ACPI based PCI +hotplug for cold plugged bridges. It is only applicable for x86 guests +using qemu driver. How about: A new config option under sub-element has been added to allow users to enable/disable ACPI based PCI hotplug for cold plugged bridges. It is only applicable for x86 guests using qemu driver. Reviewed-by: Laine Stump + * **Improvements** * **Bug fixes**
Re: [PATCH v6 3/4] qemu: command: add support for acpi-bridge-hotplug feature
On 10/5/21 1:51 AM, Ani Sinha wrote: This change adds backend qemu command line support for new libvirt global feature 'acpi-bridge-hotplug'. This option can be used as following: The '' sub-element under '' is also newly introduced. 'acpi-bridge-hotplug' turns on the following command line option to qemu for x86 guests: (pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support= (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support= This change also adds the required qemuxml2argv unit tests in order to test correct qemu arguments. Unit tests have also been added to test qemu capability validation checks as well as checks for using this option with the right architecture. Signed-off-by: Ani Sinha --- src/qemu/qemu_command.c | 14 .../aarch64-acpi-hotplug-bridge-disable.err | 1 + ...pc-i440fx-acpi-hotplug-bridge-disable.args | 31 + .../pc-i440fx-acpi-hotplug-bridge-disable.err | 1 + .../q35-acpi-hotplug-bridge-disable.args | 33 +++ .../q35-acpi-hotplug-bridge-disable.err | 1 + tests/qemuxml2argvtest.c | 16 + 7 files changed, 97 insertions(+) create mode 100644 tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.err create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.err diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 08f6d735f8..6c8c99e595 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6069,6 +6069,7 @@ qemuBuildPMCommandLine(virCommand *cmd, qemuDomainObjPrivate *priv) { virQEMUCaps *qemuCaps = priv->qemuCaps; +int acpihp_br = def->pci_features[(virDomainPCI)VIR_DOMAIN_PCI_ACPI_BRIDGE_HP]; I'm not sure why you put the typecast on the array subscript, but it's unnecessary. I removed it. if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { /* with new qemu we always want '-no-shutdown' on startup and we set @@ -6114,6 +6115,19 @@ qemuBuildPMCommandLine(virCommand *cmd, pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); } +if (acpihp_br) { +const char *pm_object = "PIIX4_PM"; + +if (qemuDomainIsQ35(def) && +virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) +pm_object = "ICH9-LPC"; So on Q35 systems that don't support setting "ICH9-LPC.acpi-pci-hotplug." it will also work to set "PIIX4_PM.acpi-pci-"? (apparently this is the case with the s3 and s4 acpi suspend options, which have a similar hack a few lines below this one). Assuming that is the case, then this patch also seems fine to me Reviewed-by: Laine Stump I may want to make the documentation in formatdomain.rst a bit more clear, but I can do that as a separate patch after these (since I know you're leaving on a vacation so won't be around to do it). As long as nobody complains about the naming of the feature, I think I can push this tomorrow evening (U.S. time) (that will give time for any discussion about naming in the morning, then I'll have to be afk for a few hours in the afternoon. Thanks for your contribution, and for your perseverance in the face of my procrastination, and enjoy your vacation!! + +virCommandAddArg(cmd, "-global"); +virCommandAddArgFormat(cmd, "%s.acpi-pci-hotplug-with-bridge-support=%s", + pm_object, + virTristateSwitchTypeToString(acpihp_br)); +} + return 0; } diff --git a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err new file mode 100644 index 00..9f0a88b826 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.err @@ -0,0 +1 @@ +unsupported configuration: acpi-bridge-hotplug is not available for architecture 'aarch64' diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args new file mode 100644 index 00..a1e5dc85c7 --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-i440fx \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=i440fx,debug-threads=on
Re: [PATCH v6 2/4] conf: introduce support for acpi-bridge-hotplug feature
On 10/7/21 11:12 PM, Laine Stump wrote: @@ -27932,6 +27995,30 @@ virDomainDefFormatFeatures(virBuffer *buf, virDomainIBSTypeToString(def->features[i])); break; + case VIR_DOMAIN_FEATURE_PCI: + if (def->features[i] != VIR_TRISTATE_SWITCH_ON) + break; + + virBufferAddLit(, "\n"); + virBufferAdjustIndent(, 2); + for (j = 0; j < VIR_DOMAIN_PCI_LAST; j++) { + switch ((virDomainPCI) j) { + case VIR_DOMAIN_PCI_ACPI_BRIDGE_HP: + if (def->pci_features[j]) Oops, I missed this the first time I went through - this should compare to VIR_TRISTATE_SWITCH_ABSENT rather than just checking for non-0. It ends up being the same thing, but you never know - some day someone may decide to change the values of the VIR_TRISTATE_* enums just for fun... + virBufferAsprintf(, "<%s state='%s'/>\n", + virDomainPCITypeToString(j), + virTristateSwitchTypeToString( + def->pci_features[j])); + break;
Re: [PATCH v6 2/4] conf: introduce support for acpi-bridge-hotplug feature
= + ``pmu`` Depending on the ``state`` attribute (values ``on``, ``off``, default ``on``) enable or disable the performance monitoring unit for the guest. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ec5bd91740..6f33d1e774 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6169,6 +6169,9 @@ + + + @@ -6400,6 +6403,18 @@ + + + + + + + + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b8370f6950..229b75fecc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -172,6 +172,7 @@ VIR_ENUM_IMPL(virDomainFeature, "cfpc", "sbbc", "ibs", + "pci", ); VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, @@ -212,6 +213,11 @@ VIR_ENUM_IMPL(virDomainXen, "passthrough", ); +VIR_ENUM_IMPL(virDomainPCI, + VIR_DOMAIN_PCI_LAST, + "acpi-bridge-hotplug", +); + VIR_ENUM_IMPL(virDomainXenPassthroughMode, VIR_DOMAIN_XEN_PASSTHROUGH_MODE_LAST, "default", @@ -17536,6 +17542,36 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, return 0; } +static int +virDomainFeaturesPCIDefParse(virDomainDef *def, + xmlNodePtr node) +{ +def->features[VIR_DOMAIN_FEATURE_PCI] = VIR_TRISTATE_SWITCH_ON; + +node = xmlFirstElementChild(node); +while (node) { +int feature; +virTristateSwitch value; + +feature = virDomainPCITypeFromString((const char *)node->name); +if (feature < 0) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported PCI feature: %s"), + node->name); +return -1; +} + +if (virXMLPropTristateSwitch(node, "state", VIR_XML_PROP_REQUIRED, + ) < 0) +return -1; + +def->pci_features[feature] = value; + +node = xmlNextElementSibling(node); +} + +return 0; +} Hmm. On one hand, this is adding a new case of manually iterating through the nodes of an XML document, which I thought we were trying to get rid of. On the other hand, it was obviously based on the existing code for the other subelements of the element (which are the only other remaining uses of xmlFirstElementChild()) so either that code was purposefully left that way (and not reworked to use all of Tim's nice helper functions) in order to detect unknown subelements at parse time (which we usually don't do (and which has always kind of bothered me)), or there was some issue that required more thought so it was left as an exercise for later, in which case this new function could be redone at the same time as the others. So unless someone else has a problem with it, I'm okay with putting it in. Reviewed-by: Laine Stump (as long as there are no objections to the name of the element in the XML, and adding a new function that uses the "old" parsing method). static int virDomainFeaturesXENDefParse(virDomainDef *def, @@ -17835,6 +17871,10 @@ virDomainFeaturesDefParse(virDomainDef *def, break; } +case VIR_DOMAIN_FEATURE_PCI: +if (virDomainFeaturesPCIDefParse(def, nodes[i]) < 0) +return -1; + case VIR_DOMAIN_FEATURE_LAST: break; } @@ -17843,7 +17883,6 @@ virDomainFeaturesDefParse(virDomainDef *def, return 0; } - static int virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDef *def) { @@ -21521,6 +21560,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: +case VIR_DOMAIN_FEATURE_PCI: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " @@ -21777,6 +21817,29 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, } } +/* pci */ +if (src->features[VIR_DOMAIN_FEATURE_PCI] == VIR_TRISTATE_SWITCH_ON) { +for (i = 0; i < VIR_DOMAIN_PCI_LAST; i++) { +switch ((virDomainPCI) i) { +case VIR_DOMAIN_PCI_ACPI_BRIDGE_HP: +if (src->pci_features[i] != dst->pci_features[i]) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of PCI feature '%s' differs: " +
Re: [PATCH v6 1/4] qemu: capablities: detect presence of acpi-pci-hotplug-with-bridge-support
On 10/5/21 1:51 AM, Ani Sinha wrote: qemu added support for i440fx specific global boolean flag PIIX4_PM.acpi-pci-hotplug-with-bridge-support around version 2.1. This flag is enabled by default. When disabled, it turns off acpi pci hotplug for cold plugged pci bridges in i440fx machine types. Very recently, in qemu version 6.1, the same global option was also added for q35 machine types as well. ICH9-LPC.acpi-pci-hotplug-with-bridge-support This option turns on or off acpi based hotplug for cold plugged pcie bridges like pcie root ports. This flag is also enabled by default. Please refer to the following qemu changes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") This patch adds the corresponding qemu capabilities in libvirt. For i440fx, the capability is detected as QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE. Please note that the test specific qemu capabilities .replies files has already been updated as a part of regular refreshing them when a new qemu version is released. Hence, no updates to those files are required. Signed-off-by: Ani Sinha Reviewed-by: Laine Stump --- src/qemu/qemu_capabilities.c | 4 src/qemu/qemu_capabilities.h | 2 ++ tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 2 ++ 14 files changed, 19 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 82687dbf39..c4d0e1858c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -644,6 +644,8 @@ VIR_ENUM_IMPL(virQEMUCaps, "virtio-mem-pci", /* QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI */ "memory-backend-file.reserve", /* QEMU_CAPS_MEMORY_BACKEND_RESERVE */ "piix4.acpi-root-pci-hotplug", /* QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG */ + "piix4.acpi-hotplug-bridge", /* QEMU_CAPS_PIIX4_ACPI_HOTPLUG_BRIDGE */ + "ich9.acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ The mechanics of this are all fine, as long as everyone is happy with the names. Reviewed-by: Laine Stump
Re: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
On 10/3/21 10:10 PM, Douglas, William wrote: On Sun, 2021-10-03 at 15:52 -0400, Laine Stump wrote: On 10/3/21 3:43 PM, Laine Stump wrote: cleanup: + if (mon) + virCHMonitorClose(mon); Oops, I also meant to point out that the "if (mon)" is unnecessary here, because (as with all similar functions in libvirt) virCHMonitorClose() can be called with a null argument, and will just be a NOP in that case. If you want me to split and push, I'll fix that up before pushing too. Ah oops, thanks for pointing that out. I'll gladly take advantage of your offer to split them up and push, thanks! OKay, I've pushed everything now. Thanks for following up!
Re: [PATCH] qemu: capabilities: rename piix4-acpi-root-hotplug-en to more appropriate name
On 10/4/21 1:26 PM, Ani Sinha wrote: The capability name piix4-acpi-root-hotplug-en is not conventional and appreared to be confusing to some. "en" suffix is also incorrect as the capability in qemu is used to both enable and disable hotplug on the pci root bus on the i440fx. Hence, rename it to piix4.acpi-root-pci-hotplug so that it is clearer, less confusing and more accurate. Signed-off-by: Ani Sinha --- src/qemu/qemu_capabilities.c | 2 +- tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9d0c96a20c..d7b79ef7c0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -641,7 +641,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */ /* 410 */ - "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ + "piix4.acpi-root-pci-hotplug", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ Since the enum matched the capability name exactly except that one is "piix4" and the other is "PIIX", I changed the enum to match this new name before pushing. Reviewed-by: Laine Stump Will be pushed in a few minutes as soon as gitlab CI finishes running on my review branch. ); diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index 1e5833a9f0..834fb86636 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -230,7 +230,7 @@ - + 5002000 0 43100243 diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml index b54dd8a22e..e9d1a26400 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -238,7 +238,7 @@ - + 600 0 43100242 diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml index 0ad493191d..971d55e0cc 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -240,7 +240,7 @@ - + 6001000 0 43100243
Re: [PATCH v7 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
On 10/4/21 1:05 PM, Andrea Bolognani wrote: On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote: On Mon, 4 Oct 2021, Andrea Bolognani wrote: On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote: +++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from? "en" stands for enable. AFAICT there are no other capabilities that use this convention. Moreover, the option can be used both to enable *and* disable hotplug, so the name doesn't seem accurate either... The string is not exposed to users, so ultimately none of this really matters, but I would still suggest changing it to piix4.acpi-root-pci-hotplug I'd be happy to either review a patch of yours that does that, or preparing such a patch myself. BAH!!! Someone else mentioned that in an earlier iteration of the patches (and may have even suggested the "." after piix4 instead of "-"), and I had meant to check/ask about it in the respin, and then forgot. :-/ (somehow my brain skipped right over those 3 characters) I agree with Andrea's suggested change, and apologize for not catching it in review (fortunately it was pushed just *after* a release instead of before, so we can still make such a change). I'll gladly review and/or push any such patch that arrives.
Re: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
On 10/3/21 3:43 PM, Laine Stump wrote: cleanup: + if (mon) + virCHMonitorClose(mon); Oops, I also meant to point out that the "if (mon)" is unnecessary here, because (as with all similar functions in libvirt) virCHMonitorClose() can be called with a null argument, and will just be a NOP in that case. If you want me to split and push, I'll fix that up before pushing too.
Re: [PATCH 3/5] ch: use g_auto in virCHMonitorBuildMemoryJson
On 10/1/21 2:12 PM, William Douglas wrote: Signed-off-by: William Douglas --- src/ch/ch_monitor.c | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 800457af41..7326ac645b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -154,22 +154,17 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) { -virJSONValue *memory; unsigned long long total_memory = virDomainDefGetMemoryInitial(vmdef) * 1024; if (total_memory != 0) { -memory = virJSONValueNewObject(); +g_autoptr(virJSONValue) memory = virJSONValueNewObject(); there should be an extra empy line here between variable definition and the start of instructions. if (virJSONValueObjectAppendNumberUlong(memory, "size", total_memory) < 0) -goto cleanup; +return -1; also an empty line here makes the code more readable. if (virJSONValueObjectAppend(content, "memory", ) < 0) -goto cleanup; +return -1; } return 0; - - cleanup: -virJSONValueFree(memory); -return -1; } static int Reviewed-by: Laine Stump I'll add the extra empty lines before pushing.
Re: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
On 10/1/21 2:12 PM, William Douglas wrote: In virCHMontiorNew the monitor object is referenced an additional time incorrectly preventing it from being disposed of. Because the disposal wasn't being used, a bug in virCHMonitorClose that would incorrectly unref the domain object wasn't being seen. This change fixes both. Although each is very small, the fixes should be in separate patches since they are separate and unconnected problems. If it's okay with you, I'll just split this patch and adjust the log comments (while still attributing to you) before pushing. Or if you'd rather, you can resend two separate patches for it, with log messages of your choice. Let me know which you prefer. Otherwise: Reviewed-by: Laine Stump Signed-off-by: William Douglas --- src/ch/ch_monitor.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index a1430f0e65..800457af41 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -468,7 +468,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) if (!vm->def) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("VM is not defined")); -return NULL; +goto cleanup; } /* prepare to launch Cloud-Hypervisor socket */ @@ -502,12 +502,14 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) mon->handle = curl_easy_init(); /* now has its own reference */ -virObjectRef(mon); mon->vm = virObjectRef(vm); ret = mon; +mon = NULL; cleanup: +if (mon) +virCHMonitorClose(mon); virCommandFree(cmd); return ret; } @@ -542,7 +544,6 @@ void virCHMonitorClose(virCHMonitor *mon) g_free(mon->socketpath); } -virObjectUnref(mon->vm); virObjectUnref(mon); }
Re: [PATCH 1/5] ch_monitor: Stop leaking json value objects
On 10/1/21 2:12 PM, William Douglas wrote: In virCHMonitorBuildKernelRelatedJson there are two cases of json value objects being lost after the pointer being redefined. This change removes the needless redefinition. Signed-off-by: William Douglas Reviewed-by: Laine Stump
Re: [PATCH v7 0/4] Add support to enable/disable hotplug on pci-root controller
On 10/1/21 5:29 AM, Ani Sinha wrote: changelog: v7: rebased the patches post libvirt release 7.8. Adjusted NEWS.rst to reflect 7.9 (unreleased) version. Changed version info for new config option in formatdomain.rst as well. Added reviewed-by tags. Also in formatdomain.rst added information on what type of hotplug (ACPI/native etc) are affected by the setting. v6: incorporated Jan's suggestions. v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command line is now added in a new function and called from qemuBuildControllersByTypeCommandLine(). Output xmls are now symlinked to input xmls for unit tests. v4: split the original patchset into a pci-root controller specific patch series. also the libvirt conf is now a sub-element of the pci-root controller as was suggested by Dan Berrange. Please see discussion here: https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html v3: reorganized the patches as per Laine's suggestion. Added more details in commit messages. Added conf description in formatdomain.rst. Added changelog for next release. v2: fixed bugs and added additional missing unit tests. v1: initial implementation. Had some bugs and missed some unit tests This patchset introduces libvirt xml support to enable/disable hotplug on the pci-root controller. It adds a 'target' subelement for the pci-root controller with a 'hotplug' property. This property can be used to enable or disable hotplug for the pci-root controller. For example, in order to disable hotplug on the pci-root controller, one has to use set '' as shown below: '' option would enable hotplug for pci-root controller. This is also the default value. This option is only available for pc machine types and is applicable for qemu only/kvm accelerator onlt.This feature was introduced from qemu version 5.2 with the following change in qemu repository: 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some reasons why users might to disable hotplug on PCI root buses [1]. The corresponding commandline option to qemu for x86 guests is: -global PIIX4_PM.acpi-root-pci-hotplug= Notes: 1. The use case scenario described by Laine in https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html intentionally does not discuss i440fx and focusses solely on q35. I do realize that redhat has moved on from i440fx and currently efforts for new features are concentrated on q35 machines only. We have had some hard debates on this on the qemu mailing list before. The fact of the matter is that i440fx is not at 1-1 parity with q35. There are many users who are currenly using i440fx and are simply not ready to move to q35 without sacrificing some existing features they support today. For example https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations. https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more information on the differences. Hence we need to solve the issue Laine has described in the above email for i440fx without adding additional bridges. Further, in Daniel Berrange's words from : https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html "From the upstream POV, there's been no decision / agreement to phase out PIIX, this is purely a RHEL downstream decision & plan. If other distros / users have a different POV, and find the feature useful, we should accept the patch if it meets the normal QEMU patch requirements. " Also to be noted that I have already experimented this qemu commandline option using libvirt passthrough feature as has been documented in http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html This was only meant to be a short term solution until libvirt started supporting this natively. Supporting this option through libvirt would simplify their use case as well as add capability validations and graceful failure scenarios in case qemu did not support the option. Ani Sinha (4): qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines conf: introduce option to enable/disable pci hotplug on pci-root controller qemu: command: add support to enable/disable hotplug on pci-root controller NEWS: release note the new hotplug enable/disable option on pci-root controller NEWS.rst | 6 docs/formatdomain.rst | 14 ++--- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 17 ++ src/qemu/qemu_validate.c | 9 +- .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 +
Re: [libvirt PATCH v5 5/7] Add PCI VPD Capability Support
On 9/27/21 3:30 PM, Dmitrii Shcherbakov wrote: [...] diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c [...] + +static void +virNodeDeviceCapVPDResourceFormat(virBuffer *buf, virPCIVPDResource *res) +{ +GEnumValue *resRype = NULL; +void (*resFormatFunc)(virBuffer *buf, virPCIVPDResource *res); + +if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_STRING_RESOURCE)) { +resFormatFunc = virNodeDeviceCapVPDStringResourceFormat; +} else if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE)) { +resFormatFunc = virNodeDeviceCapVPDKeywordResourceFormat; +} else { +/* Unexpected resource type. This should not happen unless the PCI(e) specs + * change and new resource types are introduced into util.virpcivpd. Either way, + * we can only return the control back to the caller here. + */ +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected VPD resource type encountered during formatting")); +return; +} +virBufferAdjustIndent(buf, 2); + +resRype = virPCIVPDResourceGetResourceType(res); +virBufferEscapeString(buf, "", resRype->value_nick); +/* Format the resource in a type-specific way. */ +resFormatFunc(buf, res); It's probably irrelevant since this will be mostly rewritten based on Dan's suggestions, but the resFormatFunc() pointer seems like an unnecessary complication - you could just move the if/else construct down to here and call the appropriate function directly.
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On 10/1/21 5:57 AM, Daniel P. Berrangé wrote: On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote: [...] +GType +vir_pci_vpd_resource_type_get_type(void) I know you had asked about using under_scored_naming in a reply to Peter pointing out "non-standard" names in V3 of your patches, but I don't think anyone followed up on that. I do see some of the examples you pointed out in your reply, so definitely there is precedent. Personally when I see a function with underscores in its name, my subconscious immediately thinks (before looking at the words in the name) that they must be from an external library somewhere. My preference would be to have all functions defined within libvirt follow libvirt's naming convention (yeah, I know there's lots of stuff that doesn't!), but I may be an outlier here though (especially since at least some of your examples, e.g. vir_object_init(), was authored by danpb, who also hasn't complained about your choices for names, so...) So this is more a question for the rest of longtime libvirt developers rather than you - do we care about this? If so, exactly what is the policy? +{ +static GType resourceType; + +static const GEnumValue resourceTypes[] = { +{VIR_PCI_VPD_RESOURCE_TYPE_STRING, "String resource", "string"}, +{VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, "Read-only resource", "vpd-r"}, +{VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, "Read-write resource", "vpd-w"}, +{VIR_PCI_VPD_RESOURCE_TYPE_LAST, "last", "last"}, +{0, NULL, NULL}, +}; + +if (!resourceType) { +resourceType = g_enum_register_static("virPCIVPDResourceType", resourceTypes); +} +return resourceType; +} + +static gboolean In a similar "coding standards" vein - other uses of "gboolean" (rather than the much more common "bool", or *very rarely* "boolean") in our code seem to be the product of auto-generated(?) xdr headers for wireshark, functions that are registered as callbacks to glib (and so must follow the types in the callback function pointer prototype), and a very small number of other cases. Do we want new code using gboolean rather than bool in these "other" cases that don't require gboolean for proper glib type compatibility? I don't have a strong opinion except that I think consistency is good (otherwise people will spend time trying to decide which one to use in every case), and now is a better time than change the types than later, if that's what people want. (a side-nit completely unrelated to your patches - I noticed that in at least a couple places in libvirt, a gboolean is assigned "true" or "false", although the glib documentation says that it should only have the value "TRUE" or "FALSE". Good thing those happen to use the same values!) +virPCIVPDResourceIsVendorKeyword(const gchar *keyword) I similarly wonder about use of gchar rather than char when it's not required for type compatibility with glib APIs. So I guess basically I'm wondering just how far down the glib rabbit hole we aim to go :-) + +G_BEGIN_DECLS; > > This is not required in libvirt internal code, since we never use C++ > internally. > Note to Daniel: I'm glad you gave up on waiting for me to review these patches, because I'm not conversant enough with glib to have caught this (and also would have missed the whole thing about the unnecessary strduping of hash keys). +#ifdef __linux__ +/** + * virCreateAnonymousFile: + * @data: a pointer to data to be written into a new file. + * @len: the length of data to be written (in bytes). + * + * Create a fake fd, write initial data to it. + * + */ +int +virCreateAnonymousFile(const uint8_t *data, size_t len) +{ +int fd = -1; +char path[] = abs_builddir "testutils-memfd-XX"; +/* A temp file is used since not all supported distributions support memfd. */ +if ((fd = g_mkstemp_full(path, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) { +return fd; +} +g_unlink(path); + +if (ftruncate(fd, len) != 0) { +VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file", +g_strerror(errno)); +goto cleanup; +} Since it is a new temporary file it is guaranteed zero length, so this should be redundant AFAICT. I would've missed this too, not due to unfamiliarity, but just that I'd be too busy looking for non-standard names and leaked pointers to pay attention. (Okay, I'll stop embarrassing myself now).
Re: [PATCH] failover: allow to pause the VM during the migration
On 9/30/21 1:09 PM, Laurent Vivier wrote: If we want to save a snapshot of a VM to a file, we used to follow the following steps: 1- stop the VM: (qemu) stop 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" 3- resume the VM: (qemu) cont After that we can restore the snapshot with: qemu-system-x86_64 ... -incoming "exec:cat snapshot" (qemu) cont This is the basics of what libvirt does for a snapshot, and steps 1+2 are what it does for a "managedsave" (where it saves the snapshot to disk and then terminates the qemu process, for later re-animation). In those cases, it seems like this new parameter could work for us - instead of explicitly pausing the guest prior to migrating it to disk, we would set this new parameter to on, then directly migrate-to-disk (relying on qemu to do the pause). Care will need to be taken to assure that error recovery behaves the same though. There are a couple of cases when libvirt apparently *doesn't* pause the guest during the migrate-to-disk, both having to do with saving a coredump of the guest. Since I really have no idea of how common/important that is (or even if my assessment of the code is correct), I'm Cc'ing this patch to libvir-list to make sure it catches the attention of someone who knows the answers and implications. But when failover is configured, it doesn't work anymore. As the failover needs to ask the guest OS to unplug the card the machine cannot be paused. This patch introduces a new migration parameter, "pause-vm", that asks the migration to pause the VM during the migration startup phase after the the card is unplugged. Once the migration is done, we only need to resume the VM with "cont" and the card is plugged back: 1- set the parameter: (qemu) migrate_set_parameter pause-vm on 2- migrate the VM to a file: (qemu) migrate "exec:cat > snapshot" The primary failover card (VFIO) is unplugged and the VM is paused. 3- resume the VM: (qemu) cont The VM restarts and the primary failover card is plugged back The VM state sent in the migration stream is "paused", it means when the snapshot is loaded or if the stream is sent to a destination QEMU, the VM needs to be resumed manually. Signed-off-by: Laurent Vivier --- qapi/migration.json| 20 +++--- include/hw/virtio/virtio-net.h | 1 + hw/net/virtio-net.c| 33 ++ migration/migration.c | 37 +- monitor/hmp-cmds.c | 8 5 files changed, 95 insertions(+), 4 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index 88f07baedd06..86284d96ad2a 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -743,6 +743,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -758,7 +762,7 @@ 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', 'multifd-zlib-level' ,'multifd-zstd-level', - 'block-bitmap-mapping' ] } + 'block-bitmap-mapping', 'pause-vm' ] } ## # @MigrateSetParameters: @@ -903,6 +907,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -934,7 +942,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8', '*multifd-zstd-level': 'uint8', -'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } } +'*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ], +'*pause-vm': 'bool' } } ## # @migrate-set-parameters: @@ -1099,6 +1108,10 @@ #block device name if there is one, and to their node name #otherwise. (Since 5.2) # +# @pause-vm: Pause the virtual machine before doing the migration. +# This allows QEMU to unplug a card before doing the +# migration as it depends on the guest kernel. (since 6.2) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -1128,7 +1141,8 @@ '*multifd-compression': 'MultiFDCompression', '*multifd-zlib-level': 'uint8',
Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On 9/30/21 2:16 AM, Ani Sinha wrote: On Fri, Sep 24, 2021 at 2:16 AM Laine Stump wrote: On 9/11/21 11:26 PM, Ani Sinha wrote: The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options. ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are: The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports". I think emphasizing cold plugged bridges is important as Igor (CC'd) has clarified in the other email on patch #3 of this series. Okay, so the implication in Igor's email is that a) it is possible to hotplug a pcie controller, but b) any controller that is hotplugged will not have ACPI enabled. Note though that libvirt doesn't allow hotplugging *any* PCI controller, since we were told long ago that no OS will actually rescan the PCI topology when this is done, and so the new controller wouldn't be usable anyway. (that information may well be outdated). I think if you're going to mention that it is specifically for "cold-plugged bridges" then you should also 1) define what "cold-plugged" means, i.e. "(PCI controllers that were present in the domain definition when the guest was first started"), and 2) note that "ACPI is not enabled for bridges that are hot-plugged (but currently libvirt doesn't support hotplugging a pci controller anyway)" or something like that.
Re: [PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and acpi-root-hotplug pm options
On 9/28/21 12:54 PM, Ani Sinha wrote: On Tue, Sep 28, 2021 at 10:17 PM Laine Stump <mailto:la...@redhat.com>> wrote: On 9/28/21 4:44 AM, Daniel P. Berrangé wrote: > On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: >> This change introduces libvirt xml support for the following two pm options: >> >> >> >> >> > > >> +``acpi-hotplug-bridge`` >> + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support >> + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc >> + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 >> + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges >> + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, >> + it includes PCIE root ports (pcie-root-port controller). This is a global option that >> + affects all bridges. No other bridge specific option is required to be specified. > > Can you confirm my understanding of the situation.. > > - i440fx / PCI topology - hotplug always uses ACPI > > - q35 / PCIe topology - hotplug historically used native PCIe hotplug, > but in 6.1 switched to ACPI > > Given, the name "acpi-hotplug-bridge", am I right that this option > has *no* effect, if the q35 machine is using native PCIe hotplug > approach ? IOW, is it a no-op until 6.1 based machine types for q35 ? I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on 6.1 not only introduced this option/command line in Qemu but also flipped the switch to make ACPI hotplug default for pcie root ports. So there are no officially released version of Qemu where this command line exists and the default is native pcie hotplug. You're assuming that everyone will use the canonical "q35" machinetype rather than a specific versioned machinetype (e.g. "pc-q35-6.0"). For pre-6.1 machinetypes, the default will still be native-pcie hotplug, even when running qemu-6.1+. will simultaneously enable ACPI hotplug and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports. This is for 6.1 based machines as well. Similarly on 6.1-based machinetypes, setting acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug. On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root). As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root. (for completeness - when a pcie-root-port or pcie-downstream-port has , that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
Re: [PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and acpi-root-hotplug pm options
On 9/28/21 4:44 AM, Daniel P. Berrangé wrote: On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: This change introduces libvirt xml support for the following two pm options: +``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified. Can you confirm my understanding of the situation.. - i440fx / PCI topology - hotplug always uses ACPI - q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ? IOW, is it a no-op until 6.1 based machine types for q35 ? I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on will simultaneously enable ACPI hotplug and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports. Similarly on 6.1-based machinetypes, setting acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug. On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root). As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root. (for completeness - when a pcie-root-port or pcie-downstream-port has , that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
On 9/27/21 12:54 AM, Ani Sinha wrote: On Sun, 26 Sep 2021, Laine Stump wrote: +QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG); +DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable"); Ah, now I understand the origin of pc-i440fx-acpi-root-hotplug-enable.xml in the previous patch - you had created it to be test case for this negative test. Actually I think the negative test should be done using the ...-disable test case with no caps; in the case that someone has "hotplug='on'" and the option is missing, I think we should just ignore it, since "hotplug='on'" is what they're going to get anyway (that could be added into the validation that was added in the previous patch). You still you add the -enable test case to qemuxml2xmltest.c as I suggested in the previous patch. No. Lets make hotplug=on/off explicit either way. Qemu defaults keep changing :-) Yes, now that there is a settable property its default can be changed, but we do know that on all versions of QEMU that don't support setting this property, hotplug is always enabled for pci-root. Anyway, this isn't important - we can always relax it later if someone really wants it.
Re: [PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On 9/27/21 4:05 AM, Ján Tomko wrote: On a Monday in 2021, Ani Sinha wrote: This change introduces libvirt xml support to enable/disable hotplug on the pci-root controller. It adds a 'target' subelement for the pci-root controller with a 'hotplug' property. This property can be used to enable or disable hotplug for the pci-root controller. For example, in order to disable hotplug on the pci-root controller, one has to use set 'hotplug='off'>' as shown below: '' option would enable hotplug for pci-root controller. This is also the default value. This option is only available for pc machine types and is applicable for qemu only/kvm accelerator onlt.This feature was s/onlt./only. / introduced from qemu version 5.2 with the following change in qemu repository: 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some reasons why users might to disable hotplug on PCI root buses. Related unit tests to exercise the new conf option has also been added. Signed-off-by: Ani Sinha --- docs/formatdomain.rst | 12 src/qemu/qemu_validate.c | 9 +- .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 +++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + tests/qemuxml2xmltest.c | 4 +++ 7 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 12 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, /* hotplug */ if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) { switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting the %s property on a pci-root device is not supported by this QEMU binary"), + "hotplug"); No need to create a new translatable string, the one used by the case below can be reused: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("setting the hotplug property on a '%s' device is not supported by this QEMU binary"), Wouldn't it be better if "hotplug" was also put in the arg list rather than inline in the function? That would avoid some overzealous translator attempting to translate that word. (Or maybe just use the function virReportControllerInvalidOption() for both of these error cases, thus removing both strings)
Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
On 9/26/21 10:35 AM, Ani Sinha wrote: This change adds qemu backend command line support for enabling or disabling hotplug on the pci-root controller using the 'target' sub-element of the pci-root controller as shown below: '' is only valid for pc (x86) machines and turns on the following command line option that is passed to qemu for x86 guests: -global PIIX4_PM.acpi-root-pci-hotplug= This change also adds the required qemuxml2argv unit tests in order to test correct qemu arguments. Unit tests have also been added to test qemu capability validation checks. Signed-off-by: Ani Sinha --- src/qemu/qemu_command.c | 12 +++ .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++ .../pc-i440fx-acpi-root-hotplug-enable.err| 1 + tests/qemuxml2argvtest.c | 6 4 files changed, 50 insertions(+) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b60ee1192b..0cdc10bf29 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6029,6 +6029,7 @@ qemuBuildPMCommandLine(virCommand *cmd, qemuDomainObjPrivate *priv) { virQEMUCaps *qemuCaps = priv->qemuCaps; +int n; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { /* with new qemu we always want '-no-shutdown' on startup and we set @@ -6074,6 +6075,17 @@ qemuBuildPMCommandLine(virCommand *cmd, pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); } +for (n = 0; n < def->ncontrollers; n++) { +virDomainControllerDef *cdef = def->controllers[n]; +if (cdef->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && +cdef->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && +cdef->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { +virCommandAddArg(cmd, "-global"); +virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s", + virTristateSwitchTypeToString(cdef->opts.pciopts.hotplug)); +} +} + It would make more sense to include this in the commandline generator for PCI controllers, since that's where all other XML for PCI controllers is converted into a qemu commandline argument. That's a bit convoluted though, unfortunately. The loop going through all the controllers is in qemuBuildControllersByTypeCommandLine() which then calls qemuBuildControllerDevStr() and *that* is the function that builds the argument to the -device for each controller. For two reasons, we can't add the code for this new option alongside the other PCI controller commandline generation code in qemuBuildControllerDevStr() though: 1) the output of qemuBuildControllerDevStr() is assumed to be following a "-device" argument on the commandline, so we would end up with: "-device -global PIIX4_PM.acpi-root-pci-hotplug=off" which is *not* what we want. 2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls qemuBuildSkipController(), which tells the loop to skip generating any commandline for a pci-root controller (unless it's a P-series guest and the index is non-0). So the only way to make this work is to add a special case to qemuBuildControllersByTypeCommandLine() *before* the call to qemuBuildSkipController() - if the type is pci, model is pci-root, and index is 0 then conditionally add "-global", "PIIX4" and continue (skip the rest of the body of the loop). As with what you've already done here, this is also not 100% clean and consistent with the rest of the code, but since neither method is perfect I think putting it in the function that generates all the rest of the commandline args associated with PCI controllers is more logical and easier to follow. (If anyone disagrees and thinks that the commandline for this should be generated in the place this patch already does it, please speak up!) return 0; } diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args new file mode 100644 index 00..1fb68381d9 --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-i440fx \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=i440fx,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \ +-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \ +-m 1024 \ +-realtime mlock=off \ +-smp
Re: [PATCH v4 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On 9/26/21 10:35 AM, Ani Sinha wrote: This change introduces libvirt xml support to enable/disable hotplug on the pci-root controller. It adds a 'target' subelement for the pci-root controller with a 'hotplug' property. This property can be used to enable or disable hotplug for the pci-root controller. For example, in order to disable hotplug on the pci-root controller, one has to use set '' as shown below: '' option would enable hotplug for pci-root controller. This is also the default value. This option is only available for pc machine types and is applicable for qemu only/kvm accelerator onlt.This feature was introduced from qemu version 5.2 with the following change in qemu repository: 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some reasons why users might to disable hotplug on PCI root buses. Related unit tests to exercise the new conf option has also been added. Signed-off-by: Ani Sinha --- docs/formatdomain.rst | 12 docs/schemas/domaincommon.rng | 10 +++ src/qemu/qemu_validate.c | 9 +- .../pc-i440fx-acpi-root-hotplug-disable.xml | 17 +++ .../pc-i440fx-acpi-root-hotplug-enable.xml| 17 +++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++ tests/qemuxml2xmltest.c | 2 ++ 7 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml create mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 0f5d833521..e2a1768ba7 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).` controller's "port" configuration value, which is visible to the virtual machine. If set, port must be between 0 and 255. ``hotplug`` - pcie-root-port and pcie-switch-downstream-port controllers can also have a - ``hotplug`` attribute in the subelement, which is used to - disable hotplug/unplug of devices on a particular controller. The default - setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable - hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0` + pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and + pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can + also have a ``hotplug`` attribute in the subelement, which is + used to disable hotplug/unplug of devices on a particular controller. The + default setting of ``hotplug`` is ``on``; it should be set to ``off`` to + disable hotplug/unplug of devices on a particular controller. + ``busNr`` pci-expander-bus and pcie-expander-bus controllers can have an optional ``busNr`` attribute (1-254). This will be the bus number of the new bus; All diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fdc04f90aa..cce50c9c89 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2668,6 +2668,16 @@ + + + + + + + + + + This chunk is unnecessary, since the schema already allows for a target/hotplug attribute for *all* PCI controller models (on line 2645), so this new bit in the scheme has no effect. (qemuValidateDomainDeviceDefControllerPCI is doing a more specific check to assure nobody tries to specify the hotplug attribute for controller models that don't support it, and you've added the proper check there) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 13fbfd01b2..510e766cfd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, /* hotplug */ if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) { switch ((virDomainControllerModelPCI) cont->model) { +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting the %s property on a pci-root device is not supported by this QEMU binary"), + "hotplug"); +return -1; +} +break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case
Re: [PATCH v3 2/5] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
On 9/11/21 11:26 PM, Ani Sinha wrote: The following change in qemu added support for a global boolean flag specific to i440fx machines that would turn off or on acpi based hotplug for pci root bus: 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu commandline. It is enabled by default. This patch adds the corresponding qemu capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG. Please note that the test specific qemu capabilities .replies files has already been updated as a part of regular refreshing them when a new qemu version is released. Hence, no updates to those files are required. Signed-off-by: Ani Sinha Reviewed-by: Laine Stump (also needs rebasing due to the other capability added) --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 + 5 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c954e42382..706169a5cf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -642,6 +642,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 410 */ "ich9-acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ ); @@ -1470,6 +1471,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, { "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, NULL }, +{ "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f53009b6e9..b0f9559d95 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -622,6 +622,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 410 */ QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ +QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc PIIX4_PM.acpi-root-pci-hotplug */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index 126bae2401..da52eda200 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -247,6 +247,7 @@ + 5002000 0 43100243 diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml index b01db2dfbe..d75760f1f8 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -255,6 +255,7 @@ + 600 0 43100242 diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml index b136853aba..8fd3c51f4e 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -258,6 +258,7 @@ + 6001000 0 43100243