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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/24/20 12:33 PM, Igor Mammedov wrote:
> On Mon, 24 Feb 2020 09:45:11 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> 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".

Good cleanup.

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

LGTM.

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