diff mbox series

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

Message ID 20210121161504.1007247-1-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v3] machine: add missing doc for memory-backend option | expand

Commit Message

Igor Mammedov Jan. 21, 2021, 4:15 p.m. UTC
Add documentation for '-machine memory-backend' CLI option and
how to use it.

And document that x-use-canonical-path-for-ramblock-id,
is considered to be stable to make sure it won't go away by accident.

x- was intended for unstable/iternal properties, and not supposed to
be stable option. However it's too late to rename (drop x-)
it as it would mean that users will have to mantain both
x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
and prefix-less for later versions.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 - add doc that x-use-canonical-path-for-ramblock-id is considered stable,
     (Peter Krempa <pkrempa@redhat.com>)
v3:
 - s/x-use-canonical-path-for-ramblock-id=on/x-use-canonical-path-for-ramblock-id=off/
     (Michal Privoznik <mprivozn@redhat.com>)
 - add to commit message why x- prefix is preserved
 - drop clause about x-use-canonical-path-for-ramblock-id being stable
   from help section, but keep it in code comment above
   x-use-canonical-path-for-ramblock-id property. It's sufficient
   to prevent option being changed/removed by accident.
     (Peter Maydell <peter.maydell@linaro.org>)
---
 backends/hostmem.c | 10 ++++++++++
 qemu-options.hx    | 26 +++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

Comments

Michal Privoznik Jan. 27, 2021, 10:40 a.m. UTC | #1
On 1/21/21 5:15 PM, Igor Mammedov wrote:
> Add documentation for '-machine memory-backend' CLI option and
> how to use it.
> 
> And document that x-use-canonical-path-for-ramblock-id,
> is considered to be stable to make sure it won't go away by accident.
> 
> x- was intended for unstable/iternal properties, and not supposed to
> be stable option. However it's too late to rename (drop x-)
> it as it would mean that users will have to mantain both
> x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
> and prefix-less for later versions.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - add doc that x-use-canonical-path-for-ramblock-id is considered stable,
>       (Peter Krempa <pkrempa@redhat.com>)
> v3:
>   - s/x-use-canonical-path-for-ramblock-id=on/x-use-canonical-path-for-ramblock-id=off/
>       (Michal Privoznik <mprivozn@redhat.com>)
>   - add to commit message why x- prefix is preserved
>   - drop clause about x-use-canonical-path-for-ramblock-id being stable
>     from help section, but keep it in code comment above
>     x-use-canonical-path-for-ramblock-id property. It's sufficient
>     to prevent option being changed/removed by accident.
>       (Peter Maydell <peter.maydell@linaro.org>)
> ---
>   backends/hostmem.c | 10 ++++++++++
>   qemu-options.hx    | 26 +++++++++++++++++++++++++-
>   2 files changed, 35 insertions(+), 1 deletion(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Daniel P. Berrangé Jan. 27, 2021, 10:45 a.m. UTC | #2
On Thu, Jan 21, 2021 at 11:15:04AM -0500, Igor Mammedov wrote:
> Add documentation for '-machine memory-backend' CLI option and
> how to use it.
> 
> And document that x-use-canonical-path-for-ramblock-id,
> is considered to be stable to make sure it won't go away by accident.
> 
> x- was intended for unstable/iternal properties, and not supposed to
> be stable option. However it's too late to rename (drop x-)
> it as it would mean that users will have to mantain both
> x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
> and prefix-less for later versions.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>  - add doc that x-use-canonical-path-for-ramblock-id is considered stable,
>      (Peter Krempa <pkrempa@redhat.com>)
> v3:
>  - s/x-use-canonical-path-for-ramblock-id=on/x-use-canonical-path-for-ramblock-id=off/
>      (Michal Privoznik <mprivozn@redhat.com>)
>  - add to commit message why x- prefix is preserved
>  - drop clause about x-use-canonical-path-for-ramblock-id being stable
>    from help section, but keep it in code comment above
>    x-use-canonical-path-for-ramblock-id property. It's sufficient
>    to prevent option being changed/removed by accident.
>      (Peter Maydell <peter.maydell@linaro.org>)
> ---
>  backends/hostmem.c | 10 ++++++++++
>  qemu-options.hx    | 26 +++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 9f9ac95edd..813aeb83c9 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -498,6 +498,16 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
>          host_memory_backend_get_share, host_memory_backend_set_share);
>      object_class_property_set_description(oc, "share",
>          "Mark the memory as private to QEMU or shared");
> +    /*
> +     * Do not delete/rename option. This option must be considered stable
> +     * (as if it didn't have the 'x-' prefix including deprecation period) as
> +     * long as 4.0 and older machine types exists.
> +     * Option will be used by upper layers to override (disable) canonical path
> +     * for ramblock-id set by compat properties on old machine types ( <= 4.0),
> +     * to keep migration working when backend is used for main RAM with
> +     * -machine memory-backend= option (main RAM historically used prefix-less
> +     * ramblock-id).
> +     */
>      object_class_property_add_bool(oc, "x-use-canonical-path-for-ramblock-id",
>          host_memory_backend_get_use_canonical_path,
>          host_memory_backend_set_use_canonical_path);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 62791f56d8..059b1a1d14 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>      "                nvdimm=on|off controls NVDIMM support (default=off)\n"
>      "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
> -    "                hmat=on|off controls ACPI HMAT support (default=off)\n",
> +    "                hmat=on|off controls ACPI HMAT support (default=off)\n"
> +    "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n",
>      QEMU_ARCH_ALL)
>  SRST
>  ``-machine [type=]name[,prop=value[,...]]``
> @@ -96,6 +97,29 @@ SRST
>      ``hmat=on|off``
>          Enables or disables ACPI Heterogeneous Memory Attribute Table
>          (HMAT) support. The default is off.
> +
> +     ``memory-backend='id'``
> +        An alternative to legacy ``-mem-path`` and ``mem-prealloc`` options.
> +        Allows to use a memory backend as main RAM.
> +
> +        For example:
> +        ::
> +        -object memory-backend-file,id=pc.ram,size=512M,mem-path=/hugetlbfs,prealloc=on,share=on
> +        -machine memory-backend=pc.ram
> +        -m 512M
> +
> +        Migration compatibility note:
> +        a) as backend id one shall use value of 'default-ram-id', advertised by
> +        machine type (available via ``query-machines`` QMP command), if migration
> +        to/from old QEMU (<5.0) is expected.
> +        b) for machine types 4.0 and older, user shall
> +        use ``x-use-canonical-path-for-ramblock-id=off`` backend option
> +        if migration to/from old QEMU (<5.0) is expected.

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.

