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

[David Hildenbrand]

On 25.05.23 15:30, Philippe Mathieu-Daudé wrote:
> 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?

With

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;

    /*
     * An empty region (size == 0) indicates that memory devices are supported
     * by the machine, but they are not enabled (see memory_device_pre_plug()).
     */
    memory_region_init(&ms->device_memory->mr, OBJECT(ms), "device-memory",
                       size);
    memory_region_set_enabled(&ms->device_memory->mr, !!size);
    memory_region_add_subregion(get_system_memory(), ms->device_memory->base,
                                &ms->device_memory->mr);
}

"info mtree -D" on x86-64 with only "-m 2G" (no maxmem) will show that the
region is placed at address 0 and disabled:

memory-region: system
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-0000000000000000 (prio 0, i/o): device-memory [disabled]
    0000000000000000-000000007fffffff (prio 0, ram): alias ram-below-4g @pc.ram 
0000000000000000-000000007fffffff
    0000000000000000-ffffffffffffffff (prio -1, i/o): pci


Good enough for me.

However, I think we could just stop allocating ms->device_memory with size == 0 
and
not care about the "not supported" case in memory_device_pre_plug(): this 
function should
only get called by a machine, and if the machine does not support memory 
devices, it is
to blame for calling that function after all. (and it will only affect the 
error message
after all)

--
Thanks,

David / dhildenb