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

[Igor Mammedov]

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

> 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";
there was cleanup patch like this in v2, which I dropped for now
as it needs more not directly related this refactoring.
See my reply to: "[PATCH v4 43/80] x86/pc: use memdev for RAM"
I dropped it since if above were done, it would trigger a bunch
of changes /to properly handle case/ to boards that do not care
about -m and manage RAM in its own way and not related to
removing memory_region_allocate_system_memory().

I'm not saying it shouldn't be done but rather,
it should be done on top to take in account cases for fixed size
boards and for boards that do not use -m at all and just create
RAM memory region on its own.

> > Basically default_ram_id is a means to optin into hostmem
> > based allocation with (-m).  
> 
> It's just a cryptic way to do it.
name came up from current meaning of the value stored there,
namely boards use value as the default memory region id.
But optin behavior is not clear from that, How about adding
following to doc comment to make intent explicit:

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 56a4dd9..142b86d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -147,6 +147,9 @@ typedef struct {
  *    Specifies inital RAM MemoryRegion name to be used for default backend
  *    creation if user explicitly hasn't specified backend with 
"memory-backend"
  *    property.
+ *    It also will be used as a way to optin into "-m" option support.
+ *    If it's not set by board, '-m' will be ignored and generic code will
+ *    not create default RAM MemoryRegion.
  */


[...]


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