> +        For example:
> +        ::
> +        -object memory-backend-ram,id=pc.ram,size=512M,x-use-canonical-path-for-ramblock-id=off
> +        -machine memory-backend=pc.ram
> +        -m 512M
>  ERST
>  

Regards,
Daniel
Daniel P. Berrangé Jan. 27, 2021, 10:54 a.m. UTC | #3
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:
> > Add documentation for '-machine memory-backend' CLI option and
> > how to use it.
> > 
> > And document that x-use-canonical-path-for-ramblock-id,
> > is considered to be stable to make sure it won't go away by accident.
> > 
> > x- was intended for unstable/iternal properties, and not supposed to
> > be stable option. However it's too late to rename (drop x-)
> > it as it would mean that users will have to mantain both
> > x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions
> > and prefix-less for later versions.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >  - add doc that x-use-canonical-path-for-ramblock-id is considered stable,
> >      (Peter Krempa <pkrempa@redhat.com>)
> > v3:
> >  - s/x-use-canonical-path-for-ramblock-id=on/x-use-canonical-path-for-ramblock-id=off/
> >      (Michal Privoznik <mprivozn@redhat.com>)
> >  - add to commit message why x- prefix is preserved
> >  - drop clause about x-use-canonical-path-for-ramblock-id being stable
> >    from help section, but keep it in code comment above
> >    x-use-canonical-path-for-ramblock-id property. It's sufficient
> >    to prevent option being changed/removed by accident.
> >      (Peter Maydell <peter.maydell@linaro.org>)
> > ---
> >  backends/hostmem.c | 10 ++++++++++
> >  qemu-options.hx    | 26 +++++++++++++++++++++++++-
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index 9f9ac95edd..813aeb83c9 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -498,6 +498,16 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
> >          host_memory_backend_get_share, host_memory_backend_set_share);
> >      object_class_property_set_description(oc, "share",
> >          "Mark the memory as private to QEMU or shared");
> > +    /*
> > +     * Do not delete/rename option. This option must be considered stable
> > +     * (as if it didn't have the 'x-' prefix including deprecation period) as
> > +     * long as 4.0 and older machine types exists.
> > +     * Option will be used by upper layers to override (disable) canonical path
> > +     * for ramblock-id set by compat properties on old machine types ( <= 4.0),
> > +     * to keep migration working when backend is used for main RAM with
> > +     * -machine memory-backend= option (main RAM historically used prefix-less
> > +     * ramblock-id).
> > +     */
> >      object_class_property_add_bool(oc, "x-use-canonical-path-for-ramblock-id",
> >          host_memory_backend_get_use_canonical_path,
> >          host_memory_backend_set_use_canonical_path);
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 62791f56d8..059b1a1d14 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> >      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
> >      "                nvdimm=on|off controls NVDIMM support (default=off)\n"
> >      "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
> > -    "                hmat=on|off controls ACPI HMAT support (default=off)\n",
> > +    "                hmat=on|off controls ACPI HMAT support (default=off)\n"
> > +    "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n",
> >      QEMU_ARCH_ALL)
> >  SRST
> >  ``-machine [type=]name[,prop=value[,...]]``
> > @@ -96,6 +97,29 @@ SRST
> >      ``hmat=on|off``
> >          Enables or disables ACPI Heterogeneous Memory Attribute Table
> >          (HMAT) support. The default is off.
> > +
> > +     ``memory-backend='id'``
> > +        An alternative to legacy ``-mem-path`` and ``mem-prealloc`` options.
> > +        Allows to use a memory backend as main RAM.
> > +
> > +        For example:
> > +        ::
> > +        -object memory-backend-file,id=pc.ram,size=512M,mem-path=/hugetlbfs,prealloc=on,share=on
> > +        -machine memory-backend=pc.ram
> > +        -m 512M
> > +
> > +        Migration compatibility note:
> > +        a) as backend id one shall use value of 'default-ram-id', advertised by
> > +        machine type (available via ``query-machines`` QMP command), if migration
> > +        to/from old QEMU (<5.0) is expected.
> > +        b) for machine types 4.0 and older, user shall
> > +        use ``x-use-canonical-path-for-ramblock-id=off`` backend option
> > +        if migration to/from old QEMU (<5.0) is expected.
> 
> 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.

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


