[PATCH 00/10] memory-device: Some cleanups

[PATCH 00/10] memory-device: Some cleanups

[David Hildenbrand]

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.

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 memory_devices_init()
  hw/arm/virt: Use memory_devices_init()
  hw/ppc/spapr: Use memory_devices_init()
  hw/loongarch/virt: Use memory_devices_init()
  hw/i386/pc: Use 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            | 14 ++-----
 hw/mem/memory-device.c         | 69 +++++++++++++++-------------------
 hw/ppc/spapr.c                 | 37 +++++++++---------
 hw/ppc/spapr_hcall.c           |  2 +-
 include/hw/boards.h            |  2 +
 include/hw/i386/pc.h           |  1 -
 include/hw/mem/memory-device.h |  2 +
 10 files changed, 68 insertions(+), 113 deletions(-)

-- 
2.40.1

[PATCH 01/10] memory-device: Unify enabled vs. supported error messages

[David Hildenbrand]

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 ...

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

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

[David Hildenbrand]

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/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 49f86ec8a8..2197dcf356 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 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/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);
 
 #endif
-- 
2.40.1

[PATCH 03/10] hw/arm/virt: Use memory_devices_init()

[David Hildenbrand]

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>
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..49bc8378ec 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);
+        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

[PATCH 04/10] hw/ppc/spapr: Use memory_devices_init()

[David Hildenbrand]

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>
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..44155f6613 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);
+        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

[PATCH 05/10] hw/loongarch/virt: Use memory_devices_init()

[David Hildenbrand]

Let's use our new helper.

Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Cc: Song Gao <gaosong@loongson.cn>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/loongarch/virt.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index ceddec1b23..a6790714fe 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -45,7 +45,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/block/flash.h"
 #include "qemu/error-report.h"
-
+#include "hw/mem/memory-device.h"
 
 static void virt_flash_create(LoongArchMachineState *lams)
 {
@@ -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(0x90000000 + highram_size, 1 * GiB);
+        memory_devices_init(machine, device_mem_base, device_mem_size);
     }
 
     /* Add isa io region */
-- 
2.40.1

[PATCH 06/10] hw/i386/pc: Use memory_devices_init()

[David Hildenbrand]

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>
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..920aa32b53 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);
+        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

[PATCH 07/10] hw/i386/acpi-build: Rely on machine->device_memory when building SRAT

[David Hildenbrand]

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>
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

[PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[David Hildenbrand]

There are no remaining users in the tree, so let's remove 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>
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 920aa32b53..c4789e2f35 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

[PATCH 09/10] memory-device: Refactor memory_device_pre_plug()

[David Hildenbrand]

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 2197dcf356..616067cbdc 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.1

[PATCH 10/10] memory-device: Track used region size in DeviceMemoryState

[David Hildenbrand]

Let's avoid iterating over all devices and simply track it in the
DeviceMemoryState.

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 616067cbdc..b8eb2a7188 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 a385010909..9d48c73edb 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -294,11 +294,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

Re: [PATCH 00/10] memory-device: Some cleanups

[David Hildenbrand]

This was supposed to carry the "v2" indicator ;)


On 30.05.23 13:38, 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.
> 
> 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 memory_devices_init()
>    hw/arm/virt: Use memory_devices_init()
>    hw/ppc/spapr: Use memory_devices_init()
>    hw/loongarch/virt: Use memory_devices_init()
>    hw/i386/pc: Use 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            | 14 ++-----
>   hw/mem/memory-device.c         | 69 +++++++++++++++-------------------
>   hw/ppc/spapr.c                 | 37 +++++++++---------
>   hw/ppc/spapr_hcall.c           |  2 +-
>   include/hw/boards.h            |  2 +
>   include/hw/i386/pc.h           |  1 -
>   include/hw/mem/memory-device.h |  2 +
>   10 files changed, 68 insertions(+), 113 deletions(-)
> 

