[PATCH 0/4] qemu: Fix regression wrt virtio-pmem
The first one fixes a regression in which we're unable to start a domain with a virtio-pmem device. It should go into the release. The rest is just a cleanup because the function in question started looking disarranged. Michal Prívozník (4): qemu: Don't error out on 'unknown' memory model in qemuMonitorJSONGetMemoryDeviceInfo() libvirt_private.syms: Export virDomainMemoryModelTypeFromString() qemu_monitor: Switch to virDomainMemoryModel enum in qemuMonitorJSONGetMemoryDeviceInfo() qemu_monitor: Decouple switch()-es in qemuMonitorJSONGetMemoryDeviceInfo() src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 124 +++++++++++++++++++---------------- 2 files changed, 67 insertions(+), 58 deletions(-) -- 2.39.2
When starting QEMU (or when reconnecting to a running one), qemuMonitorJSONGetMemoryDeviceInfo() is called to refresh info on memory devices. In here, query-memory-devices is called which returns info on all memory devices. The result is then iterated over and for some memory models runtime information is updated. The rest is to be ignored. Except, when introducing SGX support, this was turned into an error leaving us unable to start any domain with virtio-pmem memory device (as virtio-pmem is to be ignored). Fixes: ddb1bc051959eef4ad7ed6ac47b57056632bdb5e Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e81b464eea..d76fea8b00 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7243,9 +7243,8 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, return -1; } } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s memory device info is not handled yet"), type); - return -1; + /* type not handled yet */ + continue; } meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); -- 2.39.2
On Mon, Feb 27, 2023 at 12:35 PM Michal Privoznik <mprivozn@redhat.com> wrote:
When starting QEMU (or when reconnecting to a running one), qemuMonitorJSONGetMemoryDeviceInfo() is called to refresh info on memory devices. In here, query-memory-devices is called which returns info on all memory devices. The result is then iterated over and for some memory models runtime information is updated. The rest is to be ignored. Except, when introducing SGX support, this was turned into an error leaving us unable to start any domain with virtio-pmem memory device (as virtio-pmem is to be ignored).
Fixes: ddb1bc051959eef4ad7ed6ac47b57056632bdb5e Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
The virDomainMemoryModelTypeFromString() is not exported, though the enum translation functions are declared in src/conf/domain_conf.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c6c47dbfac..f1bed27ba7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -519,6 +519,7 @@ virDomainMemoryFindByDeviceAlias; virDomainMemoryFindByDeviceInfo; virDomainMemoryFindInactiveByDef; virDomainMemoryInsert; +virDomainMemoryModelTypeFromString; virDomainMemoryModelTypeToString; virDomainMemoryRemove; virDomainMemorySourceTypeFromString; -- 2.39.2
When processing memory devices (as a reply from QEMU), a bunch of STREQ()-s is used. Fortunately, the set of strings we process is the same as virDomainMemoryModel enum. Therefore, we can use virDomainMemoryModelTypeFromString() and when use integer comparison (well, switch()). This has an up side, that introducing a new memory model let's us see immediately at compile time, what places need adjusting. NB, this is in contrast with cmd line generator (qemuBuildMemoryDeviceProps()), where more specific models are generated (e.g. "pc-dimm", "virtio-mem-pci", etc.). But QEMU reports back the parent model, instead of specific child instance. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d76fea8b00..a5e525b7ce 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7211,15 +7211,21 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, g_autofree qemuMonitorMemoryDeviceInfo *meminfo = NULL; virJSONValue *dimminfo; const char *devalias; - const char *type; + const char *modelStr; + int model; - if (!(type = virJSONValueObjectGetString(elem, "type"))) { + if (!(modelStr = virJSONValueObjectGetString(elem, "type"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-memory-devices reply data doesn't contain " "enum type discriminator")); return -1; } + if ((model = virDomainMemoryModelTypeFromString(modelStr)) < 0) { + VIR_WARN("Unknown memory model: %s", modelStr); + continue; + } + if (!(dimminfo = virJSONValueObjectGetObject(elem, "data"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-memory-devices reply data doesn't " @@ -7227,30 +7233,40 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, return -1; } - if (STREQ(type, "dimm") || STREQ(type, "nvdimm") || STREQ(type, "virtio-mem")) { + switch ((virDomainMemoryModel) model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: /* While 'id' attribute is marked as optional in QEMU's QAPI - * specification, Libvirt always sets it. Thus we can fail if not - * present. */ + * specification, Libvirt always sets it. Thus we can fail if not + * present. */ if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("dimm memory info data is missing 'id'")); + _("dimm memory info data is missing 'id'")); return -1; } - } else if (STREQ(type, "sgx-epc")) { + break; + + case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: if (!(devalias = virJSONValueObjectGetString(dimminfo, "memdev"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("sgx-epc memory info data is missing 'memdev'")); + _("sgx-epc memory info data is missing 'memdev'")); return -1; } - } else { + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: /* type not handled yet */ continue; } meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); - /* dimm memory devices */ - if (STREQ(type, "dimm") || STREQ(type, "nvdimm")) { + switch ((virDomainMemoryModel) model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: if (virJSONValueObjectGetNumberUlong(dimminfo, "addr", &meminfo->address) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -7280,16 +7296,18 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, return -1; } + break; - } else if (STREQ(type, "virtio-mem")) { + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: if (virJSONValueObjectGetNumberUlong(dimminfo, "size", &meminfo->size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed/missing size in virtio memory info")); return -1; } - } else if (STREQ(type, "sgx-epc")) { - /* sgx-epc memory devices */ + break; + + case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: if (virJSONValueObjectGetNumberUlong(dimminfo, "memaddr", &meminfo->address) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -7303,7 +7321,11 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, _("malformed/missing size in sgx-epc memory info")); return -1; } - } else { + break; + + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: /* type not handled yet */ continue; } -- 2.39.2
On Mon, Feb 27, 2023 at 12:35 PM Michal Privoznik <mprivozn@redhat.com> wrote:
When processing memory devices (as a reply from QEMU), a bunch of STREQ()-s is used. Fortunately, the set of strings we process is the same as virDomainMemoryModel enum. Therefore, we can use virDomainMemoryModelTypeFromString() and when use integer comparison (well, switch()). This has an up side, that introducing a new memory model let's us see immediately at compile time, what places need adjusting.
NB, this is in contrast with cmd line generator (qemuBuildMemoryDeviceProps()), where more specific models are generated (e.g. "pc-dimm", "virtio-mem-pci", etc.). But QEMU reports back the parent model, instead of specific child instance.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 15 deletions(-)
Seems that I can't compile this patch - compiler is sad that devalias may be used uninitialized: ../src/qemu/qemu_monitor_json.c: In function ‘qemuMonitorJSONGetMemoryDeviceInfo’: ../src/qemu/qemu_monitor_json.c:7333:13: error: ‘devalias’ may be used uninitialized [-Werror=maybe-uninitialized] 7333 | if (virHashAddEntry(info, devalias, meminfo) < 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/qemu/qemu_monitor_json.c:7213:21: note: ‘devalias’ was declared here 7213 | const char *devalias; Using gcc 12.2.1 Kristina
On 2/27/23 13:49, Kristina Hanicova wrote:
On Mon, Feb 27, 2023 at 12:35 PM Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> wrote:
When processing memory devices (as a reply from QEMU), a bunch of STREQ()-s is used. Fortunately, the set of strings we process is the same as virDomainMemoryModel enum. Therefore, we can use virDomainMemoryModelTypeFromString() and when use integer comparison (well, switch()). This has an up side, that introducing a new memory model let's us see immediately at compile time, what places need adjusting.
NB, this is in contrast with cmd line generator (qemuBuildMemoryDeviceProps()), where more specific models are generated (e.g. "pc-dimm", "virtio-mem-pci", etc.). But QEMU reports back the parent model, instead of specific child instance.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> --- src/qemu/qemu_monitor_json.c | 52 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 15 deletions(-)
Seems that I can't compile this patch - compiler is sad that devalias may be used uninitialized:
../src/qemu/qemu_monitor_json.c: In function ‘qemuMonitorJSONGetMemoryDeviceInfo’: ../src/qemu/qemu_monitor_json.c:7333:13: error: ‘devalias’ may be used uninitialized [-Werror=maybe-uninitialized] 7333 | if (virHashAddEntry(info, devalias, meminfo) < 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/qemu/qemu_monitor_json.c:7213:21: note: ‘devalias’ was declared here 7213 | const char *devalias;
Using gcc 12.2.1
Oh right. Thank you for catching this. I'm building with -O0 by default (because I want to be able to debug libvirtd), which apparently enables gcc understand the code. Without it, (maybe) it enables some optimizations which make it incapable of understanding the code. Pity. Anyway, consider this squashed in: diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c index a5e525b7ce..5ed1f9442e 100644 --- i/src/qemu/qemu_monitor_json.c +++ w/src/qemu/qemu_monitor_json.c @@ -7213 +7213 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, - const char *devalias; + const char *devalias = NULL; BTW: clang does not produce such warning. Michal
On Mon, Feb 27, 2023 at 2:04 PM Michal Prívozník <mprivozn@redhat.com> wrote:
On 2/27/23 13:49, Kristina Hanicova wrote:
On Mon, Feb 27, 2023 at 12:35 PM Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> wrote:
When processing memory devices (as a reply from QEMU), a bunch of STREQ()-s is used. Fortunately, the set of strings we process is the same as virDomainMemoryModel enum. Therefore, we can use virDomainMemoryModelTypeFromString() and when use integer
*then
comparison (well, switch()). This has an up side, that introducing a new memory model let's us see immediately at compile time, what places need adjusting.
I would rewrite the last sentence into: "This has an upside: introducing a new memory model lets us see what places need adjusting immediately at compile time."
NB, this is in contrast with cmd line generator (qemuBuildMemoryDeviceProps()), where more specific models are generated (e.g. "pc-dimm", "virtio-mem-pci", etc.). But QEMU reports back the parent model, instead of specific child instance.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> --- src/qemu/qemu_monitor_json.c | 52
+++++++++++++++++++++++++-----------
1 file changed, 37 insertions(+), 15 deletions(-)
Seems that I can't compile this patch - compiler is sad that devalias may be used uninitialized:
../src/qemu/qemu_monitor_json.c: In function ‘qemuMonitorJSONGetMemoryDeviceInfo’: ../src/qemu/qemu_monitor_json.c:7333:13: error: ‘devalias’ may be used uninitialized [-Werror=maybe-uninitialized] 7333 | if (virHashAddEntry(info, devalias, meminfo) < 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/qemu/qemu_monitor_json.c:7213:21: note: ‘devalias’ was declared
here
7213 | const char *devalias;
Using gcc 12.2.1
Oh right. Thank you for catching this. I'm building with -O0 by default (because I want to be able to debug libvirtd), which apparently enables gcc understand the code. Without it, (maybe) it enables some optimizations which make it incapable of understanding the code. Pity. Anyway, consider this squashed in:
diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c index a5e525b7ce..5ed1f9442e 100644 --- i/src/qemu/qemu_monitor_json.c +++ w/src/qemu/qemu_monitor_json.c @@ -7213 +7213 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, - const char *devalias; + const char *devalias = NULL;
In that case, Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
There are two switch() statements over the same variable inside of qemuMonitorJSONGetMemoryDeviceInfo(). Join them together into one switch. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 97 ++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a5e525b7ce..464e1e1822 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7233,6 +7233,8 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, return -1; } + meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); + switch ((virDomainMemoryModel) model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: @@ -7245,6 +7247,46 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, _("dimm memory info data is missing 'id'")); return -1; } + + if (model == VIR_DOMAIN_MEMORY_MODEL_DIMM || + model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (virJSONValueObjectGetNumberUlong(dimminfo, "addr", + &meminfo->address) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing addr in dimm memory info")); + return -1; + } + + if (virJSONValueObjectGetNumberUint(dimminfo, "slot", + &meminfo->slot) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing slot in dimm memory info")); + return -1; + } + + if (virJSONValueObjectGetBoolean(dimminfo, "hotplugged", + &meminfo->hotplugged) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing hotplugged in dimm memory info")); + return -1; + + } + + if (virJSONValueObjectGetBoolean(dimminfo, "hotpluggable", + &meminfo->hotpluggable) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing hotpluggable in dimm memory info")); + return -1; + + } + } else if (model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM) { + if (virJSONValueObjectGetNumberUlong(dimminfo, "size", + &meminfo->size) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing size in virtio memory info")); + return -1; + } + } break; case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: @@ -7253,61 +7295,6 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, _("sgx-epc memory info data is missing 'memdev'")); return -1; } - break; - - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM: - case VIR_DOMAIN_MEMORY_MODEL_NONE: - case VIR_DOMAIN_MEMORY_MODEL_LAST: - /* type not handled yet */ - continue; - } - - meminfo = g_new0(qemuMonitorMemoryDeviceInfo, 1); - - switch ((virDomainMemoryModel) model) { - case VIR_DOMAIN_MEMORY_MODEL_DIMM: - case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: - if (virJSONValueObjectGetNumberUlong(dimminfo, "addr", - &meminfo->address) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed/missing addr in dimm memory info")); - return -1; - } - - if (virJSONValueObjectGetNumberUint(dimminfo, "slot", - &meminfo->slot) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed/missing slot in dimm memory info")); - return -1; - } - - if (virJSONValueObjectGetBoolean(dimminfo, "hotplugged", - &meminfo->hotplugged) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed/missing hotplugged in dimm memory info")); - return -1; - - } - - if (virJSONValueObjectGetBoolean(dimminfo, "hotpluggable", - &meminfo->hotpluggable) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed/missing hotpluggable in dimm memory info")); - return -1; - - } - break; - - case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: - if (virJSONValueObjectGetNumberUlong(dimminfo, "size", - &meminfo->size) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed/missing size in virtio memory info")); - return -1; - } - break; - - case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: if (virJSONValueObjectGetNumberUlong(dimminfo, "memaddr", &meminfo->address) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.39.2
On Mon, Feb 27, 2023 at 12:35 PM Michal Privoznik <mprivozn@redhat.com> wrote:
The first one fixes a regression in which we're unable to start a domain with a virtio-pmem device. It should go into the release. The rest is just a cleanup because the function in question started looking disarranged.
Michal Prívozník (4): qemu: Don't error out on 'unknown' memory model in qemuMonitorJSONGetMemoryDeviceInfo() libvirt_private.syms: Export virDomainMemoryModelTypeFromString() qemu_monitor: Switch to virDomainMemoryModel enum in qemuMonitorJSONGetMemoryDeviceInfo() qemu_monitor: Decouple switch()-es in qemuMonitorJSONGetMemoryDeviceInfo()
src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 124 +++++++++++++++++++---------------- 2 files changed, 67 insertions(+), 58 deletions(-)
-- 2.39.2
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
participants (3)
-
Kristina Hanicova -
Michal Privoznik -
Michal Prívozník