|
From: | David Hildenbrand |
Subject: | Re: [PATCH v1 2/3] memory-device: Factor out device memory initialization into memory_devices_init() |
Date: | Thu, 25 May 2023 15:40:14 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 |
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.
Right, I agree if I'd be touching more code (I consider this fairly minimal and straight-forward refactoring).
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?
We have right now: ms->device_memory not allocated: no support ms->device_memory->mr with size 0: not enabled ms->device_memory->mr with size > 0: enabledLet me see if initializing a memory region with size 0 (and adding a subregion with size 0) works.
Thanks! -- Thanks, David / dhildenb
[Prev in Thread] | Current Thread | [Next in Thread] |