[PATCH] qemu: Don't generate '-machine memory-backend' and '-numa memdev'

[Peter Krempa]

On Tue, Oct 06, 2020 at 10:03:29 +0200, Michal Privoznik wrote:
> In 88957116c9 I've switched to -machine memory-backend=ID and
> -object memory-backend-* because QEMU is obsoleting -mem-path
> and -mem-prealloc. However, what I did not foresee was that using
> -machine memory-backend in combination with -numa is not allowed
> in QEMU. This was reported upstream and fortunately not released
> yet.
> 
> The solution is to generate -machine memory-backend=ID iff there

s/iff/only if/

> are no guest NUMA nodes, and require use of -numa memdev= for new
> enough QEMUs.

To me it's not obvious enough from this commit message that the root of
the problem is that for NUMA configurations we already specify the
memory via memory-backend thus actually adding another memory backend
for non-numa config would be wrong anyways.

> Reported-by: Masayoshi Mizuma <msys.mizuma at gmail.com>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_command.c                               | 11 +++++++++--
>  .../hugepages-nvdimm.x86_64-latest.args               |  6 +-----
>  .../memfd-memory-default-hugepage.x86_64-latest.args  |  5 +----
>  .../memfd-memory-numa.x86_64-latest.args              |  5 +----
>  .../memory-hotplug-nvdimm-access.x86_64-latest.args   |  4 +---
>  .../memory-hotplug-nvdimm-align.x86_64-latest.args    |  4 +---
>  .../memory-hotplug-nvdimm-label.x86_64-latest.args    |  4 +---
>  .../memory-hotplug-nvdimm-pmem.x86_64-latest.args     |  4 +---
>  .../memory-hotplug-nvdimm-readonly.x86_64-latest.args |  4 +---
>  .../memory-hotplug-nvdimm.x86_64-latest.args          |  4 +---
>  .../qemuxml2argvdata/numatune-hmat.x86_64-latest.args |  4 +---
>  .../vhost-user-fs-fd-memory.x86_64-latest.args        |  4 +---
>  .../vhost-user-fs-hugepages.x86_64-latest.args        |  5 +----
>  .../vhost-user-gpu-secondary.x86_64-latest.args       |  3 +--
>  .../vhost-user-vga.x86_64-latest.args                 |  3 +--
>  15 files changed, 23 insertions(+), 47 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 476cf6972e..def79d2f20 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7049,7 +7049,8 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>      defaultRAMid = virQEMUCapsGetMachineDefaultRAMid(qemuCaps,
>                                                       def->virtType,
>                                                       def->os.machine);
> -    if (defaultRAMid)
> +    if (defaultRAMid &&
> +        !virDomainNumaGetNodeCount(def->numa))
>          virBufferAsprintf(&buf, ",memory-backend=%s", defaultRAMid);
>  
>      virCommandAddArgBuffer(cmd, &buf);

IMO this is starting to be too arcane. Since 'defaultRAMid' is used to
gate the formatting of the command line attribute and it's used only
there you can move the !virDomainNumaGetNodeCount condition and just
don't query the defaultRAMid.

Also please add a comment describing what is happening, especially why
the numa nodes influence this.


> @@ -7216,7 +7217,8 @@ qemuBuildMemCommandLine(virCommandPtr cmd,
>                                                       def->os.machine);
>  
>      if (defaultRAMid) {
> -        qemuBuildMemCommandLineMemoryDefaultBackend(cmd, def, priv, defaultRAMid);
> +        if (!virDomainNumaGetNodeCount(def->numa))
> +            qemuBuildMemCommandLineMemoryDefaultBackend(cmd, def, priv, defaultRAMid);
>      } else {
>          if (def->mem.allocation == VIR_DOMAIN_MEMORY_ALLOCATION_IMMEDIATE) {
>              virCommandAddArgList(cmd, "-mem-prealloc", NULL);

Same here.

> @@ -7428,6 +7430,11 @@ qemuBuildNumaCommandLine(virQEMUDriverConfigPtr cfg,
>          hmat = true;
>      }
>  
> +    if (virQEMUCapsGetMachineDefaultRAMid(qemuCaps,
> +                                          def->virtType,
> +                                          def->os.machine))
> +        needBackend = true;

Does this even make sense to have here? In case we are formatting NUMA
we should stick to the rules about NUMA node formatting and this doesn't
seem to fit.

Aditionally I'd expect that it's redundant since the condition involving
virQEMUCapsGetMachineNumaMemSupported will already assert needBackend
for any modern qemu.

If it makes sense please make sure to note it down.

> +
>      nodeBackends = g_new0(virBuffer, ncells);
>  
>      /* using of -numa memdev= cannot be combined with -numa mem=, thus we