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

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

[Igor Mammedov]

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.

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>)
---
 backends/hostmem.c | 10 ++++++++++
 qemu-options.hx    | 28 +++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4bde00e8e7..6aaeefd0cf 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -497,6 +497,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 459c916d3d..22aa267c30 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,31 @@ 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)
+        b) for machine types 4.0 and older, user shall
+        use ``x-use-canonical-path-for-ramblock-id=on`` backend 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),
+        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=on
+        -machine memory-backend=pc.ram
+        -m 512M
 ERST
 
 HXCOMM Deprecated by -machine
-- 
2.27.0

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

[Michal Privoznik]

On 1/15/21 12:46 AM, 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.
> 
> 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>)
> ---
>   backends/hostmem.c | 10 ++++++++++
>   qemu-options.hx    | 28 +++++++++++++++++++++++++++-
>   2 files changed, 37 insertions(+), 1 deletion(-)
> 


> @@ -96,6 +97,31 @@ 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)
> +        b) for machine types 4.0 and older, user shall
> +        use ``x-use-canonical-path-for-ramblock-id=on`` backend 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),
> +        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=on
> +        -machine memory-backend=pc.ram
> +        -m 512M

Igor, this doesn't correspond with your comment in bugzilla:

https://bugzilla.redhat.com/show_bug.cgi?id=1836043#c31

In fact, we had to turn the attribute OFF so that canonical path is not 
used. Isn't ON the default state anyway?

Michal

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

[Peter Maydell]

On Thu, 14 Jan 2021 at 23:48, Igor Mammedov <imammedo@redhat.com> 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.

That's not what the x- prefix is supposed to mean.
If we have an internal constraint that we mustn't delete
the option in order to support some other must-be-stable
interface (eg migration of some machines) we can document
that in a comment, but that doesn't mean that we should
document to users that direct use of an x-prefix option
is supported as a stable interface.

Alternatively, if the option is really stable for direct
use by users then we should commit to making it so by
removing the x-.

thanks
-- PMM

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

[Peter Krempa]

On Fri, Jan 15, 2021 at 10:02:04 +0000, Peter Maydell wrote:
> On Thu, 14 Jan 2021 at 23:48, Igor Mammedov <imammedo@redhat.com> 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.
> 
> That's not what the x- prefix is supposed to mean.

This is the exact reason I was asking on behalf of the libvirt team for
adding such statement if we were to use it. We want guarantee that it's
considered stable since without that it will not be accepted into
libvirt.

> If we have an internal constraint that we mustn't delete
> the option in order to support some other must-be-stable
> interface (eg migration of some machines) we can document
> that in a comment, but that doesn't mean that we should
> document to users that direct use of an x-prefix option
> is supported as a stable interface.

AFAIK the issue is that with the new approach to configure system memory
(via a memory-backend=id, since the old one was deprecated) migration
fails from/to older qemus ...

> Alternatively, if the option is really stable for direct
> use by users then we should commit to making it so by
> removing the x-.

... thus the idea behind keeping this interface as is is it also fixes
the migration compatibility for qemu 5.0/5.1/5.2 which were already
released.

Removing the 'x-' will fix it only starting with qemu-6.0 and any
downstream which backports the removal of the prefix.

Obviously not using 'x-' prefixed options is strongly preferred in
libvirt. 

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

[Peter Krempa]

On Fri, Jan 15, 2021 at 11:43:10 +0100, Peter Krempa wrote:
> On Fri, Jan 15, 2021 at 10:02:04 +0000, Peter Maydell wrote:
> > On Thu, 14 Jan 2021 at 23:48, Igor Mammedov <imammedo@redhat.com> wrote:

[...]

> Removing the 'x-' will fix it only starting with qemu-6.0 and any
> downstream which backports the removal of the prefix.
> 
> Obviously not using 'x-' prefixed options is strongly preferred in
> libvirt. 

For reference, here are the relevant threads on libvir-list:

v1:
https://www.redhat.com/archives/libvir-list/2021-January/msg00601.html
v2:
https://www.redhat.com/archives/libvir-list/2021-January/msg00684.html

And my objection to use 'x-' prefixed features without being guaranteed
that it's considered stable (note that the premise is that it's supposed
to fix already-released qemu versions thus not completely refusing using
the existing naming)
https://www.redhat.com/archives/libvir-list/2021-January/msg00606.html
https://www.redhat.com/archives/libvir-list/2021-January/msg00699.html

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

[Igor Mammedov]

On Fri, 15 Jan 2021 10:36:05 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 1/15/21 12:46 AM, 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.
> > 
> > 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>)
> > ---
> >   backends/hostmem.c | 10 ++++++++++
> >   qemu-options.hx    | 28 +++++++++++++++++++++++++++-
> >   2 files changed, 37 insertions(+), 1 deletion(-)
> >   
> 
> 
> > @@ -96,6 +97,31 @@ 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)
> > +        b) for machine types 4.0 and older, user shall
> > +        use ``x-use-canonical-path-for-ramblock-id=on`` backend 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),
> > +        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=on
> > +        -machine memory-backend=pc.ram
> > +        -m 512M  
> 
> Igor, this doesn't correspond with your comment in bugzilla:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1836043#c31
> 
> In fact, we had to turn the attribute OFF so that canonical path is not 
> used. Isn't ON the default state anyway?

indeed, it should be 'off',

> 
> Michal
> 
> 

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

[Igor Mammedov]

On Fri, 15 Jan 2021 10:02:04 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 14 Jan 2021 at 23:48, Igor Mammedov <imammedo@redhat.com> 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.  
> 
> That's not what the x- prefix is supposed to mean.
> If we have an internal constraint that we mustn't delete
> the option in order to support some other must-be-stable
> interface (eg migration of some machines) we can document
> that in a comment,
that was in v1, and Peter asked for adding assurance to help/doc as well.

> but that doesn't mean that we should
> document to users that direct use of an x-prefix option
> is supported as a stable interface.
A concur, that we don't have to declare it as stable in help/doc,
but we still have to document x-use-canonical-path-for-ramblock-id=off
the so users would know how/when to use it in this particular case.


> Alternatively, if the option is really stable for direct
> use by users then we should commit to making it so by
> removing the x-.

Peter Maydell,

I think Peter Krempa already explained/pointed to discussion why
x-use-canonical-path-for-ramblock-id wasn't renamed.

So as I see options are:
  1) keep x- prefix declare it as stable both in doc and comments (like in this patch)
     add to commit message why we are keeping x-
  2) keep x- prefix declare it as stable in comments only,
     keep doc changes to explaining how/when to use it
     add to commit message why we are keeping x-
  3) rename/drop x- prefix and don't care about QEMU-5.0-5.2
     (libvirt would use old syntax (-mem-path/mem-prealloc) for them
      which also leads to => no virtiofs as it needs shared RAM that
      new syntax with backend provides for main RAM)

Which one is acceptable to you?

> thanks
> -- PMM
>