Re: [PATCH v3] machine: add missing doc for memory-backend option

[Igor Mammedov]

On Wed, 27 Jan 2021 18:03:27 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/27/21 4:35 PM, Igor Mammedov wrote:
> > On Wed, 27 Jan 2021 15:24:26 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> On 1/27/21 11:54 AM, Daniel P. Berrangé wrote:  
> >>> On Wed, Jan 27, 2021 at 10:45:11AM +0000, Daniel P. Berrangé wrote:  
> >>>> On Thu, Jan 21, 2021 at 11:15:04AM -0500, Igor Mammedov wrote:  
> >>
> >>  
> >>>>
> >>>> How does a mgmt app know which machine types need to use this
> >>>> option ? The machine type names are opaque strings, and apps
> >>>> must not attempt to parse or interpret the version number
> >>>> inside the machine type name, as they can be changed by
> >>>> distros.  IOW, saying to use it for machine types 4.0 and
> >>>> older isn't a valid usage strategy IMHO.  
> > it's possible (but no necessary) to use knob with new machine types
> > (defaults for these match suggested property value).
> > Limiting knob usage to 4.0 and older would allow us to drop
> > without extra efforts once 4.0 is deprecated/removed.  
> 
> Problem here is that libvirt treats machine type as an opaque string. 
> Therefore, as could be seen in my patch for libvirt, the property is 
> disabled for all started VMs, regardless of machine type:
> 
> https://www.redhat.com/archives/libvir-list/2021-January/msg00686.html
> 
> So it can't really go away ever, can it?
> 
> >   
> >>> Looking at the libvirt patch, we do indeed use his property
> >>> unconditionally for all machine types, precisely because parsing
> >>> version numbers from the machine type is not allowed.
> >>>
> >>> https://www.redhat.com/archives/libvir-list/2021-January/msg00633.html
> >>>
> >>> So this doc is telling apps to do something that isn't viable  
> >>
> >> The other approach that I was suggesting was, that QEMU stops reporting
> >> 'default-ram-id' for affected machine types. The way the switch from '-m
> >> XMB' to memory-backend-* was implemented in libvirt is that if libvirt
> >> sees 'default-ram-id' attribute for given machine type it uses
> >> memory-backend-* otherwise it falls back to -m.
> >>
> >> Since we know which machine types are "broken", we can stop reporting
> >> the attribute and thus stop tickling this bug. I agree that it puts more
> >> burden on distro maintainers to backport the change, but I think it's
> >> acceptable risk.  
> > 
> > default-ram-id is already exposed in wild including old machine types
> > starting from 5.2  
> 
> It is, but according to qapi/machine.json it is optional. Mgmt apps have 
> to be able to deal with it missing.
> 
> > 
> > if libvirt will take care this one quirk, then I guess we can
> > do as suggested. I can post an additional patch to this effect if there
> > is agreement to go this route.  
> 
> The beauty of this solution is that libvirt wouldn't need to do anything 
> :-)  As I said earlier, if no default-ram-id is found then libvirt falls 
> back to '-m X'.
> 
> I've cooked a dirty patch that works in my testing:
we can discuss this separately from the current topic as it doesn't
affect libvirt side directly.
I'm still not sure we actually need it,
since 5.2 will expose default_ram_id for all machine types, and who knows
what other users do with it (aside libvirt).
It looks to me that following usual deprecation process is simpler,
than adding hacks on top of existing ones.

> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index affffe0c4a..2214782d72 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -238,8 +238,33 @@ MachineInfoList *qmp_query_machines(Error **errp)
>               info->has_default_cpu_type = true;
>           }
>           if (mc->default_ram_id) {
> -            info->default_ram_id = g_strdup(mc->default_ram_id);
> -            info->has_default_ram_id = true;
> +            int i;
> +            bool broken = false;
> +
> +            /* Default RAM ID is broken if 
> x-use-canonical-path-for-ramblock-id
> +             * property of memory-backend is on. That's why it's 
> disabled in
> +             * create_default_memdev(). However, some machine types 
> turn it on
> +             * for backwards compatibility. */
> +            for (i = 0; i < mc->compat_props->len; i++) {
> +                GlobalProperty *p = g_ptr_array_index(mc->compat_props, i);
> +
> +                if (strcmp(p->driver, TYPE_MEMORY_BACKEND_FILE) != 0)
> +                    continue;
> +
> +                if (strcmp(p->property, 
> "x-use-canonical-path-for-ramblock-id") != 0)
> +                    continue;
> +
> +                if (strcmp(p->value, "true") != 0)
> +                    continue;
> +
> +                broken = true;
> +                break;
> +            }
> +
> +            if (!broken) {
> +                info->default_ram_id = g_strdup(mc->default_ram_id);
> +                info->has_default_ram_id = true;
> +            }
>           }
> 
>           QAPI_LIST_PREPEND(mach_list, info);
> 
> 
> Michal
> 
>