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

[David Hildenbrand <david@redhat.com>]

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