hw/mem/memory-device.c | 22 +++------------------- include/hw/boards.h | 2 ++ 2 files changed, 5 insertions(+), 19 deletions(-)
Let's avoid iterating over all devices and simply track it in the
DeviceMemoryState.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 22 +++-------------------
include/hw/boards.h | 2 ++
2 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 00c7755557..667d56bd29 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -52,28 +52,11 @@ static int memory_device_build_list(Object *obj, void *opaque)
return 0;
}
-static int memory_device_used_region_size(Object *obj, void *opaque)
-{
- uint64_t *size = opaque;
-
- if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
- const DeviceState *dev = DEVICE(obj);
- const MemoryDeviceState *md = MEMORY_DEVICE(obj);
-
- if (dev->realized) {
- *size += memory_device_get_region_size(md, &error_abort);
- }
- }
-
- object_child_foreach(obj, memory_device_used_region_size, opaque);
- return 0;
-}
-
static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
Error **errp)
{
+ const uint64_t used_region_size = ms->device_memory->used_region_size;
const uint64_t size = memory_region_size(mr);
- uint64_t used_region_size = 0;
/* we will need a new memory slot for kvm and vhost */
if (kvm_enabled() && !kvm_has_free_slot(ms)) {
@@ -86,7 +69,6 @@ static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
}
/* will we exceed the total amount of memory specified */
- memory_device_used_region_size(OBJECT(ms), &used_region_size);
if (used_region_size + size < used_region_size ||
used_region_size + size > ms->maxram_size - ms->ram_size) {
error_setg(errp, "not enough space, currently 0x%" PRIx64
@@ -292,6 +274,7 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
mr = mdc->get_memory_region(md, &error_abort);
g_assert(ms->device_memory);
+ ms->device_memory->used_region_size += memory_region_size(mr);
memory_region_add_subregion(&ms->device_memory->mr,
addr - ms->device_memory->base, mr);
trace_memory_device_plug(DEVICE(md)->id ? DEVICE(md)->id : "", addr);
@@ -310,6 +293,7 @@ void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
g_assert(ms->device_memory);
memory_region_del_subregion(&ms->device_memory->mr, mr);
+ ms->device_memory->used_region_size -= memory_region_size(mr);
trace_memory_device_unplug(DEVICE(md)->id ? DEVICE(md)->id : "",
mdc->get_addr(md));
}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index be06e8a41f..fcaf40b9da 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -296,11 +296,13 @@ struct MachineClass {
* address space for memory devices starts
* @mr: address space container for memory devices
* @dimm_size: the sum of plugged DIMMs' sizes
+ * @used_region_size: the part of @mr already used by memory devices
*/
typedef struct DeviceMemoryState {
hwaddr base;
MemoryRegion mr;
uint64_t dimm_size;
+ uint64_t used_region_size;
} DeviceMemoryState;
/**
--
2.40.1
Working on adding multi-memslot support for virtio-mem (teaching memory device code about memory devices that can consume multiple memslots), I have some preparatory cleanups in my queue that make sense independent of the actual memory-device/virtio-mem extensions. v2 -> v3: - "memory-device: Introduce machine_memory_devices_init()" -- Declare the function in hw/boards.h - "hw/loongarch/virt: Use machine_memory_devices_init()" -- Use VIRT_HIGHMEM_BASE -- No need to include memory-device.h - "hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZ" -- Add more details why it's ok to the patch description - Adjust to memory_devices_init() -> machine_memory_devices_init() - Add RBs v1 -> v2: - Allocate ms->device_memory only if the size > 0. - Split it up and include more cleanups David Hildenbrand (10): memory-device: Unify enabled vs. supported error messages memory-device: Introduce machine_memory_devices_init() hw/arm/virt: Use machine_memory_devices_init() hw/ppc/spapr: Use machine_memory_devices_init() hw/loongarch/virt: Use machine_memory_devices_init() hw/i386/pc: Use machine_memory_devices_init() hw/i386/acpi-build: Rely on machine->device_memory when building SRAT hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE memory-device: Refactor memory_device_pre_plug() memory-device: Track used region size in DeviceMemoryState hw/arm/virt.c | 9 +----- hw/i386/acpi-build.c | 9 ++---- hw/i386/pc.c | 36 +++------------------- hw/loongarch/virt.c | 12 ++------ hw/mem/memory-device.c | 69 +++++++++++++++++++----------------------- hw/ppc/spapr.c | 37 +++++++++++----------- hw/ppc/spapr_hcall.c | 2 +- include/hw/boards.h | 4 +++ include/hw/i386/pc.h | 1 - 9 files changed, 67 insertions(+), 112 deletions(-) -- 2.40.1
On 01.06.23 14:14, David Hildenbrand wrote: > Working on adding multi-memslot support for virtio-mem (teaching memory > device code about memory devices that can consume multiple memslots), I > have some preparatory cleanups in my queue that make sense independent of > the actual memory-device/virtio-mem extensions. > > v2 -> v3: > - "memory-device: Introduce machine_memory_devices_init()" > -- Declare the function in hw/boards.h > - "hw/loongarch/virt: Use machine_memory_devices_init()" > -- Use VIRT_HIGHMEM_BASE > -- No need to include memory-device.h > - "hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZ" > -- Add more details why it's ok to the patch description > - Adjust to memory_devices_init() -> machine_memory_devices_init() > - Add RBs > > v1 -> v2: > - Allocate ms->device_memory only if the size > 0. > - Split it up and include more cleanups > > David Hildenbrand (10): > memory-device: Unify enabled vs. supported error messages > memory-device: Introduce machine_memory_devices_init() > hw/arm/virt: Use machine_memory_devices_init() > hw/ppc/spapr: Use machine_memory_devices_init() > hw/loongarch/virt: Use machine_memory_devices_init() > hw/i386/pc: Use machine_memory_devices_init() > hw/i386/acpi-build: Rely on machine->device_memory when building SRAT > hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE > memory-device: Refactor memory_device_pre_plug() > memory-device: Track used region size in DeviceMemoryState > > hw/arm/virt.c | 9 +----- > hw/i386/acpi-build.c | 9 ++---- > hw/i386/pc.c | 36 +++------------------- > hw/loongarch/virt.c | 12 ++------ > hw/mem/memory-device.c | 69 +++++++++++++++++++----------------------- > hw/ppc/spapr.c | 37 +++++++++++----------- > hw/ppc/spapr_hcall.c | 2 +- > include/hw/boards.h | 4 +++ > include/hw/i386/pc.h | 1 - > 9 files changed, 67 insertions(+), 112 deletions(-) > Any further comments? If not, I'm planning on taking this through my memory-device tree. -- Cheers, David / dhildenb
On 01.06.23 14:14, David Hildenbrand wrote: > Let's avoid iterating over all devices and simply track it in the > DeviceMemoryState. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: David Hildenbrand <david@redhat.com> Not my week for sending patches, ignore this one, it's a duplicate with #10 ... -- Thanks, David / dhildenb
Let's unify the error messages, such that we can simply stop allocating
ms->device_memory if the size would be 0 (and there are no memory
devices ever).
The case of "not supported by the machine" should barely pop up either
way: if the machine doesn't support memory devices, it usually doesn't
call the pre_plug handler ...
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 1636db9679..49f86ec8a8 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -104,15 +104,10 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
GSList *list = NULL, *item;
Range as, new = range_empty;
- if (!ms->device_memory) {
- error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
- "supported by the machine");
- return 0;
- }
-
- if (!memory_region_size(&ms->device_memory->mr)) {
- error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
- "enabled, please specify the maxmem option");
+ if (!ms->device_memory || !memory_region_size(&ms->device_memory->mr)) {
+ error_setg(errp, "the configuration is not prepared for memory devices"
+ " (e.g., for memory hotplug), consider specifying the"
+ " maxmem option");
return 0;
}
range_init_nofail(&as, ms->device_memory->base,
--
2.40.1
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.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 14 ++++++++++++++
include/hw/boards.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 49f86ec8a8..bb9d7c2a20 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)
@@ -328,6 +329,19 @@ uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
return memory_region_size(mr);
}
+void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t size)
+{
+ g_assert(size);
+ g_assert(!ms->device_memory);
+ ms->device_memory = g_new0(DeviceMemoryState, 1);
+ ms->device_memory->base = base;
+
+ 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);
+}
+
static const TypeInfo memory_device_info = {
.name = TYPE_MEMORY_DEVICE,
.parent = TYPE_INTERFACE,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index a385010909..be06e8a41f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -35,6 +35,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
Error **errp);
void machine_parse_smp_config(MachineState *ms,
const SMPConfiguration *config, Error **errp);
+void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t size);
+
/**
* machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
--
2.40.1
Let's use our new helper. We'll add the subregion to system RAM now
earlier. That shouldn't matter, because the system RAM memory region should
already be alive at that point.
Cc: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/arm/virt.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9b9f7d9c68..087e7059c7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1817,10 +1817,7 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
virt_set_high_memmap(vms, base, pa_bits);
if (device_memory_size > 0) {
- ms->device_memory = g_malloc0(sizeof(*ms->device_memory));
- ms->device_memory->base = device_memory_base;
- memory_region_init(&ms->device_memory->mr, OBJECT(vms),
- "device-memory", device_memory_size);
+ machine_memory_devices_init(ms, device_memory_base, device_memory_size);
}
}
@@ -2261,10 +2258,6 @@ static void machvirt_init(MachineState *machine)
memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base,
machine->ram);
- if (machine->device_memory) {
- memory_region_add_subregion(sysmem, machine->device_memory->base,
- &machine->device_memory->mr);
- }
virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem);
--
2.40.1
Let's use our new helper and stop always allocating ms->device_memory.
There is no difference in common memory-device code anymore between
ms->device_memory being NULL or the size being 0. So we only have to
teach spapr code that ms->device_memory isn't always around.
We can now modify two maxram_size checks to rely on ms->device_memory
for detecting whether we have memory devices.
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Greg Kurz <groug@kaod.org>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/ppc/spapr.c | 37 +++++++++++++++++++------------------
hw/ppc/spapr_hcall.c | 2 +-
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index dcb7f1c70a..b53aa4b5a8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -546,10 +546,8 @@ static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
cpu_to_be32(lmb_size & 0xffffffff)};
MemoryDeviceInfoList *dimms = NULL;
- /*
- * Don't create the node if there is no device memory
- */
- if (machine->ram_size == machine->maxram_size) {
+ /* Don't create the node if there is no device memory. */
+ if (!machine->device_memory) {
return 0;
}
@@ -859,16 +857,23 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
int rtas;
GString *hypertas = g_string_sized_new(256);
GString *qemu_hypertas = g_string_sized_new(256);
- uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
- memory_region_size(&MACHINE(spapr)->device_memory->mr);
uint32_t lrdr_capacity[] = {
- cpu_to_be32(max_device_addr >> 32),
- cpu_to_be32(max_device_addr & 0xffffffff),
+ 0,
+ 0,
cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE >> 32),
cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE & 0xffffffff),
cpu_to_be32(ms->smp.max_cpus / ms->smp.threads),
};
+ /* Do we have device memory? */
+ if (MACHINE(spapr)->device_memory) {
+ uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
+ memory_region_size(&MACHINE(spapr)->device_memory->mr);
+
+ lrdr_capacity[0] = cpu_to_be32(max_device_addr >> 32);
+ lrdr_capacity[1] = cpu_to_be32(max_device_addr & 0xffffffff);
+ }
+
_FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
/* hypertas */
@@ -2454,6 +2459,7 @@ static void spapr_create_lmb_dr_connectors(SpaprMachineState *spapr)
uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
int i;
+ g_assert(!nr_lmbs || machine->device_memory);
for (i = 0; i < nr_lmbs; i++) {
uint64_t addr;
@@ -2866,12 +2872,11 @@ static void spapr_machine_init(MachineState *machine)
/* map RAM */
memory_region_add_subregion(sysmem, 0, machine->ram);
- /* always allocate the device memory information */
- machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
-
/* initialize hotplug memory address space */
if (machine->ram_size < machine->maxram_size) {
ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+ hwaddr device_mem_base;
+
/*
* Limit the number of hotpluggable memory slots to half the number
* slots that KVM supports, leaving the other half for PCI and other
@@ -2890,12 +2895,8 @@ static void spapr_machine_init(MachineState *machine)
exit(1);
}
- machine->device_memory->base = ROUND_UP(machine->ram_size,
- SPAPR_DEVICE_MEM_ALIGN);
- memory_region_init(&machine->device_memory->mr, OBJECT(spapr),
- "device-memory", device_mem_size);
- memory_region_add_subregion(sysmem, machine->device_memory->base,
- &machine->device_memory->mr);
+ device_mem_base = ROUND_UP(machine->ram_size, SPAPR_DEVICE_MEM_ALIGN);
+ machine_memory_devices_init(machine, device_mem_base, device_mem_size);
}
if (smc->dr_lmb_enabled) {
@@ -5109,7 +5110,7 @@ static bool phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
int i;
/* Do we have device memory? */
- if (MACHINE(spapr)->maxram_size > ram_top) {
+ if (MACHINE(spapr)->device_memory) {
/* Can't just use maxram_size, because there may be an
* alignment gap between normal and device memory regions
*/
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index b904755575..1dd32f340f 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -31,7 +31,7 @@ bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
if (addr < machine->ram_size) {
return true;
}
- if ((addr >= dms->base)
+ if (dms && (addr >= dms->base)
&& ((addr - dms->base) < memory_region_size(&dms->mr))) {
return true;
}
--
2.40.1
On 6/1/23 14:14, David Hildenbrand wrote:
> Let's use our new helper and stop always allocating ms->device_memory.
> There is no difference in common memory-device code anymore between
> ms->device_memory being NULL or the size being 0. So we only have to
> teach spapr code that ms->device_memory isn't always around.
>
> We can now modify two maxram_size checks to rely on ms->device_memory
> for detecting whether we have memory devices.
>
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Greg Kurz <groug@kaod.org>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/ppc/spapr.c | 37 +++++++++++++++++++------------------
> hw/ppc/spapr_hcall.c | 2 +-
> 2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index dcb7f1c70a..b53aa4b5a8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -546,10 +546,8 @@ static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
> cpu_to_be32(lmb_size & 0xffffffff)};
> MemoryDeviceInfoList *dimms = NULL;
>
> - /*
> - * Don't create the node if there is no device memory
> - */
> - if (machine->ram_size == machine->maxram_size) {
> + /* Don't create the node if there is no device memory. */
> + if (!machine->device_memory) {
> return 0;
> }
>
> @@ -859,16 +857,23 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> int rtas;
> GString *hypertas = g_string_sized_new(256);
> GString *qemu_hypertas = g_string_sized_new(256);
> - uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
> - memory_region_size(&MACHINE(spapr)->device_memory->mr);
> uint32_t lrdr_capacity[] = {
> - cpu_to_be32(max_device_addr >> 32),
> - cpu_to_be32(max_device_addr & 0xffffffff),
> + 0,
> + 0,
> cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE >> 32),
> cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE & 0xffffffff),
> cpu_to_be32(ms->smp.max_cpus / ms->smp.threads),
> };
>
> + /* Do we have device memory? */
> + if (MACHINE(spapr)->device_memory) {
> + uint64_t max_device_addr = MACHINE(spapr)->device_memory->base +
> + memory_region_size(&MACHINE(spapr)->device_memory->mr);
> +
> + lrdr_capacity[0] = cpu_to_be32(max_device_addr >> 32);
> + lrdr_capacity[1] = cpu_to_be32(max_device_addr & 0xffffffff);
> + }
> +
> _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
>
> /* hypertas */
> @@ -2454,6 +2459,7 @@ static void spapr_create_lmb_dr_connectors(SpaprMachineState *spapr)
> uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
> int i;
>
> + g_assert(!nr_lmbs || machine->device_memory);
> for (i = 0; i < nr_lmbs; i++) {
> uint64_t addr;
>
> @@ -2866,12 +2872,11 @@ static void spapr_machine_init(MachineState *machine)
> /* map RAM */
> memory_region_add_subregion(sysmem, 0, machine->ram);
>
> - /* always allocate the device memory information */
> - machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
> -
> /* initialize hotplug memory address space */
> if (machine->ram_size < machine->maxram_size) {
> ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
> + hwaddr device_mem_base;
> +
> /*
> * Limit the number of hotpluggable memory slots to half the number
> * slots that KVM supports, leaving the other half for PCI and other
> @@ -2890,12 +2895,8 @@ static void spapr_machine_init(MachineState *machine)
> exit(1);
> }
>
> - machine->device_memory->base = ROUND_UP(machine->ram_size,
> - SPAPR_DEVICE_MEM_ALIGN);
> - memory_region_init(&machine->device_memory->mr, OBJECT(spapr),
> - "device-memory", device_mem_size);
> - memory_region_add_subregion(sysmem, machine->device_memory->base,
> - &machine->device_memory->mr);
> + device_mem_base = ROUND_UP(machine->ram_size, SPAPR_DEVICE_MEM_ALIGN);
> + machine_memory_devices_init(machine, device_mem_base, device_mem_size);
> }
>
> if (smc->dr_lmb_enabled) {
> @@ -5109,7 +5110,7 @@ static bool phb_placement_2_7(SpaprMachineState *spapr, uint32_t index,
> int i;
>
> /* Do we have device memory? */
> - if (MACHINE(spapr)->maxram_size > ram_top) {
> + if (MACHINE(spapr)->device_memory) {
> /* Can't just use maxram_size, because there may be an
> * alignment gap between normal and device memory regions
> */
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index b904755575..1dd32f340f 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -31,7 +31,7 @@ bool is_ram_address(SpaprMachineState *spapr, hwaddr addr)
> if (addr < machine->ram_size) {
> return true;
> }
> - if ((addr >= dms->base)
> + if (dms && (addr >= dms->base)
> && ((addr - dms->base) < memory_region_size(&dms->mr))) {
> return true;
> }
Let's use our new helper. While at it, use VIRT_HIGHMEM_BASE.
Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Cc: Song Gao <gaosong@loongson.cn>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Song Gao <gaosong@loongson.cn>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/loongarch/virt.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index ceddec1b23..c1327a51c4 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -805,8 +805,8 @@ static void loongarch_init(MachineState *machine)
/* initialize device memory address space */
if (machine->ram_size < machine->maxram_size) {
- machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+ hwaddr device_mem_base;
if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
error_report("unsupported amount of memory slots: %"PRIu64,
@@ -821,14 +821,8 @@ static void loongarch_init(MachineState *machine)
exit(EXIT_FAILURE);
}
/* device memory base is the top of high memory address. */
- machine->device_memory->base = 0x90000000 + highram_size;
- machine->device_memory->base =
- ROUND_UP(machine->device_memory->base, 1 * GiB);
-
- memory_region_init(&machine->device_memory->mr, OBJECT(lams),
- "device-memory", device_mem_size);
- memory_region_add_subregion(address_space_mem, machine->device_memory->base,
- &machine->device_memory->mr);
+ device_mem_base = ROUND_UP(VIRT_HIGHMEM_BASE + highram_size, 1 * GiB);
+ machine_memory_devices_init(machine, device_mem_base, device_mem_size);
}
/* Add isa io region */
--
2.40.1
Let's use our new helper and stop always allocating ms->device_memory.
Once allcoated, we're sure that the size > 0 and that the base was
initialized.
Adjust the code in pc_memory_init() to check for machine->device_memory
instead of pcmc->has_reserved_memory and machine->device_memory->base.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/i386/pc.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bb62c994fa..3d7f991368 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1039,13 +1039,11 @@ void pc_memory_init(PCMachineState *pcms,
exit(EXIT_FAILURE);
}
- /* always allocate the device memory information */
- machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
-
/* initialize device memory address space */
if (pcmc->has_reserved_memory &&
(machine->ram_size < machine->maxram_size)) {
ram_addr_t device_mem_size;
+ hwaddr device_mem_base;
if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
error_report("unsupported amount of memory slots: %"PRIu64,
@@ -1060,19 +1058,14 @@ void pc_memory_init(PCMachineState *pcms,
exit(EXIT_FAILURE);
}
- pc_get_device_memory_range(pcms, &machine->device_memory->base, &device_mem_size);
+ pc_get_device_memory_range(pcms, &device_mem_base, &device_mem_size);
- if ((machine->device_memory->base + device_mem_size) <
- device_mem_size) {
+ if (device_mem_base + device_mem_size < device_mem_size) {
error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
machine->maxram_size);
exit(EXIT_FAILURE);
}
-
- memory_region_init(&machine->device_memory->mr, OBJECT(pcms),
- "device-memory", device_mem_size);
- memory_region_add_subregion(system_memory, machine->device_memory->base,
- &machine->device_memory->mr);
+ machine_memory_devices_init(machine, device_mem_base, device_mem_size);
}
if (pcms->cxl_devices_state.is_enabled) {
@@ -1120,7 +1113,7 @@ void pc_memory_init(PCMachineState *pcms,
rom_set_fw(fw_cfg);
- if (pcmc->has_reserved_memory && machine->device_memory->base) {
+ if (machine->device_memory) {
uint64_t *val = g_malloc(sizeof(*val));
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
uint64_t res_mem_end = machine->device_memory->base;
--
2.40.1
We're already looking at machine->device_memory when calling
build_srat_memory(), so let's simply avoid going via
PC_MACHINE_DEVMEM_REGION_SIZE to get the size and rely on
machine->device_memory directly.
Once machine->device_memory is set, we know that the size > 0. The code now
looks much more similar the hw/arm/virt-acpi-build.c variant.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/i386/acpi-build.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 512162003b..9c74fa17ad 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1950,12 +1950,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
MachineClass *mc = MACHINE_GET_CLASS(machine);
X86MachineState *x86ms = X86_MACHINE(machine);
const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
- PCMachineState *pcms = PC_MACHINE(machine);
int nb_numa_nodes = machine->numa_state->num_nodes;
NodeInfo *numa_info = machine->numa_state->nodes;
- ram_addr_t hotpluggable_address_space_size =
- object_property_get_int(OBJECT(pcms), PC_MACHINE_DEVMEM_REGION_SIZE,
- NULL);
AcpiTable table = { .sig = "SRAT", .rev = 1, .oem_id = x86ms->oem_id,
.oem_table_id = x86ms->oem_table_id };
@@ -2071,9 +2067,10 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
* Memory devices may override proximity set by this entry,
* providing _PXM method if necessary.
*/
- if (hotpluggable_address_space_size) {
+ if (machine->device_memory) {
build_srat_memory(table_data, machine->device_memory->base,
- hotpluggable_address_space_size, nb_numa_nodes - 1,
+ memory_region_size(&machine->device_memory->mr),
+ nb_numa_nodes - 1,
MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
}
--
2.40.1
There are no remaining users in the tree. Libvirt never used that
property and a quick internet search revealed no other users.
Further, we renamed that property already in commit f2ffbe2b7dd0
("pc: rename "hotplug memory" terminology to "device memory"") without
anybody complaining.
So let's just get rid of it.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/i386/pc.c | 19 -------------------
include/hw/i386/pc.h | 1 -
2 files changed, 20 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3d7f991368..96334cf60d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1646,21 +1646,6 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine,
return NULL;
}
-static void
-pc_machine_get_device_memory_region_size(Object *obj, Visitor *v,
- const char *name, void *opaque,
- Error **errp)
-{
- MachineState *ms = MACHINE(obj);
- int64_t value = 0;
-
- if (ms->device_memory) {
- value = memory_region_size(&ms->device_memory->mr);
- }
-
- visit_type_int(v, name, &value, errp);
-}
-
static void pc_machine_get_vmport(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -1980,10 +1965,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
object_class_property_set_description(oc, PC_MACHINE_MAX_RAM_BELOW_4G,
"Maximum ram below the 4G boundary (32bit boundary)");
- object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
- pc_machine_get_device_memory_region_size, NULL,
- NULL, NULL);
-
object_class_property_add(oc, PC_MACHINE_VMPORT, "OnOffAuto",
pc_machine_get_vmport, pc_machine_set_vmport,
NULL, NULL);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index c661e9cc80..6c9ad2d132 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -60,7 +60,6 @@ typedef struct PCMachineState {
#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
-#define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
#define PC_MACHINE_VMPORT "vmport"
#define PC_MACHINE_SMBUS "smbus"
#define PC_MACHINE_SATA "sata"
--
2.40.1
Let's move memory_device_check_addable() and basic checks out of
memory_device_get_free_addr() directly into memory_device_pre_plug().
Separating basic checks from address assignment is cleaner and
prepares for further changes.
As all memory device users now use memory_devices_init(), and that
function enforces that the size is 0, we can drop the check for an empty
region.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index bb9d7c2a20..00c7755557 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -69,9 +69,10 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
return 0;
}
-static void memory_device_check_addable(MachineState *ms, uint64_t size,
+static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
Error **errp)
{
+ const uint64_t size = memory_region_size(mr);
uint64_t used_region_size = 0;
/* we will need a new memory slot for kvm and vhost */
@@ -101,16 +102,9 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
uint64_t align, uint64_t size,
Error **errp)
{
- Error *err = NULL;
GSList *list = NULL, *item;
Range as, new = range_empty;
- if (!ms->device_memory || !memory_region_size(&ms->device_memory->mr)) {
- error_setg(errp, "the configuration is not prepared for memory devices"
- " (e.g., for memory hotplug), consider specifying the"
- " maxmem option");
- return 0;
- }
range_init_nofail(&as, ms->device_memory->base,
memory_region_size(&ms->device_memory->mr));
@@ -122,12 +116,6 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
align);
}
- memory_device_check_addable(ms, size, &err);
- if (err) {
- error_propagate(errp, err);
- return 0;
- }
-
if (hint && !QEMU_IS_ALIGNED(*hint, align)) {
error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
align);
@@ -251,11 +239,23 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
uint64_t addr, align = 0;
MemoryRegion *mr;
+ if (!ms->device_memory) {
+ error_setg(errp, "the configuration is not prepared for memory devices"
+ " (e.g., for memory hotplug), consider specifying the"
+ " maxmem option");
+ return;
+ }
+
mr = mdc->get_memory_region(md, &local_err);
if (local_err) {
goto out;
}
+ memory_device_check_addable(ms, mr, &local_err);
+ if (local_err) {
+ goto out;
+ }
+
if (legacy_align) {
align = *legacy_align;
} else {
--
2.40.1On 1/6/23 14:14, David Hildenbrand wrote: > Let's move memory_device_check_addable() and basic checks out of > memory_device_get_free_addr() directly into memory_device_pre_plug(). > > Separating basic checks from address assignment is cleaner and > prepares for further changes. > > As all memory device users now use memory_devices_init(), and that > function enforces that the size is 0, we can drop the check for an empty > region. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/mem/memory-device.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Let's avoid iterating over all devices and simply track it in the
DeviceMemoryState.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 22 +++-------------------
include/hw/boards.h | 2 ++
2 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 00c7755557..667d56bd29 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -52,28 +52,11 @@ static int memory_device_build_list(Object *obj, void *opaque)
return 0;
}
-static int memory_device_used_region_size(Object *obj, void *opaque)
-{
- uint64_t *size = opaque;
-
- if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
- const DeviceState *dev = DEVICE(obj);
- const MemoryDeviceState *md = MEMORY_DEVICE(obj);
-
- if (dev->realized) {
- *size += memory_device_get_region_size(md, &error_abort);
- }
- }
-
- object_child_foreach(obj, memory_device_used_region_size, opaque);
- return 0;
-}
-
static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
Error **errp)
{
+ const uint64_t used_region_size = ms->device_memory->used_region_size;
const uint64_t size = memory_region_size(mr);
- uint64_t used_region_size = 0;
/* we will need a new memory slot for kvm and vhost */
if (kvm_enabled() && !kvm_has_free_slot(ms)) {
@@ -86,7 +69,6 @@ static void memory_device_check_addable(MachineState *ms, MemoryRegion *mr,
}
/* will we exceed the total amount of memory specified */
- memory_device_used_region_size(OBJECT(ms), &used_region_size);
if (used_region_size + size < used_region_size ||
used_region_size + size > ms->maxram_size - ms->ram_size) {
error_setg(errp, "not enough space, currently 0x%" PRIx64
@@ -292,6 +274,7 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
mr = mdc->get_memory_region(md, &error_abort);
g_assert(ms->device_memory);
+ ms->device_memory->used_region_size += memory_region_size(mr);
memory_region_add_subregion(&ms->device_memory->mr,
addr - ms->device_memory->base, mr);
trace_memory_device_plug(DEVICE(md)->id ? DEVICE(md)->id : "", addr);
@@ -310,6 +293,7 @@ void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
g_assert(ms->device_memory);
memory_region_del_subregion(&ms->device_memory->mr, mr);
+ ms->device_memory->used_region_size -= memory_region_size(mr);
trace_memory_device_unplug(DEVICE(md)->id ? DEVICE(md)->id : "",
mdc->get_addr(md));
}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index be06e8a41f..fcaf40b9da 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -296,11 +296,13 @@ struct MachineClass {
* address space for memory devices starts
* @mr: address space container for memory devices
* @dimm_size: the sum of plugged DIMMs' sizes
+ * @used_region_size: the part of @mr already used by memory devices
*/
typedef struct DeviceMemoryState {
hwaddr base;
MemoryRegion mr;
uint64_t dimm_size;
+ uint64_t used_region_size;
} DeviceMemoryState;
/**
--
2.40.1
© 2016 - 2024 Red Hat, Inc.