Re: [PATCH v1 2/3] memory-device: Factor out device memory initialization into memory_devices_init()

[Philippe Mathieu-Daudé]

Hi David,

On 23/5/23 20:51, David Hildenbrand wrote:
> Let's factor the common setup out, to prepare for further changes.
>
> On arm64, we'll add the subregion to system RAM now earlier -- which
> shouldn't matter, because the system RAM memory region should already be
> alive at that point.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/arm/virt.c                  |  9 +--------
>   hw/i386/pc.c                   | 17 ++++++-----------
>   hw/loongarch/virt.c            | 14 ++++----------
>   hw/mem/memory-device.c         | 20 ++++++++++++++++++++
>   hw/ppc/spapr.c                 | 15 ++++++---------
>   include/hw/mem/memory-device.h |  2 ++
>   6 files changed, 39 insertions(+), 38 deletions(-)

Split in boring 'first add method then use it for each arch'
would be easier to review.

> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 6c025b02c1..d99ceb621a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -17,6 +17,7 @@
>   #include "qemu/range.h"
>   #include "hw/virtio/vhost.h"
>   #include "sysemu/kvm.h"
> +#include "exec/address-spaces.h"
>   #include "trace.h"
>   
>   static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
> @@ -333,6 +334,25 @@ uint64_t memory_device_get_region_size(const 
> MemoryDeviceState *md,
>       return memory_region_size(mr);
>   }
>   
> +void memory_devices_init(MachineState *ms, hwaddr base, uint64_t size)
> +{
> +    g_assert(!ms->device_memory);
> +    ms->device_memory = g_new0(DeviceMemoryState, 1);
> +    ms->device_memory->base = base;
> +
> +    /*
> +     * See memory_device_get_free_addr(): An empty device memory region
> +     * means "this machine supports memory devices, but they are not enabled".
> +     */
> +    if (size > 0) {
> +        memory_region_init(&ms->device_memory->mr, OBJECT(ms), "device-memory",
> +                           size);
> +        memory_region_add_subregion(get_system_memory(),
> +                                    ms->device_memory->base,
> +                                    &ms->device_memory->mr);

What about always init/register and set if enabled?

  memory_region_set_enabled(&ms->device_memory->mr, size > 0);

Otherwise why allocate ms->device_memory?

> +    }
> +}