-- 
Thanks,

David / dhildenb

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

[Philippe Mathieu-Daudé]

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(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/10] hw/arm/virt: Use memory_devices_init()

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> 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>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/arm/virt.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 05/10] hw/loongarch/virt: Use memory_devices_init()

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> Let's use our new helper.
> 
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/loongarch/virt.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/10] hw/i386/acpi-build: Rely on machine->device_memory when building SRAT

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> 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>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/i386/acpi-build.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> There are no remaining users in the tree, so let's remove 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>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/i386/pc.c         | 19 -------------------
>   include/hw/i386/pc.h |  1 -
>   2 files changed, 20 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/10] memory-device: Track used region size in DeviceMemoryState

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> Let's avoid iterating over all devices and simply track it in the
> DeviceMemoryState.
> 
> 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(-)

Nice.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

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".

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".

What do you think?

Re: [PATCH 05/10] hw/loongarch/virt: Use memory_devices_init()

[Song Gao]

在 2023/5/30 下午7:38, David Hildenbrand 写道:
> Let's use our new helper.
>
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/loongarch/virt.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index ceddec1b23..a6790714fe 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -45,7 +45,7 @@
>   #include "sysemu/block-backend.h"
>   #include "hw/block/flash.h"
>   #include "qemu/error-report.h"
> -
> +#include "hw/mem/memory-device.h"
>   
>   static void virt_flash_create(LoongArchMachineState *lams)
>   {
> @@ -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(0x90000000 + highram_size, 1 * GiB);
Use  VIRT_HIGHMEM_BASE.

Otherwise,
Reviewed-by: Song Gao <gaosong@loongson.cn>

Thanks.
Song Gao
> +        memory_devices_init(machine, device_mem_base, device_mem_size);
>       }
>   
>       /* Add isa io region */

Re: [PATCH 01/10] memory-device: Unify enabled vs. supported error messages

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> 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 ...
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/mem/memory-device.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/10] hw/ppc/spapr: Use memory_devices_init()

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, 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>
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/10] hw/i386/pc: Use memory_devices_init()

[Philippe Mathieu-Daudé]

On 30/5/23 13:38, David Hildenbrand wrote:
> 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>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/i386/pc.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[David Hildenbrand]

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

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[Michael S. Tsirkin]

On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote:
> There are no remaining users in the tree, so let's remove 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>
> Signed-off-by: David Hildenbrand <david@redhat.com>


This (with previous patches) means any user changing
device-memory-region-size machine property is now broken, right?
How do we know there are no users?

> ---
>  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 920aa32b53..c4789e2f35 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

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[David Hildenbrand]

On 30.05.23 15:07, Michael S. Tsirkin wrote:
> On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote:
>> There are no remaining users in the tree, so let's remove 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>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> 
> This (with previous patches) means any user changing
> device-memory-region-size machine property is now broken, right?

We only had a getter, no setter (for good reason).

> How do we know there are no users?

We don't. A quick google search makes "device-memory-region-size" and 
"qom-get" only pop up in BUG fixes for something that appears to be QEMU 
developer driven.

I don't consider it any useful, but if we want to be careful, sure we 
can leave it around.

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 30.05.23 15:04, David Hildenbrand wrote:
> 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!

FWIW, I went with "machine_memory_devices_init()", to mach the style of 
"machine_run_board_init()".

-- 
Thanks,

David / dhildenb

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[David Hildenbrand]

On 30.05.23 15:11, David Hildenbrand wrote:
> On 30.05.23 15:07, Michael S. Tsirkin wrote:
>> On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote:
>>> There are no remaining users in the tree, so let's remove 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>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>>
>> This (with previous patches) means any user changing
>> device-memory-region-size machine property is now broken, right?
> 
> We only had a getter, no setter (for good reason).
> 
>> How do we know there are no users?
> 
> We don't. A quick google search makes "device-memory-region-size" and
> "qom-get" only pop up in BUG fixes for something that appears to be QEMU
> developer driven.
> 
> I don't consider it any useful, but if we want to be careful, sure we
> can leave it around.

