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

[Peter Krempa]

On Thu, Jan 14, 2021 at 13:37:24 +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
> 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>
> ---
>  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
> @@ -2950,7 +2950,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>                              qemuDomainObjPrivatePtr priv,
>                              const virDomainDef *def,
>                              const virDomainMemoryDef *mem,
> -                            bool force)
> +                            bool force,
> +                            bool systemMemory)
>  {
>      const char *backendType = "memory-backend-file";
>      virDomainNumatuneMemMode mode;
> @@ -2967,6 +2968,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>      bool needHugepage = !!pagesize;
>      bool useHugepage = !!pagesize;
>      int discard = mem->discard;
> +    bool useCanonicalPath = true;

Preferably invert this to 'disableCanonicalPath' and add a comment which
will point to the explanatiol below, so that you save potential readers
a re-read once they find the note about the hack below.

/* Disabling canonical path is required for migration compatibility of
 * system memory objects, see below */

> @@ -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'.

Add:

Note that QEMU consideres 'x-use-canonical-path-for-ramblock-id' stable
and supported despite the 'x-' prefix.

> +     * 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)

The qemu documentation calls for enabling this property rather than
disabling it as the libvirt code does:

+        b) for machine types 4.0 and older, user shall
+        use ``x-use-canonical-path-for-ramblock-id=on`` backend option,
+        if migration to/from old QEMU (<5.0) is expected.
+        For example:
+        ::
+        -object memory-backend-ram,id=pc.ram,size=512M,x-use-canonical-path-for-ramblock-id=on

See qapi_bool_parse, which parses 'on' and 'yes' as identical values.

Also the at least the commit message should mention why it's okay to use
it also for other machine types and newer qemu despite the documentation
in qemu.