Re: [PATCH 2/7] Support multiple watchdog devices

On 1/23/23 15:57, Martin Kletzander wrote:

This is already possible with qemu, and actually already happening with q35 machines and a specified watchdog since q35 already includes a watchdog we do not include in the XML. In order to express such posibility multiple watchdogs need to be supported.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 82 +++++++++++++------ src/conf/domain_conf.h | 8 +- src/conf/schemas/domaincommon.rng | 4 +- src/libvirt_private.syms | 1 + src/qemu/qemu_alias.c | 26 ++++-- src/qemu/qemu_alias.h | 5 +- src/qemu/qemu_command.c | 24 ++++-- src/qemu/qemu_domain_address.c | 8 +- src/qemu/qemu_driver.c | 14 ++-- src/qemu/qemu_hotplug.c | 61 ++++++-------- src/qemu/qemu_process.c | 3 +- src/qemu/qemu_validate.c | 25 ++++++ .../watchdog-q35-multiple.x86_64-latest.args | 39 +++++++++ .../watchdog-q35-multiple.xml | 25 ++++++ tests/qemuxml2argvtest.c | 1 + .../watchdog-q35-multiple.x86_64-latest.xml | 50 +++++++++++ tests/qemuxml2xmltest.c | 1 + 17 files changed, 287 insertions(+), 90 deletions(-) create mode 100644 tests/qemuxml2argvdata/watchdog-q35-multiple.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/watchdog-q35-multiple.xml create mode 100644 tests/qemuxml2xmloutdata/watchdog-q35-multiple.x86_64-latest.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 733399e6da0d..7c61da1d765b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3875,7 +3875,9 @@ void virDomainDefFree(virDomainDef *def) def->blkio.ndevices); g_free(def->blkio.devices);

- virDomainWatchdogDefFree(def->watchdog); + for (i = 0; i < def->nwatchdogs; i++) + virDomainWatchdogDefFree(def->watchdogs[i]); + g_free(def->watchdogs);