Looking further, libvirt doesn't use it (and never used it).

I already renamed it in 2018 without anybody complaining:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg532101.html

So I'm quite confident that nobody will really miss this property.

-- 
Thanks,

David / dhildenb

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[Philippe Mathieu-Daudé]

On 30/5/23 15:11, David Hildenbrand wrote:
> On 30.05.23 15:07, Michael S. Tsirkin wrote:
>> On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote:
>>> There are no remaining users in the tree, so let's remove 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>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>>
>> This (with previous patches) means any user changing
>> device-memory-region-size machine property is now broken, right?
> 
> We only had a getter, no setter (for good reason).
> 
>> How do we know there are no users?
> 
> We don't. A quick google search makes "device-memory-region-size" and 
> "qom-get" only pop up in BUG fixes for something that appears to be QEMU 
> developer driven.

This was my analysis.
> I don't consider it any useful, but if we want to be careful, sure we 
> can leave it around.

If we want to keep it, we should move it to generic code IMHO,
not PC machine. Otherwise the less unused code the better :)

Re: [PATCH 08/10] hw/i386/pc: Remove PC_MACHINE_DEVMEM_REGION_SIZE

[Michael S. Tsirkin]

On Tue, May 30, 2023 at 03:41:58PM +0200, David Hildenbrand wrote:
> On 30.05.23 15:11, David Hildenbrand wrote:
> > On 30.05.23 15:07, Michael S. Tsirkin wrote:
> > > On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote:
> > > > There are no remaining users in the tree, so let's remove 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>
> > > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > 
> > > 
> > > This (with previous patches) means any user changing
> > > device-memory-region-size machine property is now broken, right?
> > 
> > We only had a getter, no setter (for good reason).
> > 
> > > How do we know there are no users?
> > 
> > We don't. A quick google search makes "device-memory-region-size" and
> > "qom-get" only pop up in BUG fixes for something that appears to be QEMU
> > developer driven.
> > 
> > I don't consider it any useful, but if we want to be careful, sure we
> > can leave it around.
> 
> Looking further, libvirt doesn't use it (and never used it).
> 
> I already renamed it in 2018 without anybody complaining:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg532101.html
> 
> So I'm quite confident that nobody will really miss this property.
> 
> -- 
> Thanks,
> 
> David / dhildenb

OK. In the future we need to be careful and use "x-" prefix for what
we don't want to expose.

-- 
MST

Re: [PATCH 00/10] memory-device: Some cleanups

[Michael S. Tsirkin]

On Tue, May 30, 2023 at 01:38:28PM +0200, 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.

pc/acpi things:

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> 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 memory_devices_init()
>   hw/arm/virt: Use memory_devices_init()
>   hw/ppc/spapr: Use memory_devices_init()
>   hw/loongarch/virt: Use memory_devices_init()
>   hw/i386/pc: Use 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            | 14 ++-----
>  hw/mem/memory-device.c         | 69 +++++++++++++++-------------------
>  hw/ppc/spapr.c                 | 37 +++++++++---------
>  hw/ppc/spapr_hcall.c           |  2 +-
>  include/hw/boards.h            |  2 +
>  include/hw/i386/pc.h           |  1 -
>  include/hw/mem/memory-device.h |  2 +
>  10 files changed, 68 insertions(+), 113 deletions(-)
> 
> -- 
> 2.40.1

Re: [PATCH 00/10] memory-device: Some cleanups

[David Hildenbrand]

On 22.06.23 22:13, Michael S. Tsirkin wrote:
> On Tue, May 30, 2023 at 01:38:28PM +0200, 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.
> 
> pc/acpi things:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks, I added that to patch 6/7/8.

-- 
Cheers,

David / dhildenb