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

[Michael S. Tsirkin]

On Mon, Feb 03, 2020 at 10:27:20AM +0100, Igor Mammedov wrote:
> 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.

However for non versioned machines (and that's the majority of them), we could 
supply the
id in the generic code. Basically
        if (!default_ram_id)
                default_ram_id = "ram";


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

It's just a cryptic way to do it.

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