Regards,
Daniel
Michal Privoznik Jan. 27, 2021, 2:24 p.m. UTC | #4
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.
> 
> 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.

Michal
Daniel P. Berrangé Jan. 27, 2021, 3:09 p.m. UTC | #5
On Wed, Jan 27, 2021 at 03:24:26PM +0100, Michal Privoznik 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.
> > 
> > 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.

IIUC That's only a burden for distros if they're creating their own
machine types, in which case they've already decided the burden is
a net win.


Regards,
Daniel
Igor Mammedov Jan. 27, 2021, 3:35 p.m. UTC | #6
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.

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

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.

my take on it, at this point is that it's not worth the effort,
we can just use option unconditionally and flow usual deprecate/remove
process once 4.0 machine type is removed.

> 
> Michal
> 
>
Michal Privoznik Jan. 27, 2021, 5:03 p.m. UTC | #7
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:

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
Daniel P. Berrangé Jan. 27, 2021, 5:56 p.m. UTC | #8
On Wed, Jan 27, 2021 at 04:35:22PM +0100, 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).

IIUC, this means that setting the property has no impact on
migration ABI for new machine types > 4.0....

> Limiting knob usage to 4.0 and older would allow us to drop
> without extra efforts once 4.0 is deprecated/removed.

...so, even if we set the property unconditionally for *all*
machine types, then we can still remove it in future, becuase
its removal won't affect ABI of the 5.x, 6.x machine types.


