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

[Michal Privoznik]

On 2/10/21 5:10 PM, Peter Krempa wrote:
> On Wed, Feb 10, 2021 at 14:38:14 +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
> 
> s/XXX/actual commit id/
> 
>> 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.
>>
>> Ideally, we would need to set it only for some machine types
>> (4.0 and older) because newer machine types already do what we
>> are doing. However, we treat machine types as opaque strings and
>> therefore we don't want to parse nor inspect their versions. But
>> then again, newer machine types already do what we are doing in
>> this commit, so when old machine types are deprecated and removed
>> we can remove our hack and forget it ever happened.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912201
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_command.c                       | 35 ++++++++++++++++---
>>   src/qemu/qemu_command.h                       |  3 +-
>>   src/qemu/qemu_hotplug.c                       |  2 +-
>>   .../disk-vhostuser.x86_64-latest.args         |  3 +-
>>   .../hugepages-memaccess3.x86_64-latest.args   |  4 +--
>>   5 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 92036d26c0..699d89de1d 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
> 
> [...]
> 
>> @@ -3148,10 +3156,29 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>>   
>>           if (qemuBuildMemoryBackendPropsShare(props, memAccess) < 0)
>>               return -1;
>> +
>> +        if (systemMemory)
>> +            disableCanonicalPath = true;
>> +
>>       } 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'. Note
>> +     * that QEMU consideres 'x-use-canonical-path-for-ramblock-id' stable
>> +     * and supported despite the 'x-' prefix.
>> +     * See QEMU commit 8db0b20415c129cf5e577a593a4a0372d90b7cc9.
>> +     */
>> +    if (disableCanonicalPath &&
>> +        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;
> 
> I don't remember whether you and Daniel reached some consensus about the
> machine type version check, but if the consensus is to always output it
> then okay.

Yeah, this is the consensus. The reason is that distros may introduce 
their own machine types with version appearing at unexpected place (or 
not appearing at all). Also, finding an ordering function is not trivial 
and we also would need to know which machine types have the feature 
turned on/off by default. Since QEMU claims it is safe to turn the 
feature off at all times, let's not bother with parsing anything and 
just turn it off always.

> 
>  From my side:
> 
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> 

Thanks, pushed.

Michal