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

[Zhiyong Ye]

I'm sorry that the format of the last email seems to be problematic, so 
I sent it again.

On 4/26/21 3:55 PM, Peter Krempa wrote:
> 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).
> 

So, how should we place 'blow4g'? I have summarized that we may have two 
ways:

The first way is to put 'below4g' in the existing element, as Daniel 
suggested, that 'below4g' does not have its own 'unit' and let it 
default to KiB, e.g, <memory unit="MiB" below4g="2048"> 4096 </memory>

The second way is to add a new element and 'below4g' has its own 'unit', 
such as:
<memory unit='MiB'> 4096 </memory>
<below4gMemory unit='MiB'> 2048 </below4gMemory>

>>    ...
>> </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.
> 

This is seen from QEMU code. When it is less than 1MiB, QEMU cannot 
create a new virtual machine.

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