Re: [PATCH v3 2/2] qemu: Do not Use canonical path for system memory

On Wed, Feb 10, 2021 at 14:38:14 +0100, Michal Privoznik wrote: > In commit 88957116c9d3cb4705380c3702c9d4315fb500bb I've adapted > libvirt to QEMU's deprecation of -mem-path and -mem-prealloc and > switched to memory-backend-* even for system memory. My claim was > that that's what QEMU does under the hood anyway. And indeed it > was: see QEMU commit 900c0ba373aada4c13d47d95330aa72ec4067ba5 and > look at function create_default_memdev(). > > However, then commit d96c4d5f193e0e45beec80a6277728b32875bddb was > merged into QEMU. While it was fixing a bug, it also changed the > create_default_memdev() function in which it started turning off > use of canonical path (by setting > "x-use-canonical-path-for-ramblock-id" attribute to false). This > wasn't documented until QEMU commit XXX. The path affects s/XXX/actual commit id/ > migration - the same path has to be used on the source and on the > destination. Therefore, if there is old guest started with '-m X' > it has "pc.ram" block which doesn't use canonical path and thus > when migrating to newer QEMU which uses memory-backend-* we have > to turn off the canonical path explicitly. Otherwise, > "/objects/pc.ram" path would be expected by QEMU which doesn't > match the source. > > Ideally, we would need to set it only for some machine types > (4.0 and older) because newer machine types already do what we > are doing. However, we treat machine types as opaque strings and > therefore we don't want to parse nor inspect their versions. But > then again, newer machine types already do what we are doing in > this commit, so when old machine types are deprecated and removed > we can remove our hack and forget it ever happened. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912201 > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 35 ++++++++++++++++--- > src/qemu/qemu_command.h | 3 +- > src/qemu/qemu_hotplug.c | 2 +- > .../disk-vhostuser.x86_64-latest.args | 3 +- > .../hugepages-memaccess3.x86_64-latest.args | 4 +-- > 5 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 92036d26c0..699d89de1d 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c [...] > @@ -3148,10 +3156,29 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps, > > if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0) > return -1; > + > + if (systemMemory) > + disableCanonicalPath = true; > + > } else { > backendType = "memory-backend-ram"; > } > > + /* This is a terrible hack, but unfortunately there is no better way. > + * The replacement for '-m X' argument is not simple '-machine > + * memory-backend' and '-object memory-backend-*,size=X' (which was the > + * idea). This is because of create_default_memdev() in QEMU sets > + * 'x-use-canonical-path-for-ramblock-id' attribute to false and is > + * documented in QEMU in qemu-options.hx under 'memory-backend'. Note > + * that QEMU consideres 'x-use-canonical-path-for-ramblock-id' stable > + * and supported despite the 'x-' prefix. > + * See QEMU commit 8db0b20415c129cf5e577a593a4a0372d90b7cc9. > + */ > + if (disableCanonicalPath && > + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_X_USE_CANONICAL_PATH_FOR_RAMBLOCK_ID) && > + virJSONValueObjectAdd(props, "b:x-use-canonical-path-for-ramblock-id", false, NULL) < 0) > + return -1; I don't remember whether you and Daniel reached some consensus about the machine type version check, but if the consensus is to always output it then okay.

From my side: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;