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

[Jiri Denemark]

On Tue, Jan 12, 2021 at 20:21:19 +0100, Igor Mammedov wrote:
> On Tue, 12 Jan 2021 09:29:50 +0100
> Michal Privoznik <mprivozn at redhat.com> wrote:
> 
> > In commit v6.9.0-rc1~450 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 v5.0.0-rc0~75^2~1^2~76 and
> > look at function create_default_memdev().
> > 
> > However, then commit v5.0.0-rc1~11^2~3 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 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.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912201
> > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > ---
> > 
> > I'll replace both occurrences of 'QEMU commit XXX' once QEMU patch is
> > merged.
> > 
> >  src/qemu/qemu_command.c                       | 30 ++++++++++++++++---
> >  src/qemu/qemu_command.h                       |  3 +-
> >  src/qemu/qemu_hotplug.c                       |  2 +-
> >  .../hugepages-memaccess3.x86_64-latest.args   |  4 +--
> >  4 files changed, 31 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 6f970a3128..b99d4e5faf 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
...
> > @@ -3122,10 +3127,27 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
> >  
> >          if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0)
> >              return -1;
> > +
> > +        if (systemMemory)
> > +            useCanonicalPath = false;
> > +
> >      } 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'.
> > +     * See QEMU commit XXX.
> > +     */
> > +    if (!useCanonicalPath &&
> > +        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;
> 
> is it possible to do it only for old machine types <= 4.0, to limit hack exposure?

We consume machine types as opaque strings, we don't parse them and thus
we don't have any ordering on them. Without this telling what <= 4.0
means is impossible. And I don't think we should start doing it, and
especially not for limiting this hack as it would be limiting a hack
with another one.

A reasonable solution would be if we could tell which machine types need
(or perhaps don't need) such treatment by probing QEMU for available
machine types.

Jirka