Re: [PATCH 02/10] memory-device: Introduce memory_devices_init()

[David Hildenbrand]

On 30.05.23 14:29, Philippe Mathieu-Daudé wrote:
> Hi David,
>
> On 30/5/23 13:38, David Hildenbrand wrote:
> > Let's intrduce a new helper that we will use to replace existing memory
> > device setup code during machine initialization. We'll enforce that the
> > size has to be > 0.
> >
> > Once all machines were converted, we'll only allocate ms->device_memory
> > if the size > 0.
> >
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >    hw/mem/memory-device.c         | 14 ++++++++++++++
> >    include/hw/mem/memory-device.h |  2 ++
> >    2 files changed, 16 insertions(+)
>
>
> > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> > index 48d2611fc5..6e8a10e2f5 100644
> > --- a/include/hw/mem/memory-device.h
> > +++ b/include/hw/mem/memory-device.h
> > @@ -16,6 +16,7 @@
> >    #include "hw/qdev-core.h"
> >    #include "qapi/qapi-types-machine.h"
> >    #include "qom/object.h"
> > +#include "exec/hwaddr.h"
> >    
> >    #define TYPE_MEMORY_DEVICE "memory-device"
> >    
> > @@ -113,5 +114,6 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms);
> >    void memory_device_unplug(MemoryDeviceState *md, MachineState *ms);
> >    uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
> >                                           Error **errp);
> > +void memory_devices_init(MachineState *ms, hwaddr base, uint64_t size);
>
>
> While hw/mem/memory-device.c contains the implementation, all callers
> are expected to be around Machine object, right? Thus maybe this _init()
> could be declared in "hw/boards.h", already included by machines
> (eventually renaming as machine_init_memory_devices() ). Then machines
> implementation don't have to all include "hw/mem/memory-device.h".

Some (arm, i386) want to call the hotplug handle functions either way, 
so they'll still have to include that header.

But sure, we can rename to machine_init_memory_devices() and declare it 
include/hw/boards.h!

> Alternatively, keep memory_devices_init() declared here, but implement
> machine_init_memory_devices() in hw/core/machine.c, declaring it in
> "hw/boards.h", so again machines don't have to include
> "hw/mem/memory-device.h".

I prefer the implementation residing in hw/mem/memory-device.c. I have 
some upcoming patches that add more "meat" to the init function, such 
that makes it looks cleaner to me to just reside hw/mem/memory-device.h.

Thanks!

--
Thanks,

David / dhildenb