Re: [PATCH v6 00/79] refactor main RAM allocation to use hostmem backend

[Igor Mammedov]

On Mon, 24 Feb 2020 09:45:11 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

> Hi Igor,
> 
> On 2/19/20 5:08 PM, Igor Mammedov wrote:
> [...]
> > Series removes ad hoc RAM allocation API 
> > (memory_region_allocate_system_memory)
> > and consolidates it around hostmem backend. It allows to
> >   * resolve conflicts between global -mem-prealloc and hostmem's "policy" 
> > option
> >     fixing premature allocation before binding policy is applied
> >   * simplify complicated memory allocation routines which had to deal with 
> > 2 ways
> >     to allocate RAM.
> >   * it allows to reuse hostmem backends of a choice for main RAM without 
> > adding
> >     extra CLI options to duplicate hostmem features.
> >     Recent case was -mem-shared, to enable vhost-user on targets that don't
> >     support hostmem backends [1] (ex: s390)
> >   * move RAM allocation from individual boards into generic machine code and
> >     provide them with prepared MemoryRegion.
> >   * clean up deprecated NUMA features which were tied to the old API (see 
> > patches)
> >      - "numa: remove deprecated -mem-path fallback to anonymous RAM"
> >      - (POSTPONED, waiting on libvirt side) "forbid '-numa node,mem' for 
> > 5.0 and newer machine types"
> >      - (POSTPONED) "numa: remove deprecated implicit RAM distribution 
> > between nodes"
> > 
> > Conversion introduces a new machine.memory-backend property and wrapper 
> > code that
> > aliases global -mem-path and -mem-alloc into automatically created hostmem
> > backend properties (provided memory-backend was not set explicitly given by 
> > user).
> > And then follows bulk of trivial patches that incrementally convert 
> > individual
> > boards to using machine.memory-backend provided MemoryRegion.
> > 
> > Board conversion typically involves:
> >   * providing MachineClass::default_ram_size and 
> > MachineClass::default_ram_id
> >     so generic code could create default backend if user didn't explicitly 
> > provide
> >     memory-backend or -m options
> >   * dropping memory_region_allocate_system_memory() call
> >   * using convenience MachineState::ram MemoryRegion, which points to 
> > MemoryRegion
> >     allocated by ram-memdev
> > On top of that for some boards:
> >   * added missing ram_size checks (typically it were boards with fixed ram 
> > size)
> >   * ram_size fixups were replaced by checks and hard errors, forcing user to
> >     provide correct "-m" values instead of ignoring it and continuing 
> > running.
> > 
> > After all boards are converted the old API is removed and memory allocation
> > routines are cleaned up.  
> 
> I wonder about the pre-QOM machines. As they don't call 
> memory_region_allocate_system_memory(), the conversion is not required? 
> (See for example pxa270_init).
Since they weren't using memory_region_allocate_system_memory(), they are
out of scope of this series.

As for the future, I'd only make boards that support user configurable
ram size to accept "-m".

For fixed size boards -m/memdev is overkill and we need to decide what to do
with them. I see following options (in order of my preference):
  1. Non popular: error out if -m is specified (it used to work, but not
     anymore when check is added, i.e similar to size checks
     introduced in this series so users have to adapt their CLI).
     It can still use automatically created memdev but I'd ditch it on
     those boards and use plain memory_region_init_ram().
     This is matches well SoCs that have embedded RAM and don't really
     care about what user may specify with -m. It would simplify
     simple boards.

  2. a path of least resistance: continue support -m and generalize
     ram_size checks for such boards. This could use memdev since it
     comes for free with -m support. I don't expect complications
     with generalizing it (but one would only know for sure when
     it's coded)

The next this I plan to do is to clean up ram_size global and
hopefully get rid of MachineState:ram_size as well.