Regards,
Daniel
Michal Privoznik Jan. 27, 2021, 6:30 p.m. UTC | #9
On 1/27/21 6:56 PM, Daniel P. Berrangé wrote:
> On Wed, Jan 27, 2021 at 04:35:22PM +0100, 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).
> 
> IIUC, this means that setting the property has no impact on
> migration ABI for new machine types > 4.0....
> 
>> Limiting knob usage to 4.0 and older would allow us to drop
>> without extra efforts once 4.0 is deprecated/removed.
> 
> ...so, even if we set the property unconditionally for *all*
> machine types, then we can still remove it in future, becuase
> its removal won't affect ABI of the 5.x, 6.x machine types.

Alright, so after all you agree with proposed patch? I'm a bit confused.

At any rate, we have patches for nearly all available options (if not 
all), and I'd like to get things moving because both qemu and libvirt 
were already released meanwhile and our upstreams are broken. Not to 
mention users that started domain with libvirt 7.0.0 - they will be 
unable to migrate to libvirt 7.1.0+ (or whatever version we fix this), 
because libvirt 7.0.0 did not set the x-something-something attribute.

Michal
Daniel P. Berrangé Jan. 27, 2021, 6:41 p.m. UTC | #10
On Wed, Jan 27, 2021 at 07:30:24PM +0100, Michal Privoznik wrote:
> On 1/27/21 6:56 PM, Daniel P. Berrangé wrote:
> > On Wed, Jan 27, 2021 at 04:35:22PM +0100, 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).
> > 
> > IIUC, this means that setting the property has no impact on
> > migration ABI for new machine types > 4.0....
> > 
> > > Limiting knob usage to 4.0 and older would allow us to drop
> > > without extra efforts once 4.0 is deprecated/removed.
> > 
> > ...so, even if we set the property unconditionally for *all*
> > machine types, then we can still remove it in future, becuase
> > its removal won't affect ABI of the 5.x, 6.x machine types.
> 
> Alright, so after all you agree with proposed patch? I'm a bit confused.

+        b) for machine types 4.0 and older, user shall
+        use ``x-use-canonical-path-for-ramblock-id=off`` backend option
+        if migration to/from old QEMU (<5.0) is expected.

instead of refering to machine type versions, I think it
could say something more like


      b) if the ``x-use-canonical-path-for-ramblock-id`` property
         exists, it should be set to ``off``, 
         if migration to/from old QEMU (<5.0) is expected.

Assuming it genuinely does not have any ABI impact on new machine
types.

Regards,
Daniel
Igor Mammedov Jan. 27, 2021, 7:19 p.m. UTC | #11
On Wed, 27 Jan 2021 18:41:46 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Wed, Jan 27, 2021 at 07:30:24PM +0100, Michal Privoznik wrote:
> > On 1/27/21 6:56 PM, Daniel P. Berrangé wrote:  
> > > On Wed, Jan 27, 2021 at 04:35:22PM +0100, 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).  
> > > 
> > > IIUC, this means that setting the property has no impact on
> > > migration ABI for new machine types > 4.0....
> > >   
> > > > Limiting knob usage to 4.0 and older would allow us to drop
> > > > without extra efforts once 4.0 is deprecated/removed.  
> > > 
> > > ...so, even if we set the property unconditionally for *all*
> > > machine types, then we can still remove it in future, becuase
> > > its removal won't affect ABI of the 5.x, 6.x machine types.  
> > 
> > Alright, so after all you agree with proposed patch? I'm a bit confused.  
> 
> +        b) for machine types 4.0 and older, user shall
> +        use ``x-use-canonical-path-for-ramblock-id=off`` backend option
> +        if migration to/from old QEMU (<5.0) is expected.
> 
> instead of refering to machine type versions, I think it
> could say something more like
> 
> 
>       b) if the ``x-use-canonical-path-for-ramblock-id`` property
>          exists, it should be set to ``off``, 
>          if migration to/from old QEMU (<5.0) is expected.

Documentation describes exact limitations/req.
libvirt can't distinguish types so it can ignore it and apply to all machine types.
Others may use it as described if they are able to distinguish
machine types.
So I'd like to keep it the way it's written instead of putting
blanket over all machine types.