virDomainMemballoonDefFree(def->memballoon); virDomainNVRAMDefFree(def->nvram); @@ -4647,10 +4649,10 @@ virDomainDeviceInfoIterateFlags(virDomainDef *def, if ((rc = cb(def, &device, &def->fss[i]->info, opaque)) != 0) return rc; } - if (def->watchdog) { - device.type = VIR_DOMAIN_DEVICE_WATCHDOG; - device.data.watchdog = def->watchdog; - if ((rc = cb(def, &device, &def->watchdog->info, opaque)) != 0) + device.type = VIR_DOMAIN_DEVICE_WATCHDOG; + for (i = 0; i < def->nwatchdogs; i++) { + device.data.watchdog = def->watchdogs[i]; + if ((rc = cb(def, &device, &def->watchdogs[i]->info, opaque)) != 0) return rc; } if (def->memballoon) { @@ -18809,24 +18811,21 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, VIR_FREE(nodes);

/* analysis of the watchdog devices */ - def->watchdog = NULL; - if ((n = virXPathNodeSet("./devices/watchdog", ctxt, &nodes)) < 0) + n = virXPathNodeSet("./devices/watchdog", ctxt, &nodes); + if (n < 0) return NULL; - if (n > 1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("only a single watchdog device is supported")); - return NULL; - } - if (n > 0) { + if (n) + def->watchdogs = g_new0(virDomainWatchdogDef *, n); + for (i = 0; i < n; i++) { virDomainWatchdogDef *watchdog;

- watchdog = virDomainWatchdogDefParseXML(xmlopt, nodes[0], ctxt, flags); + watchdog = virDomainWatchdogDefParseXML(xmlopt, nodes[i], ctxt, flags); if (!watchdog) return NULL;

- def->watchdog = watchdog; - VIR_FREE(nodes); + def->watchdogs[def->nwatchdogs++] = watchdog; } + VIR_FREE(nodes);

/* analysis of the memballoon devices */ def->memballoon = NULL; @@ -21255,18 +21254,19 @@ virDomainDefCheckABIStabilityFlags(virDomainDef *src, dst->redirfilter)) goto error;

- if ((!src->watchdog && dst->watchdog) || - (src->watchdog && !dst->watchdog)) { + + if (src->nwatchdogs != dst->nwatchdogs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain watchdog count %d " - "does not match source %d"), - dst->watchdog ? 1 : 0, src->watchdog ? 1 : 0); + _("Target domain watchdog device count %zu " + "does not match source %zu"),

Since you're touching this commit message, put it onto a single line please.

+ dst->nwatchdogs, src->nwatchdogs); goto error; }

- if (src->watchdog && - !virDomainWatchdogDefCheckABIStability(src->watchdog, dst->watchdog)) - goto error; + for (i = 0; i < src->nwatchdogs; i++) { + if (!virDomainWatchdogDefCheckABIStability(src->watchdogs[i], dst->watchdogs[i])) + goto error; + }

if ((!src->memballoon && dst->memballoon) || (src->memballoon && !dst->memballoon)) { @@ -27517,8 +27517,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, return -1; }

- if (def->watchdog) - virDomainWatchdogDefFormat(buf, def->watchdog, flags); + for (n = 0; n < def->nwatchdogs; n++) + virDomainWatchdogDefFormat(buf, def->watchdogs[n], flags);

if (def->memballoon) virDomainMemballoonDefFormat(buf, def->memballoon, flags); @@ -30578,3 +30578,33 @@ virDomainDefHasSpiceGraphics(const virDomainDef *def)

return false; } + + +ssize_t +virDomainWatchdogDefFind(const virDomainDef *def, + const virDomainWatchdogDef *watchdog) +{ + size_t i; + + for (i = 0; i < def->nwatchdogs; i++) { + const virDomainWatchdogDef *tmp = def->watchdogs[i]; + + if (tmp->model != watchdog->model) + continue; + + if (tmp->action != watchdog->action) + continue; + + if (watchdog->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&watchdog->info, &tmp->info)) + continue; + + if (watchdog->info.alias && + STRNEQ_NULLABLE(watchdog->info.alias, tmp->info.alias)) + continue; + + return i; + } + + return -1; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3e4985a67da2..da785076151d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3062,12 +3062,14 @@ struct _virDomainDef { size_t nsysinfo; virSysinfoDef **sysinfo;

+ size_t nwatchdogs; + virDomainWatchdogDef **watchdogs; + /* At maximum 2 TPMs on the domain if a TPM Proxy is present. */ size_t ntpms; virDomainTPMDef **tpms;

/* Only 1 */ - virDomainWatchdogDef *watchdog; virDomainMemballoonDef *memballoon; virDomainNVRAMDef *nvram; virCPUDef *cpu; @@ -3526,6 +3528,10 @@ virDomainSoundDef *virDomainSoundDefRemove(virDomainDef *def, size_t idx); void virDomainAudioDefFree(virDomainAudioDef *def); void virDomainMemballoonDefFree(virDomainMemballoonDef *def); void virDomainNVRAMDefFree(virDomainNVRAMDef *def); +ssize_t +virDomainWatchdogDefFind(const virDomainDef *def, + const virDomainWatchdogDef *watchdog) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; void virDomainWatchdogDefFree(virDomainWatchdogDef *def); virDomainVideoDef *virDomainVideoDefNew(virDomainXMLOption *xmlopt); void virDomainVideoDefFree(virDomainVideoDef *def); diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 6cb0a20e1e10..a5d0505d9b9b 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -6428,9 +6428,9 @@ <ref name="memorydev"/> </choice> </zeroOrMore> - <optional> + <zeroOrMore> <ref name="watchdog"/> - </optional> + </zeroOrMore> <optional> <ref name="memballoon"/> </optional> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 576ec8f95f17..2c5489b2b86e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -692,6 +692,7 @@ virDomainVsockDefFree; virDomainVsockDefNew; virDomainWatchdogActionTypeFromString; virDomainWatchdogActionTypeToString; +virDomainWatchdogDefFind; virDomainWatchdogDefFree; virDomainWatchdogModelTypeFromString; virDomainWatchdogModelTypeToString; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index ef8e87ab58a2..809a6f03ed46 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -560,12 +560,26 @@ qemuAssignDeviceShmemAlias(virDomainDef *def,

void -qemuAssignDeviceWatchdogAlias(virDomainWatchdogDef *watchdog) +qemuAssignDeviceWatchdogAlias(virDomainDef *def, + virDomainWatchdogDef *watchdog, + int idx) { - /* Currently, there's just one watchdog per domain */ + ssize_t i = 0; + + if (watchdog->info.alias) + return; + + if (idx == -1) { + for (i = 0; i < def->nwatchdogs; i++) { + idx = MAX(idx, + qemuDomainDeviceAliasIndex(&def->watchdogs[i]->info, + "watchdog"));

The MAX() macro is a glibc-ism, and worse - it evaluates arguments twice. Please switch to pattern we use elsewhere, e.g. in qemuAssignDeviceShmemAlias() just above.

+ } + + idx++; + }

- if (!watchdog->info.alias) - watchdog->info.alias = g_strdup("watchdog0"); + watchdog->info.alias = g_strdup_printf("watchdog%d", idx); }

@@ -671,8 +685,8 @@ qemuAssignDeviceAliases(virDomainDef *def) for (i = 0; i < def->nsmartcards; i++) { qemuAssignDeviceSmartcardAlias(def->smartcards[i], i); } - if (def->watchdog) { - qemuAssignDeviceWatchdogAlias(def->watchdog); + for (i = 0; i < def->nwatchdogs; i++) { + qemuAssignDeviceWatchdogAlias(def, def->watchdogs[i], i); } if (def->memballoon && def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 6433ae4cecaf..d7f34489f570 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -62,7 +62,10 @@ void qemuAssignDeviceShmemAlias(virDomainDef *def, virDomainShmemDef *shmem, int idx);

-void qemuAssignDeviceWatchdogAlias(virDomainWatchdogDef *watchdog); +void +qemuAssignDeviceWatchdogAlias(virDomainDef *def, + virDomainWatchdogDef *watchdog, + int idx);

void qemuAssignDeviceInputAlias(virDomainDef *def, virDomainInputDef *input, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b96f2d33c158..6e28f8b15e1c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4044,22 +4044,28 @@ qemuBuildWatchdogCommandLine(virCommand *cmd, const virDomainDef *def, virQEMUCaps *qemuCaps) { - virDomainWatchdogDef *watchdog = def->watchdog; - g_autoptr(virJSONValue) props = NULL; + virDomainWatchdogDef *watchdog = NULL; const char *action; int actualAction; + ssize_t i = 0;

- if (!def->watchdog) + if (def->nwatchdogs == 0) return 0;

- if (qemuCommandAddExtDevice(cmd, &def->watchdog->info, def, qemuCaps) < 0) - return -1; + for (i = 0; i < def->nwatchdogs; i++) { + g_autoptr(virJSONValue) props = NULL;

- if (!(props = qemuBuildWatchdogDevProps(def, watchdog))) - return -1; + watchdog = def->watchdogs[i];

- if (qemuBuildDeviceCommandlineFromJSON(cmd, props, def, qemuCaps)) - return -1; + if (qemuCommandAddExtDevice(cmd, &watchdog->info, def, qemuCaps) < 0) + return -1; + + if (!(props = qemuBuildWatchdogDevProps(def, watchdog))) + return -1; + + if (qemuBuildDeviceCommandlineFromJSON(cmd, props, def, qemuCaps)) + return -1; + }

/* qemu doesn't have a 'dump' action; we tell qemu to 'pause', then libvirt listens for the watchdog event, and we perform the dump

Here, I'd mention in the comment that a validator made sure that ALL watchdogs have the same action. It's not obvious at the first glance.

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b8d1969fbebd..db4e91501d3f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2326,10 +2326,10 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def, }

/* A watchdog - check if it is a PCI device */ - if (def->watchdog && - def->watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB && - virDeviceInfoPCIAddressIsWanted(&def->watchdog->info)) { - if (qemuDomainPCIAddressReserveNextAddr(addrs, &def->watchdog->info) < 0) + for (i = 0; i < def->nwatchdogs; i++) { + if (def->watchdogs[i]->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB && + virDeviceInfoPCIAddressIsWanted(&def->watchdogs[i]->info) && + qemuDomainPCIAddressReserveNextAddr(addrs, &def->watchdogs[i]->info) < 0) return -1; }

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d6879175fece..dce3bef5b85b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7258,12 +7258,13 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef, break;

case VIR_DOMAIN_DEVICE_WATCHDOG: - if (vmdef->watchdog) { + if (virDomainWatchdogDefFind(vmdef, dev->data.watchdog) >= 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain already has a watchdog")); + _("device is already in the domain configuration")); return -1; } - vmdef->watchdog = g_steal_pointer(&dev->data.watchdog); + + VIR_APPEND_ELEMENT(vmdef->watchdogs, vmdef->nwatchdogs, dev->data.watchdog); break;

case VIR_DOMAIN_DEVICE_INPUT: @@ -7457,12 +7458,13 @@ qemuDomainDetachDeviceConfig(virDomainDef *vmdef,

case VIR_DOMAIN_DEVICE_WATCHDOG: - if (!vmdef->watchdog) { + idx = virDomainWatchdogDefFind(vmdef, dev->data.watchdog); + if (idx < 0) { virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("domain has no watchdog")); + _("no matching watchdog was found")); return -1; } - g_clear_pointer(&vmdef->watchdog, virDomainWatchdogDefFree); + VIR_DELETE_ELEMENT(vmdef->watchdogs, idx, vmdef->nwatchdogs); break;

case VIR_DOMAIN_DEVICE_INPUT: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1fa3cc3ea9b1..b9832ea7815a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2928,15 +2928,9 @@ qemuDomainAttachWatchdog(virDomainObj *vm, virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_WATCHDOG, { .watchdog = watchdog } }; g_autoptr(virJSONValue) props = NULL; bool releaseAddress = false; - int rv; + int rv = 0;

- if (vm->def->watchdog) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain already has a watchdog")); - return -1; - } - - qemuAssignDeviceWatchdogAlias(watchdog); + qemuAssignDeviceWatchdogAlias(vm->def, watchdog, -1);

if (watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -2954,10 +2948,13 @@ qemuDomainAttachWatchdog(virDomainObj *vm,

qemuDomainObjEnterMonitor(vm);

- /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then - libvirt listens for the watchdog event, and we perform the dump - ourselves. so convert 'dump' to 'pause' for the qemu cli */ - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { + if (vm->def->nwatchdogs) { + /* Domain already has a watchdog and all must have the same action. */ + rv = 0; + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) { + /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then + libvirt listens for the watchdog event, and we perform the dump + ourselves. so convert 'dump' to 'pause' for the qemu cli */ qemuMonitorActionWatchdog watchdogaction = QEMU_MONITOR_ACTION_WATCHDOG_KEEP;

switch (watchdog->action) { @@ -3015,7 +3012,7 @@ qemuDomainAttachWatchdog(virDomainObj *vm, goto cleanup;

releaseAddress = false; - vm->def->watchdog = watchdog; + VIR_APPEND_ELEMENT_COPY(vm->def->watchdogs, vm->def->nwatchdogs, watchdog); ret = 0;

cleanup: @@ -4848,11 +4845,20 @@ static int qemuDomainRemoveWatchdog(virDomainObj *vm, virDomainWatchdogDef *watchdog) { + size_t i = 0; + VIR_DEBUG("Removing watchdog %s from domain %p %s", watchdog->info.alias, vm, vm->def->name);

+ for (i = 0; i < vm->def->nwatchdogs; i++) { + if (vm->def->watchdogs[i] == watchdog) + break; + } + qemuDomainReleaseDeviceAddress(vm, &watchdog->info); - g_clear_pointer(&vm->def->watchdog, virDomainWatchdogDefFree); + virDomainWatchdogDefFree(watchdog); + VIR_DELETE_ELEMENT(vm->def->watchdogs, i, vm->def->nwatchdogs); + return 0; }

@@ -5603,33 +5609,20 @@ qemuDomainDetachPrepWatchdog(virDomainObj *vm, virDomainWatchdogDef *match, virDomainWatchdogDef **detach) { - virDomainWatchdogDef *watchdog; - - *detach = watchdog = vm->def->watchdog; + ssize_t idx = virDomainWatchdogDefFind(vm->def, match);

- if (!watchdog) { - virReportError(VIR_ERR_DEVICE_MISSING, "%s", - _("watchdog device not present in domain configuration")); + if (idx < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("no matching watchdog was found")); return -1; }

- /* While domains can have up to one watchdog, the one supplied by the user - * doesn't necessarily match the one domain has. Refuse to detach in such - * case. */ - if (!(watchdog->model == match->model && - watchdog->action == match->action && - virDomainDeviceInfoAddressIsEqual(&match->info, &watchdog->info))) { - virReportError(VIR_ERR_DEVICE_MISSING, - _("model '%s' watchdog device not present " - "in domain configuration"), - virDomainWatchdogModelTypeToString(watchdog->model)); - return -1; - } + *detach = vm->def->watchdogs[idx];

- if (watchdog->model != VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { + if ((*detach)->model != VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("hot unplug of watchdog of model %s is not supported"), - virDomainWatchdogModelTypeToString(watchdog->model)); + virDomainWatchdogModelTypeToString((*detach)->model)); return -1; }

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ee9f0784d3a3..218231f3596c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -804,7 +804,8 @@ qemuProcessHandleWatchdog(qemuMonitor *mon G_GNUC_UNUSED, qemuDomainSaveStatus(vm); }

- if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) { + if (vm->def->nwatchdogs && + vm->def->watchdogs[0]->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) { qemuProcessEventSubmit(vm, QEMU_PROCESS_EVENT_WATCHDOG, VIR_DOMAIN_WATCHDOG_ACTION_DUMP, 0, NULL); } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6e04b22da43f..cb002dee0eb9 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1188,6 +1188,28 @@ qemuValidateDomainDefTPMs(const virDomainDef *def) }

+static int +qemuValidateDomainDefWatchdogs(const virDomainDef *def) +{ + ssize_t i = 0; + + for (i = 0; i < def->nwatchdogs; i++) {

Here, you can init i to 1, as ...

+ /* We could theoretically support different watchdogs having dump and + * pause, but let's be honest, we support multiple watchdogs only + * because we need to be able to add a second, implicit one, not because + * it is a brilliant idea to have multiple watchdogs. */ + if (def->watchdogs[i]->action != def->watchdogs[0]->action) {

... for i=0 case, this is never ever gonna be true.

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("watchdogs with different actions are not supported " + "with this QEMU binary")); + return -1; + } + } + + return 0; +} + +

Michal