Re: [PATCH v4 03/80] machine: alias -mem-path and -mem-prealloc into memory-foo backend

[Igor Mammedov]

On Mon, 3 Feb 2020 04:04:15 -0500
"Michael S. Tsirkin" <address@hidden> wrote:

> On Fri, Jan 31, 2020 at 04:08:33PM +0100, Igor Mammedov wrote:
> > Allow machine to opt in for hostmem backend based initial RAM
> > even if user uses old -mem-path/prealloc options by providing
> >   MachineClass::default_ram_id
> > Follow up patches will incrementally convert machines to new API,
> > by dropping memory_region_allocate_system_memory() and setting
> > default_ram_id that board used to use before conversion to keep
> > migration stream the same.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>  
> 
> What about non-versioned machines though?
> Why do these need to set default_ram_id?
> Seems redundant as they don't need to support cross-version
> migration ...

They need at least some id for migration to work,
so this series reuses id that they are using today.

Basically default_ram_id is a means to optin into hostmem
based allocation with (-m).

> 
> > ---
> > v3:
> >    * rename "ram-memdev" property to "memory-backend"
> >      (Paolo Bonzini <address@hidden>)
> > v4:
> >    * previous patch changed memory-backend property from link
> >      to string to allow for delayed backend creation, when
> >      backend comes explicitly from CLI.
> >      So s/object_property_set_link()/object_property_set_str()/
> >      to account for that.
> > 
> > CC: address@hidden
> > CC: address@hidden
> > CC: address@hidden
> > CC: address@hidden
> > ---
> >  include/hw/boards.h      |  5 +++++
> >  include/sysemu/hostmem.h | 16 ++++++++++++++++
> >  backends/hostmem-file.c  |  7 -------
> >  backends/hostmem-ram.c   |  2 --
> >  vl.c                     | 25 +++++++++++++++++++++++++
> >  5 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 7b4b6b7..106de87 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -170,6 +170,10 @@ typedef struct {
> >   *    false is returned, an error must be set to show the reason of
> >   *    the rejection.  If the hook is not provided, all hotplug will be
> >   *    allowed.
> > + * @default_ram_id:
> > + *    Specifies inital RAM MemoryRegion name to be used for default backend
> > + *    creation if user explicitly hasn't specified backend with 
> > "memory-backend"
> > + *    property.
> >   */
> >  struct MachineClass {
> >      /*< private >*/
> > @@ -226,6 +230,7 @@ struct MachineClass {
> >      bool nvdimm_supported;
> >      bool numa_mem_supported;
> >      bool auto_enable_numa;
> > +    const char *default_ram_id;
> >  
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >                                             DeviceState *dev);
> > diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> > index 4dbdadd..5db0d66 100644
> > --- a/include/sysemu/hostmem.h
> > +++ b/include/sysemu/hostmem.h
> > @@ -27,6 +27,22 @@
> >  #define MEMORY_BACKEND_CLASS(klass) \
> >      OBJECT_CLASS_CHECK(HostMemoryBackendClass, (klass), 
> > TYPE_MEMORY_BACKEND)
> >  
> > +/* hostmem-ram.c */
> > +/**
> > + * @TYPE_MEMORY_BACKEND_RAM:
> > + * name of backend that uses mmap on the anonymous RAM
> > + */
> > +
> > +#define TYPE_MEMORY_BACKEND_RAM "memory-backend-ram"
> > +
> > +/* hostmem-file.c */
> > +/**
> > + * @TYPE_MEMORY_BACKEND_FILE:
> > + * name of backend that uses mmap on a file descriptor
> > + */
> > +#define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
> > +
> > +typedef struct HostMemoryBackend HostMemoryBackend;
> >  typedef struct HostMemoryBackendClass HostMemoryBackendClass;
> >  
> >  /**
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index be64020..cb319a9 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -18,13 +18,6 @@
> >  #include "sysemu/sysemu.h"
> >  #include "qom/object_interfaces.h"
> >  
> > -/* hostmem-file.c */
> > -/**
> > - * @TYPE_MEMORY_BACKEND_FILE:
> > - * name of backend that uses mmap on a file descriptor
> > - */
> > -#define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
> > -
> >  #define MEMORY_BACKEND_FILE(obj) \
> >      OBJECT_CHECK(HostMemoryBackendFile, (obj), TYPE_MEMORY_BACKEND_FILE)
> >  
> > diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> > index 6aab8d3..5cc53e7 100644
> > --- a/backends/hostmem-ram.c
> > +++ b/backends/hostmem-ram.c
> > @@ -16,8 +16,6 @@
> >  #include "qemu/module.h"
> >  #include "qom/object_interfaces.h"
> >  
> > -#define TYPE_MEMORY_BACKEND_RAM "memory-backend-ram"
> > -
> >  static void
> >  ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >  {
> > diff --git a/vl.c b/vl.c
> > index 24951b5..2367cb6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -75,6 +75,7 @@ int main(int argc, char **argv)
> >  #include "ui/input.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/hostmem.h"
> >  #include "exec/gdbstub.h"
> >  #include "qemu/timer.h"
> >  #include "chardev/char.h"
> > @@ -2839,6 +2840,26 @@ static void configure_accelerators(const char 
> > *progname)
> >      }
> >  }
> >  
> > +static void create_default_memdev(MachineState *ms, const char *path,
> > +                                  bool prealloc)
> > +{
> > +    Object *obj;
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > +    obj = object_new(path ? TYPE_MEMORY_BACKEND_FILE : 
> > TYPE_MEMORY_BACKEND_RAM);
> > +    if (path) {
> > +        object_property_set_str(obj, path, "mem-path", &error_fatal);
> > +    }
> > +    object_property_set_bool(obj, prealloc, "prealloc", &error_fatal);
> > +    object_property_set_int(obj, ms->ram_size, "size", &error_fatal);
> > +    object_property_add_child(object_get_objects_root(), 
> > mc->default_ram_id,
> > +                              obj, &error_fatal);
> > +    user_creatable_complete(USER_CREATABLE(obj), &error_fatal);
> > +    object_unref(obj);
> > +    object_property_set_str(OBJECT(ms), mc->default_ram_id, 
> > "memory-backend",
> > +                            &error_fatal);
> > +}
> > +
> >  int main(int argc, char **argv, char **envp)
> >  {
> >      int i;
> > @@ -4302,6 +4323,10 @@ int main(int argc, char **argv, char **envp)
> >      }
> >      parse_numa_opts(current_machine);
> >  
> > +    if (machine_class->default_ram_id && current_machine->ram_size &&
> > +        !current_machine->ram_memdev_id) {
> > +        create_default_memdev(current_machine, mem_path, mem_prealloc);
> > +    }
> >      /* do monitor/qmp handling at preconfig state if requested */
> >      main_loop();  
> 
> So this check for default_ram_id will become redundant after the
> patchset is fully applied, this is just for bisect to work well, right?
> I couldn't find patches that drop this check though.

there was a patch to that effect in earlier versions,
but I dropped it for following reasons (from cover letter):

"
v3:
[...]
  - drop "[PATCH v2 76/86] post conversion default_ram_id cleanup"
    so that default memory-backedend won't be created for boards that do not 
care
    about -m. Which makes -m optin feature. We should decide  what do in  case
    board doesn't use -m (but that's out of scope of this series)
[...]
"

> 
> > -- 
> > 2.7.4
> > 
> >   
>