> Assuming it genuinely does not have any ABI impact on new machine
> types.
x-use-canonical-path-for-ramblock-id is 'off' by default, so
if one turns off it explicitly it won't change anything for
new machine types.

> 
> Regards,
> Daniel
Igor Mammedov Jan. 27, 2021, 7:31 p.m. UTC | #12
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
> 
>
Michal Privoznik Feb. 4, 2021, 12:21 p.m. UTC | #13
On 1/21/21 5:15 PM, Igor Mammedov wrote:
 >

Ping, please? Is there anything I can help with to get this merged? 
Libvirt's migration is broken without this patch [1] and thus I'd like 
to have this merged sooner rather than later.

1: There's a libvirt patch required but depends on this one.

Thanks,
Michal
Igor Mammedov Feb. 5, 2021, 3:56 p.m. UTC | #14
On Thu, 4 Feb 2021 13:21:32 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/21/21 5:15 PM, Igor Mammedov wrote:
>  >  
> 
> Ping, please? Is there anything I can help with to get this merged? 
> Libvirt's migration is broken without this patch [1] and thus I'd like 
> to have this merged sooner rather than later.

it's a part of 
  [PULL v3 00/38] Misc patches (buildsys, i386, fuzzing) for 2021-01-29


> 
> 1: There's a libvirt patch required but depends on this one.
> 
> Thanks,
> Michal
diff mbox series

Patch

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 9f9ac95edd..813aeb83c9 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -498,6 +498,16 @@  host_memory_backend_class_init(ObjectClass *oc, void *data)
         host_memory_backend_get_share, host_memory_backend_set_share);
     object_class_property_set_description(oc, "share",
         "Mark the memory as private to QEMU or shared");
+    /*
+     * Do not delete/rename option. This option must be considered stable
+     * (as if it didn't have the 'x-' prefix including deprecation period) as
+     * long as 4.0 and older machine types exists.
+     * Option will be used by upper layers to override (disable) canonical path
+     * for ramblock-id set by compat properties on old machine types ( <= 4.0),
+     * to keep migration working when backend is used for main RAM with
+     * -machine memory-backend= option (main RAM historically used prefix-less
+     * ramblock-id).
+     */
     object_class_property_add_bool(oc, "x-use-canonical-path-for-ramblock-id",
         host_memory_backend_get_use_canonical_path,
         host_memory_backend_set_use_canonical_path);
diff --git a/qemu-options.hx b/qemu-options.hx
index 62791f56d8..059b1a1d14 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -35,7 +35,8 @@  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
     "                nvdimm=on|off controls NVDIMM support (default=off)\n"
     "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
-    "                hmat=on|off controls ACPI HMAT support (default=off)\n",
+    "                hmat=on|off controls ACPI HMAT support (default=off)\n"
+    "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n",
     QEMU_ARCH_ALL)
 SRST
 ``-machine [type=]name[,prop=value[,...]]``
@@ -96,6 +97,29 @@  SRST
     ``hmat=on|off``
         Enables or disables ACPI Heterogeneous Memory Attribute Table
         (HMAT) support. The default is off.
+
+     ``memory-backend='id'``
+        An alternative to legacy ``-mem-path`` and ``mem-prealloc`` options.
+        Allows to use a memory backend as main RAM.
+
+        For example:
+        ::
+        -object memory-backend-file,id=pc.ram,size=512M,mem-path=/hugetlbfs,prealloc=on,share=on
+        -machine memory-backend=pc.ram
+        -m 512M
+
+        Migration compatibility note:
+        a) as backend id one shall use value of 'default-ram-id', advertised by
+        machine type (available via ``query-machines`` QMP command), if migration
+        to/from old QEMU (<5.0) is expected.
+        b) for machine types 4.0 and older, user shall
+        use ``x-use-canonical-path-for-ramblock-id=off`` backend option
+        if migration to/from old QEMU (<5.0) is expected.
+        For example:
+        ::
+        -object memory-backend-ram,id=pc.ram,size=512M,x-use-canonical-path-for-ramblock-id=off
+        -machine memory-backend=pc.ram
+        -m 512M
 ERST
 
 HXCOMM Deprecated by -machine