[PATCH] qemu: add support for max-ram-below-4g option

[Peter Krempa]

On Sun, Apr 25, 2021 at 17:33:31 +0800, Zhiyong Ye wrote:
> Limit the amount of ram below 4G. This can increase the address space
> used by PCI devices below 4G and it can be used by adding attributes in
> XML like this:
> <domain>
>   ...
>   <memory unit="MiB" below4g="2048">4096</memory>

This illustrates that sharing the 'unit' argument between 'below4g' and
the actual memory size is wrong (pointed out also below). In case the
user modifies the unit but doesn't change below4g value it may be
misrepresented and since below4g is new some tools might not actually
know how to handle that.

below4g must either be by default in kiB, or have an own unit (which
will probably be ugly).

>   ...
> </domain>
> 
> Signed-off-by: Zhiyong Ye <yezhiyong at bytedance.com>
> Signed-off-by: zhenwei pi <pizhenwei at bytedance.com>
> Signed-off-by: zhangruien <zhangruien at bytedance.com>
> ---
>  src/conf/domain_conf.c  | 19 +++++++++++++++++++
>  src/conf/domain_conf.h  |  3 +++
>  src/qemu/qemu_command.c |  4 ++++

Missing docs (doc/formatdomain.rst) and test changes. Also we usually
don't mix conf and qemu changes.

You need at least a test case for qemuxml2argvtest and qemuxml2xmltest.

The conf change is also missing a check in the ABI stability checking
code.


>  3 files changed, 26 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a72d58f488..c211a69ed1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4720,6 +4720,19 @@ virDomainDefPostParseMemory(virDomainDef *def,
>          return -1;
>      }
>  
> +    if (def->mem.max_ram_below_4g &&
> +        def->mem.max_ram_below_4g < (1ULL << 10)) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("maximum memory size below the 4GiB boundary is too small, "
> +                         "BIOS may not work with less than 1MiB"));

"may not work" seems like you picked an arbitrary limit, and the same
way it may not work with more than 1MiB. In cases when there isn't a
strict technical limit, but rather "it usually doesn't work" like this
the check is almost pointless.

> +        return -1;
> +    } else if (def->mem.max_ram_below_4g > (1ULL << 22)) {

we usually prefer to describe the limit as 4 * 1024 * 1024

> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("maximum memory size below the 4GiB boundary must be "
> +                         "less than or equal to 4GiB"));

don't break up error messages into multiple lines

> +        return -1;

This belongs to conf/domain_validate.c

You'll need to add a new function e.g. 'virDomainDefValidateMemory' call
it from virDomainDefValidateInternal and add those checks there.

> +    }
> +
>      return 0;
>  }
>  
> @@ -19786,6 +19799,10 @@ virDomainDefParseMemory(virDomainDef *def,
>                               &def->mem.max_memory, false, false) < 0)
>          goto error;
>  
> +    if (virDomainParseMemory("./memory[1]/@below4g", "./memory[1]/@unit", ctxt,
> +                             &def->mem.max_ram_below_4g, false, true) < 0)

This uses the 'unit' which is used for memory for the 'below4g'
attribute.


> +        goto error;
> +
>      if (virXPathUInt("string(./maxMemory[1]/@slots)", ctxt, &def->mem.memory_slots) == -2) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("Failed to parse memory slot count"));
> @@ -28844,6 +28861,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>      if (def->mem.dump_core)
>          virBufferAsprintf(buf, " dumpCore='%s'",
>                            virTristateSwitchTypeToString(def->mem.dump_core));
> +    if (def->mem.max_ram_below_4g > 0)
> +        virBufferAsprintf(buf, " below4g='%llu'", def->mem.max_ram_below_4g);
>      virBufferAsprintf(buf, " unit='KiB'>%llu</memory>\n",
>                        virDomainDefGetMemoryTotal(def));
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4838687edf..a939d43e93 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2597,6 +2597,9 @@ struct _virDomainMemtune {
>      unsigned long long max_memory; /* in kibibytes */
>      unsigned int memory_slots; /* maximum count of RAM memory slots */
>  
> +    /* maximum memory below the 4GiB boundary (32bit boundary) */
> +    unsigned long long max_ram_below_4g; /* in kibibytes */
> +
>      bool nosharepages;
>      bool locked;
>      int dump_core; /* enum virTristateSwitch */
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index be93182092..c69ad781e6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6961,6 +6961,10 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>                            cfg->dumpGuestCore ? "on" : "off");
>      }
>  
> +    if (def->mem.max_ram_below_4g > 0)
> +        virBufferAsprintf(&buf, ",max-ram-below-4g=%llu",
> +                          def->mem.max_ram_below_4g * 1024);
> +
>      if (def->mem.nosharepages)
>          virBufferAddLit(&buf, ",mem-merge=off");
>  
> -- 
> 2.24.3 (Apple Git-128)
>