[Qemu-devel] [PATCH v1 0/4] pc-dimm: pre_plug "slot" and "addr" assignment

[Qemu-devel] [PATCH v1 0/4] pc-dimm: pre_plug "slot" and "addr" assignment

[David Hildenbrand]

As requested by Igor, assign and verify "slot" and "addr" in the
pre_plug handler. Factor out the compatibility handling/configuration
for detecting the alignment to be used when searching for an address
in guest physical memory for a memory device.

This is another part of the original series
    [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on
    [PATCH v1 0/2] memory: fix alignment checks/asserts
    [PATCH v4 00/12] pc-dimm: next bunch of cleanups

This refactoring is the last step before factoring out pre_plug, plug and
unplug logic of memory devices completely into memory-device.c

David Hildenbrand (4):
  pc-dimm: assign and verify the "slot" property during pre_plug
  machine: factor out enforce_aligned_dimm into memory_device_align
  pc-dimm/memory-device: detect alignment internally
  pc-dimm: assign and verify the "addr" property during pre_plug

 hw/core/machine.c              |  3 ++
 hw/i386/pc.c                   | 20 ++++-------
 hw/i386/pc_piix.c              |  2 +-
 hw/mem/Makefile.objs           |  2 +-
 hw/mem/memory-device.c         | 25 ++++++++++++++
 hw/mem/pc-dimm.c               | 62 ++++++++++++++++++----------------
 hw/ppc/spapr.c                 |  6 ++--
 include/hw/boards.h            | 13 +++++++
 include/hw/i386/pc.h           |  3 --
 include/hw/mem/memory-device.h |  1 +
 include/hw/mem/pc-dimm.h       |  4 +--
 11 files changed, 87 insertions(+), 54 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 1/4] pc-dimm: assign and verify the "slot" property during pre_plug

[David Hildenbrand]

We can assign and verify the slot before realizing and trying to plug.
reading/writing the slot property should never fail, so let's reduce
error handling a bit by using &error_abort.

To do this during pre_plug, add and use (x86, ppc) pc_dimm_pre_plug().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             |  2 ++
 hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
 hw/ppc/spapr.c           |  1 +
 include/hw/mem/pc-dimm.h |  1 +
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f310040351..bf986baf91 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
         return;
     }
+
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
 }
 
 static void pc_memory_plug(HotplugHandler *hotplug_dev,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 65843bc52a..e56c4daef2 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -29,10 +29,27 @@
 
 static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
+void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
+{
+    Error *local_err = NULL;
+    int slot;
+
+    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
+                                   &error_abort);
+    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
+                                 machine->ram_slots, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
+    trace_mhp_pc_dimm_assigned_slot(slot);
+out:
+    error_propagate(errp, local_err);
+}
+
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp)
 {
-    int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
@@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     }
     trace_mhp_pc_dimm_assigned_address(addr);
 
-    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
-                                 machine->ram_slots, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    trace_mhp_pc_dimm_assigned_slot(slot);
-
     memory_device_plug_region(machine, mr, addr);
     vmstate_register_ram(vmstate_mr, dev);
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6934abc21e..9da233588b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3211,6 +3211,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
 out:
     g_free(mem_dev);
 }
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 26ebb7d5e9..7b120416d1 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -79,6 +79,7 @@ typedef struct PCDIMMDeviceClass {
                                                Error **errp);
 } PCDIMMDeviceClass;
 
+void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp);
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp);
 void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
-- 
2.17.1

[Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[David Hildenbrand]

We want to handle memory device address assignment without passing
compatibility parameters ("*align").

As x86 and Power use different strategies to determine an alignment and
we need clean support for compat handling, let's introduce an enum on
the machine class level. This is the machine configuration on how to
align memory devices in guest physical memory.

The three introduced types represent what is being done on x86 and Power
right now.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c    |  3 +++
 hw/i386/pc.c         | 13 +++++++------
 hw/i386/pc_piix.c    |  2 +-
 include/hw/boards.h  | 13 +++++++++++++
 include/hw/i386/pc.h |  3 ---
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 617e5f8d75..d34b205125 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -531,6 +531,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_align_shift = 23;
     mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
 
+    /* Default: use memory region alignment as memory devices alignment */
+    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION;
+
     object_class_property_add_str(oc, "accel",
         machine_get_accel, machine_set_accel, &error_abort);
     object_class_property_set_description(oc, "accel",
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bf986baf91..04a97e89e7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1331,6 +1331,7 @@ void pc_memory_init(PCMachineState *pcms,
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
 
     assert(machine->ram_size == pcms->below_4g_mem_size +
                                 pcms->above_4g_mem_size);
@@ -1363,8 +1364,6 @@ void pc_memory_init(PCMachineState *pcms,
     if (!pcmc->has_reserved_memory &&
         (machine->ram_slots ||
          (machine->maxram_size > machine->ram_size))) {
-        MachineClass *mc = MACHINE_GET_CLASS(machine);
-
         error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
                      mc->name);
         exit(EXIT_FAILURE);
@@ -1394,7 +1393,7 @@ void pc_memory_init(PCMachineState *pcms,
         machine->device_memory->base =
             ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
 
-        if (pcmc->enforce_aligned_dimm) {
+        if (mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
             /* size device region assuming 1G page max alignment per slot */
             device_mem_size += (1ULL << 30) * machine->ram_slots;
         }
@@ -1705,14 +1704,16 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
+
+    if (memory_region_get_alignment(mr) &&
+        mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
         align = memory_region_get_alignment(mr);
     }
 
@@ -2374,7 +2375,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->gigabyte_align = true;
     pcmc->has_reserved_memory = true;
     pcmc->kvmclock_enabled = true;
-    pcmc->enforce_aligned_dimm = true;
     /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
      * to be used at the moment, 32K should be enough for a while.  */
     pcmc->acpi_data_size = 0x20000 + 0x8000;
@@ -2398,6 +2398,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     nc->nmi_monitor_handler = x86_nmi;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
+    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION_OR_PAGE;
 
     object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
         pc_machine_get_device_memory_region_size, NULL,
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3b87f3cedb..cc11856c24 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -566,7 +566,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
     m->default_display = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
     pcmc->smbios_uuid_encoded = false;
-    pcmc->enforce_aligned_dimm = false;
+    m->memory_device_align = MEMORY_DEVICE_ALIGN_PAGE;
 }
 
 DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index ef7457f5dd..3f151207c1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -105,6 +105,15 @@ typedef struct {
     CPUArchId cpus[0];
 } CPUArchIdList;
 
+typedef enum MemoryDeviceAlign {
+    /* use specified memory region alignment */
+    MEMORY_DEVICE_ALIGN_REGION = 0,
+    /* use target page size as alignment */
+    MEMORY_DEVICE_ALIGN_PAGE,
+    /* use target page size if no memory region alignment has been specified */
+    MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
+} MemoryDeviceAlign;
+
 /**
  * MachineClass:
  * @max_cpus: maximum number of CPUs supported. Default: 1
@@ -156,6 +165,9 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @memory_device_align: The alignment that will be used as default when
+ *    searching for a guest physical memory address while plugging a
+ *    memory device. This is relevant for compatibility handling.
  */
 struct MachineClass {
     /*< private >*/
@@ -202,6 +214,7 @@ struct MachineClass {
     const char **valid_cpu_types;
     strList *allowed_dynamic_sysbus_devices;
     bool auto_enable_numa_with_memhp;
+    MemoryDeviceAlign memory_device_align;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fc8dedca12..ffb4654fc8 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -86,8 +86,6 @@ struct PCMachineState {
  *
  * Compat fields:
  *
- * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
- *                        backend's alignment value if provided
  * @acpi_data_size: Size of the chunk of memory at the top of RAM
  *                  for the BIOS ACPI tables and other BIOS
  *                  datastructures.
@@ -124,7 +122,6 @@ struct PCMachineClass {
     /* RAM / address space compat: */
     bool gigabyte_align;
     bool has_reserved_memory;
-    bool enforce_aligned_dimm;
     bool broken_reserved_end;
 
     /* TSC rate migration: */
-- 
2.17.1

[Qemu-devel] [PATCH v1 3/4] pc-dimm/memory-device: detect alignment internally

[David Hildenbrand]

We can now avoid having to pass in the alignment explicitly but can
instead make use of the new machine compat parameter
"memory_device_align".

As we are using TARGET_PAGE_SIZE in memory-device.c, we can now longer
compile it as common object.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c                   | 13 +------------
 hw/mem/Makefile.objs           |  2 +-
 hw/mem/memory-device.c         | 25 +++++++++++++++++++++++++
 hw/mem/pc-dimm.c               |  4 ++--
 hw/ppc/spapr.c                 |  5 ++---
 include/hw/mem/memory-device.h |  1 +
 include/hw/mem/pc-dimm.h       |  3 +--
 7 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 04a97e89e7..d5581ab0a1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1704,20 +1704,9 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
-    uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-
-    if (memory_region_get_alignment(mr) &&
-        mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
-        align = memory_region_get_alignment(mr);
-    }
-
-    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(pcms), &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
index 10be4df2a2..f519441091 100644
--- a/hw/mem/Makefile.objs
+++ b/hw/mem/Makefile.objs
@@ -1,3 +1,3 @@
 common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
-common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
+obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 6de4f70bb4..968c64be97 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -249,6 +249,31 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+uint64_t memory_device_get_align(MachineState *ms, MemoryRegion *mr) {
+    const MachineClass *mc = MACHINE_GET_CLASS(ms);
+    uint64_t align;
+
+    /* use the configured memory device alignment (compat handling) */
+    switch (mc->memory_device_align) {
+    case MEMORY_DEVICE_ALIGN_REGION:
+        align = memory_region_get_alignment(mr);;
+        break;
+    case MEMORY_DEVICE_ALIGN_PAGE:
+        align = TARGET_PAGE_SIZE;
+        break;
+    case MEMORY_DEVICE_ALIGN_REGION_OR_PAGE:
+        align = memory_region_get_alignment(mr);
+        if (!align) {
+            align = TARGET_PAGE_SIZE;
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return align;
+}
+
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr)
 {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index e56c4daef2..9198104d34 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -47,14 +47,14 @@ out:
     error_propagate(errp, local_err);
 }
 
-void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
-                  Error **errp)
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
                                                               &error_abort);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
+    const uint64_t align = memory_device_get_align(machine, mr);
     Error *local_err = NULL;
     uint64_t addr;
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9da233588b..03752c6aaf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3143,13 +3143,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
-    uint64_t align, size, addr;
+    uint64_t size, addr;
     uint32_t node;
 
-    align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
-    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(ms), &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 2853b084b5..32cd15a0cb 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -44,6 +44,7 @@ uint64_t get_plugged_memory_size(void);
 uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      uint64_t align, uint64_t size,
                                      Error **errp);
+uint64_t memory_device_get_align(MachineState *ms, MemoryRegion *mr);
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr);
 void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 7b120416d1..ba9f7e7146 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -80,7 +80,6 @@ typedef struct PCDIMMDeviceClass {
 } PCDIMMDeviceClass;
 
 void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp);
-void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
-                  Error **errp);
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp);
 void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v1 4/4] pc-dimm: assign and verify the "addr" property during pre_plug

[David Hildenbrand]

We can assign and verify the slot before realizing and trying to plug.
reading/writing the address property should never fail, so let's reduce
error handling a bit by using &error_abort. Getting access to the memory
region now might however fail. So forward errors from
get_memory_region() properly.

Keep tracing the assigned address for now in the plug code, as that's the
point we are sure plugging succeeded and the address willa ctually be
used.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9198104d34..42c4d0b750 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -31,7 +31,11 @@ static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
 {
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     Error *local_err = NULL;
+    uint64_t align, addr;
+    MemoryRegion *mr;
     int slot;
 
     slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -43,6 +47,22 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
     }
     object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
     trace_mhp_pc_dimm_assigned_slot(slot);
+
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    align = memory_device_get_align(machine, mr);
+
+    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
+                                    &error_abort);
+    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
+                                       memory_region_size(mr), &local_err);
+    if (local_err) {
+        goto out;
+    }
+    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
+                             &error_abort);
 out:
     error_propagate(errp, local_err);
 }
@@ -54,33 +74,14 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp)
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
                                                               &error_abort);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
-    const uint64_t align = memory_device_get_align(machine, mr);
-    Error *local_err = NULL;
     uint64_t addr;
 
-    addr = object_property_get_uint(OBJECT(dimm),
-                                    PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
-                                       memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
+    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
+                                    &error_abort);
     trace_mhp_pc_dimm_assigned_address(addr);
 
     memory_device_plug_region(machine, mr, addr);
     vmstate_register_ram(vmstate_mr, dev);
-
-out:
-    error_propagate(errp, local_err);
 }
 
 void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 1/4] pc-dimm: assign and verify the "slot" property during pre_plug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

On Mon, Jun 18, 2018 at 04:47:57PM +0200, David Hildenbrand wrote:
> We can assign and verify the slot before realizing and trying to plug.
> reading/writing the slot property should never fail, so let's reduce
> error handling a bit by using &error_abort.
> 
> To do this during pre_plug, add and use (x86, ppc) pc_dimm_pre_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

and ppc portion

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c             |  2 ++
>  hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
>  hw/ppc/spapr.c           |  1 +
>  include/hw/mem/pc-dimm.h |  1 +
>  4 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f310040351..bf986baf91 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>          return;
>      }
> +
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  }
>  
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 65843bc52a..e56c4daef2 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -29,10 +29,27 @@
>  
>  static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int slot;
> +
> +    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> +                                   &error_abort);
> +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> +                                 machine->ram_slots, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
> +    trace_mhp_pc_dimm_assigned_slot(slot);
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp)
>  {
> -    int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> @@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      }
>      trace_mhp_pc_dimm_assigned_address(addr);
>  
> -    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> -                                 machine->ram_slots, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    trace_mhp_pc_dimm_assigned_slot(slot);
> -
>      memory_device_plug_region(machine, mr, addr);
>      vmstate_register_ram(vmstate_mr, dev);
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6934abc21e..9da233588b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3211,6 +3211,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  out:
>      g_free(mem_dev);
>  }
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 26ebb7d5e9..7b120416d1 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -79,6 +79,7 @@ typedef struct PCDIMMDeviceClass {
>                                                 Error **errp);
>  } PCDIMMDeviceClass;
>  
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v1 1/4] pc-dimm: assign and verify the "slot" property during pre_plug

[Igor Mammedov]

On Mon, 18 Jun 2018 16:47:57 +0200
David Hildenbrand <david@redhat.com> wrote:

> We can assign and verify the slot before realizing and trying to plug.
> reading/writing the slot property should never fail, so let's reduce
> error handling a bit by using &error_abort.
> 
> To do this during pre_plug, add and use (x86, ppc) pc_dimm_pre_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c             |  2 ++
>  hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
>  hw/ppc/spapr.c           |  1 +
>  include/hw/mem/pc-dimm.h |  1 +
>  4 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f310040351..bf986baf91 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>          return;
>      }
> +
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  }
>  
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 65843bc52a..e56c4daef2 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -29,10 +29,27 @@
>  
>  static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int slot;
> +
> +    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> +                                   &error_abort);
> +    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> +                                 machine->ram_slots, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
> +    trace_mhp_pc_dimm_assigned_slot(slot);
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp)
>  {
> -    int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> @@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      }
>      trace_mhp_pc_dimm_assigned_address(addr);
>  
> -    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
> -                                 machine->ram_slots, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    trace_mhp_pc_dimm_assigned_slot(slot);
> -
>      memory_device_plug_region(machine, mr, addr);
>      vmstate_register_ram(vmstate_mr, dev);
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6934abc21e..9da233588b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3211,6 +3211,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  out:
>      g_free(mem_dev);
>  }
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 26ebb7d5e9..7b120416d1 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -79,6 +79,7 @@ typedef struct PCDIMMDeviceClass {
>                                                 Error **errp);
>  } PCDIMMDeviceClass;
>  
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);

Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[Igor Mammedov]

On Mon, 18 Jun 2018 16:47:58 +0200
David Hildenbrand <david@redhat.com> wrote:

> We want to handle memory device address assignment without passing
> compatibility parameters ("*align").
> 
> As x86 and Power use different strategies to determine an alignment and
> we need clean support for compat handling, let's introduce an enum on
> the machine class level. This is the machine configuration on how to
> align memory devices in guest physical memory.
> 
> The three introduced types represent what is being done on x86 and Power
> right now.

commit message doesn't deliver purpose of the path,
So I'm no conviced it's necessary.
we probably discussed it in previous revisions but could you reiterate
it here WHY do you need this and 3/4

 

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/machine.c    |  3 +++
>  hw/i386/pc.c         | 13 +++++++------
>  hw/i386/pc_piix.c    |  2 +-
>  include/hw/boards.h  | 13 +++++++++++++
>  include/hw/i386/pc.h |  3 ---
>  5 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 617e5f8d75..d34b205125 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -531,6 +531,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      mc->numa_mem_align_shift = 23;
>      mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
>  
> +    /* Default: use memory region alignment as memory devices alignment */
> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION;
> +
>      object_class_property_add_str(oc, "accel",
>          machine_get_accel, machine_set_accel, &error_abort);
>      object_class_property_set_description(oc, "accel",
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bf986baf91..04a97e89e7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1331,6 +1331,7 @@ void pc_memory_init(PCMachineState *pcms,
>      FWCfgState *fw_cfg;
>      MachineState *machine = MACHINE(pcms);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
>  
>      assert(machine->ram_size == pcms->below_4g_mem_size +
>                                  pcms->above_4g_mem_size);
> @@ -1363,8 +1364,6 @@ void pc_memory_init(PCMachineState *pcms,
>      if (!pcmc->has_reserved_memory &&
>          (machine->ram_slots ||
>           (machine->maxram_size > machine->ram_size))) {
> -        MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
>          error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
>                       mc->name);
>          exit(EXIT_FAILURE);
> @@ -1394,7 +1393,7 @@ void pc_memory_init(PCMachineState *pcms,
>          machine->device_memory->base =
>              ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
>  
> -        if (pcmc->enforce_aligned_dimm) {
> +        if (mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>              /* size device region assuming 1G page max alignment per slot */
>              device_mem_size += (1ULL << 30) * machine->ram_slots;
>          }
> @@ -1705,14 +1704,16 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> +
> +    if (memory_region_get_alignment(mr) &&
> +        mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>          align = memory_region_get_alignment(mr);
>      }
>  
> @@ -2374,7 +2375,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->gigabyte_align = true;
>      pcmc->has_reserved_memory = true;
>      pcmc->kvmclock_enabled = true;
> -    pcmc->enforce_aligned_dimm = true;
>      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
>       * to be used at the moment, 32K should be enough for a while.  */
>      pcmc->acpi_data_size = 0x20000 + 0x8000;
> @@ -2398,6 +2398,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      nc->nmi_monitor_handler = x86_nmi;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION_OR_PAGE;
>  
>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>          pc_machine_get_device_memory_region_size, NULL,
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3b87f3cedb..cc11856c24 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -566,7 +566,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
>      m->default_display = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
>      pcmc->smbios_uuid_encoded = false;
> -    pcmc->enforce_aligned_dimm = false;
> +    m->memory_device_align = MEMORY_DEVICE_ALIGN_PAGE;
>  }
>  
>  DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index ef7457f5dd..3f151207c1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -105,6 +105,15 @@ typedef struct {
>      CPUArchId cpus[0];
>  } CPUArchIdList;
>  
> +typedef enum MemoryDeviceAlign {
> +    /* use specified memory region alignment */
> +    MEMORY_DEVICE_ALIGN_REGION = 0,
> +    /* use target page size as alignment */
> +    MEMORY_DEVICE_ALIGN_PAGE,
> +    /* use target page size if no memory region alignment has been specified */
> +    MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
> +} MemoryDeviceAlign;
> +
>  /**
>   * MachineClass:
>   * @max_cpus: maximum number of CPUs supported. Default: 1
> @@ -156,6 +165,9 @@ typedef struct {
>   *    should instead use "unimplemented-device" for all memory ranges where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
> + * @memory_device_align: The alignment that will be used as default when
> + *    searching for a guest physical memory address while plugging a
> + *    memory device. This is relevant for compatibility handling.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -202,6 +214,7 @@ struct MachineClass {
>      const char **valid_cpu_types;
>      strList *allowed_dynamic_sysbus_devices;
>      bool auto_enable_numa_with_memhp;
> +    MemoryDeviceAlign memory_device_align;
>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fc8dedca12..ffb4654fc8 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -86,8 +86,6 @@ struct PCMachineState {
>   *
>   * Compat fields:
>   *
> - * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
> - *                        backend's alignment value if provided
>   * @acpi_data_size: Size of the chunk of memory at the top of RAM
>   *                  for the BIOS ACPI tables and other BIOS
>   *                  datastructures.
> @@ -124,7 +122,6 @@ struct PCMachineClass {
>      /* RAM / address space compat: */
>      bool gigabyte_align;
>      bool has_reserved_memory;
> -    bool enforce_aligned_dimm;
>      bool broken_reserved_end;
>  
>      /* TSC rate migration: */

Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[David Hildenbrand]

On 19.06.2018 17:59, Igor Mammedov wrote:
> On Mon, 18 Jun 2018 16:47:58 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We want to handle memory device address assignment without passing
>> compatibility parameters ("*align").
>>
>> As x86 and Power use different strategies to determine an alignment and
>> we need clean support for compat handling, let's introduce an enum on
>> the machine class level. This is the machine configuration on how to
>> align memory devices in guest physical memory.
>>
>> The three introduced types represent what is being done on x86 and Power
>> right now.
> 
> commit message doesn't deliver purpose of the path,

"We want to handle memory device address assignment without passing
compatibility parameters ("*align")."

So in order to do patch nr 4 without this, I would basically have to
move the align parameter to pc_dimm_pre_plug, along with the code for
"detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
this because ...

> So I'm no conviced it's necessary.
> we probably discussed it in previous revisions but could you reiterate
> it here WHY do you need this and 3/4
> 

.. I want to get rid of the align parameter in the long run. Alignment
is some memory device specific property that can be easily detected
using a detection configuration (this patch). This approach looks much
cleaner to me. This way we can use the same alignment strategy for all
memory devices.

In follow up series I want to factor out address assignment completely
into memory_device_pre_plug(). And I also don't want to have an align
parameter at that function. I want to avoid moving the same code around
two times (pc.c).

>  
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/core/machine.c    |  3 +++
>>  hw/i386/pc.c         | 13 +++++++------
>>  hw/i386/pc_piix.c    |  2 +-
>>  include/hw/boards.h  | 13 +++++++++++++
>>  include/hw/i386/pc.h |  3 ---
>>  5 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 617e5f8d75..d34b205125 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -531,6 +531,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      mc->numa_mem_align_shift = 23;
>>      mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
>>  
>> +    /* Default: use memory region alignment as memory devices alignment */
>> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION;
>> +
>>      object_class_property_add_str(oc, "accel",
>>          machine_get_accel, machine_set_accel, &error_abort);
>>      object_class_property_set_description(oc, "accel",
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index bf986baf91..04a97e89e7 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1331,6 +1331,7 @@ void pc_memory_init(PCMachineState *pcms,
>>      FWCfgState *fw_cfg;
>>      MachineState *machine = MACHINE(pcms);
>>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
>>  
>>      assert(machine->ram_size == pcms->below_4g_mem_size +
>>                                  pcms->above_4g_mem_size);
>> @@ -1363,8 +1364,6 @@ void pc_memory_init(PCMachineState *pcms,
>>      if (!pcmc->has_reserved_memory &&
>>          (machine->ram_slots ||
>>           (machine->maxram_size > machine->ram_size))) {
>> -        MachineClass *mc = MACHINE_GET_CLASS(machine);
>> -
>>          error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
>>                       mc->name);
>>          exit(EXIT_FAILURE);
>> @@ -1394,7 +1393,7 @@ void pc_memory_init(PCMachineState *pcms,
>>          machine->device_memory->base =
>>              ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
>>  
>> -        if (pcmc->enforce_aligned_dimm) {
>> +        if (mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>>              /* size device region assuming 1G page max alignment per slot */
>>              device_mem_size += (1ULL << 30) * machine->ram_slots;
>>          }
>> @@ -1705,14 +1704,16 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>>      HotplugHandlerClass *hhc;
>>      Error *local_err = NULL;
>>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>>      PCDIMMDevice *dimm = PC_DIMM(dev);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>>      uint64_t align = TARGET_PAGE_SIZE;
>>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>  
>> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>> +
>> +    if (memory_region_get_alignment(mr) &&
>> +        mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
>>          align = memory_region_get_alignment(mr);
>>      }
>>  
>> @@ -2374,7 +2375,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      pcmc->gigabyte_align = true;
>>      pcmc->has_reserved_memory = true;
>>      pcmc->kvmclock_enabled = true;
>> -    pcmc->enforce_aligned_dimm = true;
>>      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
>>       * to be used at the moment, 32K should be enough for a while.  */
>>      pcmc->acpi_data_size = 0x20000 + 0x8000;
>> @@ -2398,6 +2398,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>      hc->unplug = pc_machine_device_unplug_cb;
>>      nc->nmi_monitor_handler = x86_nmi;
>>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION_OR_PAGE;
>>  
>>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
>>          pc_machine_get_device_memory_region_size, NULL,
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 3b87f3cedb..cc11856c24 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -566,7 +566,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
>>      m->default_display = NULL;
>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
>>      pcmc->smbios_uuid_encoded = false;
>> -    pcmc->enforce_aligned_dimm = false;
>> +    m->memory_device_align = MEMORY_DEVICE_ALIGN_PAGE;
>>  }
>>  
>>  DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1,
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index ef7457f5dd..3f151207c1 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -105,6 +105,15 @@ typedef struct {
>>      CPUArchId cpus[0];
>>  } CPUArchIdList;
>>  
>> +typedef enum MemoryDeviceAlign {
>> +    /* use specified memory region alignment */
>> +    MEMORY_DEVICE_ALIGN_REGION = 0,
>> +    /* use target page size as alignment */
>> +    MEMORY_DEVICE_ALIGN_PAGE,
>> +    /* use target page size if no memory region alignment has been specified */
>> +    MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
>> +} MemoryDeviceAlign;
>> +
>>  /**
>>   * MachineClass:
>>   * @max_cpus: maximum number of CPUs supported. Default: 1
>> @@ -156,6 +165,9 @@ typedef struct {
>>   *    should instead use "unimplemented-device" for all memory ranges where
>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>   *    implement and a stub device is required.
>> + * @memory_device_align: The alignment that will be used as default when
>> + *    searching for a guest physical memory address while plugging a
>> + *    memory device. This is relevant for compatibility handling.
>>   */
>>  struct MachineClass {
>>      /*< private >*/
>> @@ -202,6 +214,7 @@ struct MachineClass {
>>      const char **valid_cpu_types;
>>      strList *allowed_dynamic_sysbus_devices;
>>      bool auto_enable_numa_with_memhp;
>> +    MemoryDeviceAlign memory_device_align;
>>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>>                                   int nb_nodes, ram_addr_t size);
>>  
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index fc8dedca12..ffb4654fc8 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -86,8 +86,6 @@ struct PCMachineState {
>>   *
>>   * Compat fields:
>>   *
>> - * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
>> - *                        backend's alignment value if provided
>>   * @acpi_data_size: Size of the chunk of memory at the top of RAM
>>   *                  for the BIOS ACPI tables and other BIOS
>>   *                  datastructures.
>> @@ -124,7 +122,6 @@ struct PCMachineClass {
>>      /* RAM / address space compat: */
>>      bool gigabyte_align;
>>      bool has_reserved_memory;
>> -    bool enforce_aligned_dimm;
>>      bool broken_reserved_end;
>>  
>>      /* TSC rate migration: */
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[David Hildenbrand]

On 19.06.2018 19:06, David Hildenbrand wrote:
> On 19.06.2018 17:59, Igor Mammedov wrote:
>> On Mon, 18 Jun 2018 16:47:58 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> We want to handle memory device address assignment without passing
>>> compatibility parameters ("*align").
>>>
>>> As x86 and Power use different strategies to determine an alignment and
>>> we need clean support for compat handling, let's introduce an enum on
>>> the machine class level. This is the machine configuration on how to
>>> align memory devices in guest physical memory.
>>>
>>> The three introduced types represent what is being done on x86 and Power
>>> right now.
>>
>> commit message doesn't deliver purpose of the path,
> 
> "We want to handle memory device address assignment without passing
> compatibility parameters ("*align")."
> 
> So in order to do patch nr 4 without this, I would basically have to
> move the align parameter to pc_dimm_pre_plug, along with the code for
> "detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
> this because ...
> 
>> So I'm no conviced it's necessary.
>> we probably discussed it in previous revisions but could you reiterate
>> it here WHY do you need this and 3/4
>>
> 
> .. I want to get rid of the align parameter in the long run. Alignment
> is some memory device specific property that can be easily detected
> using a detection configuration (this patch). This approach looks much
> cleaner to me. This way we can use the same alignment strategy for all
> memory devices.
> 
> In follow up series I want to factor out address assignment completely
> into memory_device_pre_plug(). And I also don't want to have an align
> parameter at that function. I want to avoid moving the same code around
> two times (pc.c).

It is probably best if you tell my your opinion on how address
assignment/alignment handling of pc-dimm/memory-devices is to be handled
after my rework.

My idea for the end result:

pc_dimm_pre_plug(machine, dev, errp) {
	// detect and verify slot ...
	memory_device_pre_plug(machine, dev, errp);
}

virtio_mem_pre_plug(machine, dev, errp) {
	memory_device_pre_plug(machine, dev, errp);
}

memory_device_pre_plug(machine, md, errp) {
	align = memory_device_get_align(machine, md); //handle compat
	addr = md->get_addr();
        addr = memory_device_get_free_addr(... addr, align ...)
	md->set_addr(addr);
}

If you want *align to remain part of the function call, then
pc_dimm_pre_plug and virtio_mem_pre_plug will have to detect the
alignment themselves (e.g. using the memory region) and either

a) pass it into memory_device_pre_plug()
b) handle what memory_device_pre_plug() would do (get_free_addr ... what
we have in pc_dimm_plug() right now)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[Igor Mammedov]

On Tue, 19 Jun 2018 19:06:38 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 19.06.2018 17:59, Igor Mammedov wrote:
> > On Mon, 18 Jun 2018 16:47:58 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> We want to handle memory device address assignment without passing
> >> compatibility parameters ("*align").
> >>
> >> As x86 and Power use different strategies to determine an alignment and
> >> we need clean support for compat handling, let's introduce an enum on
> >> the machine class level. This is the machine configuration on how to
> >> align memory devices in guest physical memory.
> >>
> >> The three introduced types represent what is being done on x86 and Power
> >> right now.  
> > 
> > commit message doesn't deliver purpose of the path,  
> 
> "We want to handle memory device address assignment without passing
> compatibility parameters ("*align")."
> 
> So in order to do patch nr 4 without this, I would basically have to
> move the align parameter to pc_dimm_pre_plug, along with the code for
> "detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
> this because ...
> 
> > So I'm no conviced it's necessary.
> > we probably discussed it in previous revisions but could you reiterate
> > it here WHY do you need this and 3/4
> >   
> 
> .. I want to get rid of the align parameter in the long run. Alignment
> is some memory device specific property that can be easily detected
> using a detection configuration (this patch). This approach looks much
> cleaner to me. This way we can use the same alignment strategy for all
> memory devices.
> 
> In follow up series I want to factor out address assignment completely
> into memory_device_pre_plug(). And I also don't want to have an align
> parameter at that function. I want to avoid moving the same code around
> two times (pc.c).

Lets look at what we have currently:

  1.1 PC: RAM backend target page size alignment (bug fixed by 92a37a04d
      as non aligned addr is not valid at all)
            align = TARGET_PAGE_SIZE

      but immediately following up commits a2b257d62 / 0c0de1b68
      overwrites it unconditionally to QEMU_VMALLOC_ALIGN for 2.2 and later

      so
      ..v2.1
            address/size = not multiple of TARGET_PAGE_SIZE in QEMU-2.1 is broken
                           (a2b257d62) and we don't care

            align = TARGET_PAGE_SIZE since QEMU-2.2 binary even for older machine types
      and later
            align = QEMU_VMALLOC_ALIGN

  1.2 SPAPR: RAM backend. memhotplug came after 1.1 so it has
         v2.5
            align = QEMU_VMALLOC_ALIGN

  2.1 PC: file backend
          v2.1
            align = TARGET_PAGE_SIZE
          v2.2 .. 2.11
            align = qemu_fd_getpagesize(fd)
          v2.12 adds one more invariant
            align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
            
  2.2 SPAPR: file backend
          v2.5..2.11
            align = qemu_fd_getpagesize(fd)
          v2.12
            align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)

also there is s390 kvm invariant for file backend see: file_ram_alloc()

           block->mr->align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
           if (vkm)
              block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);


to sum up they all have memory region based alignment except of v2.1 PC machine
which is compat trick that I don't expect to be used anywhere else.
So taking in account above and fact that it's backend property,
I'm against of pushing it up to generic machine level, I'd try to keep
compat hack local to PC machine along with enforce_aligned_dimm.

Moreover 3/4 patch where you are making memory-device.c build per target is no go,
we are trying to minimize number of such files and not to add any without
a good reason.

Pushing align detection into common helper would be sufficient, 
following could do the job with explicit comment inside that *align
is compat hack for pc machine.

   memory_device_pre_plug(.... int *align)
     use non null for pc-2.1 compat hack and NULL in all other cases


> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/core/machine.c    |  3 +++
> >>  hw/i386/pc.c         | 13 +++++++------
> >>  hw/i386/pc_piix.c    |  2 +-
> >>  include/hw/boards.h  | 13 +++++++++++++
> >>  include/hw/i386/pc.h |  3 ---
> >>  5 files changed, 24 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 617e5f8d75..d34b205125 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -531,6 +531,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
> >>      mc->numa_mem_align_shift = 23;
> >>      mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
> >>  
> >> +    /* Default: use memory region alignment as memory devices alignment */
> >> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION;
> >> +
> >>      object_class_property_add_str(oc, "accel",
> >>          machine_get_accel, machine_set_accel, &error_abort);
> >>      object_class_property_set_description(oc, "accel",
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index bf986baf91..04a97e89e7 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1331,6 +1331,7 @@ void pc_memory_init(PCMachineState *pcms,
> >>      FWCfgState *fw_cfg;
> >>      MachineState *machine = MACHINE(pcms);
> >>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> >>  
> >>      assert(machine->ram_size == pcms->below_4g_mem_size +
> >>                                  pcms->above_4g_mem_size);
> >> @@ -1363,8 +1364,6 @@ void pc_memory_init(PCMachineState *pcms,
> >>      if (!pcmc->has_reserved_memory &&
> >>          (machine->ram_slots ||
> >>           (machine->maxram_size > machine->ram_size))) {
> >> -        MachineClass *mc = MACHINE_GET_CLASS(machine);
> >> -
> >>          error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
> >>                       mc->name);
> >>          exit(EXIT_FAILURE);
> >> @@ -1394,7 +1393,7 @@ void pc_memory_init(PCMachineState *pcms,
> >>          machine->device_memory->base =
> >>              ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
> >>  
> >> -        if (pcmc->enforce_aligned_dimm) {
> >> +        if (mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
> >>              /* size device region assuming 1G page max alignment per slot */
> >>              device_mem_size += (1ULL << 30) * machine->ram_slots;
> >>          }
> >> @@ -1705,14 +1704,16 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
> >>      HotplugHandlerClass *hhc;
> >>      Error *local_err = NULL;
> >>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >> +    MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> >>      PCDIMMDevice *dimm = PC_DIMM(dev);
> >>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> >>      uint64_t align = TARGET_PAGE_SIZE;
> >>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >>  
> >> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> >> +
> >> +    if (memory_region_get_alignment(mr) &&
> >> +        mc->memory_device_align != MEMORY_DEVICE_ALIGN_PAGE) {
> >>          align = memory_region_get_alignment(mr);
> >>      }
> >>  
> >> @@ -2374,7 +2375,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >>      pcmc->gigabyte_align = true;
> >>      pcmc->has_reserved_memory = true;
> >>      pcmc->kvmclock_enabled = true;
> >> -    pcmc->enforce_aligned_dimm = true;
> >>      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
> >>       * to be used at the moment, 32K should be enough for a while.  */
> >>      pcmc->acpi_data_size = 0x20000 + 0x8000;
> >> @@ -2398,6 +2398,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >>      hc->unplug = pc_machine_device_unplug_cb;
> >>      nc->nmi_monitor_handler = x86_nmi;
> >>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
> >> +    mc->memory_device_align = MEMORY_DEVICE_ALIGN_REGION_OR_PAGE;
> >>  
> >>      object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> >>          pc_machine_get_device_memory_region_size, NULL,
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 3b87f3cedb..cc11856c24 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -566,7 +566,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
> >>      m->default_display = NULL;
> >>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
> >>      pcmc->smbios_uuid_encoded = false;
> >> -    pcmc->enforce_aligned_dimm = false;
> >> +    m->memory_device_align = MEMORY_DEVICE_ALIGN_PAGE;
> >>  }
> >>  
> >>  DEFINE_I440FX_MACHINE(v2_1, "pc-i440fx-2.1", pc_compat_2_1,
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index ef7457f5dd..3f151207c1 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -105,6 +105,15 @@ typedef struct {
> >>      CPUArchId cpus[0];
> >>  } CPUArchIdList;
> >>  
> >> +typedef enum MemoryDeviceAlign {
> >> +    /* use specified memory region alignment */
> >> +    MEMORY_DEVICE_ALIGN_REGION = 0,
> >> +    /* use target page size as alignment */
> >> +    MEMORY_DEVICE_ALIGN_PAGE,
> >> +    /* use target page size if no memory region alignment has been specified */
> >> +    MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
> >> +} MemoryDeviceAlign;
> >> +
> >>  /**
> >>   * MachineClass:
> >>   * @max_cpus: maximum number of CPUs supported. Default: 1
> >> @@ -156,6 +165,9 @@ typedef struct {
> >>   *    should instead use "unimplemented-device" for all memory ranges where
> >>   *    the guest will attempt to probe for a device that QEMU doesn't
> >>   *    implement and a stub device is required.
> >> + * @memory_device_align: The alignment that will be used as default when
> >> + *    searching for a guest physical memory address while plugging a
> >> + *    memory device. This is relevant for compatibility handling.
> >>   */
> >>  struct MachineClass {
> >>      /*< private >*/
> >> @@ -202,6 +214,7 @@ struct MachineClass {
> >>      const char **valid_cpu_types;
> >>      strList *allowed_dynamic_sysbus_devices;
> >>      bool auto_enable_numa_with_memhp;
> >> +    MemoryDeviceAlign memory_device_align;
> >>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
> >>                                   int nb_nodes, ram_addr_t size);
> >>  
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index fc8dedca12..ffb4654fc8 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -86,8 +86,6 @@ struct PCMachineState {
> >>   *
> >>   * Compat fields:
> >>   *
> >> - * @enforce_aligned_dimm: check that DIMM's address/size is aligned by
> >> - *                        backend's alignment value if provided
> >>   * @acpi_data_size: Size of the chunk of memory at the top of RAM
> >>   *                  for the BIOS ACPI tables and other BIOS
> >>   *                  datastructures.
> >> @@ -124,7 +122,6 @@ struct PCMachineClass {
> >>      /* RAM / address space compat: */
> >>      bool gigabyte_align;
> >>      bool has_reserved_memory;
> >> -    bool enforce_aligned_dimm;
> >>      bool broken_reserved_end;
> >>  
> >>      /* TSC rate migration: */  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 2/4] machine: factor out enforce_aligned_dimm into memory_device_align

[David Hildenbrand]

On 26.06.2018 17:03, Igor Mammedov wrote:
> On Tue, 19 Jun 2018 19:06:38 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 19.06.2018 17:59, Igor Mammedov wrote:
>>> On Mon, 18 Jun 2018 16:47:58 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> We want to handle memory device address assignment without passing
>>>> compatibility parameters ("*align").
>>>>
>>>> As x86 and Power use different strategies to determine an alignment and
>>>> we need clean support for compat handling, let's introduce an enum on
>>>> the machine class level. This is the machine configuration on how to
>>>> align memory devices in guest physical memory.
>>>>
>>>> The three introduced types represent what is being done on x86 and Power
>>>> right now.  
>>>
>>> commit message doesn't deliver purpose of the path,  
>>
>> "We want to handle memory device address assignment without passing
>> compatibility parameters ("*align")."
>>
>> So in order to do patch nr 4 without this, I would basically have to
>> move the align parameter to pc_dimm_pre_plug, along with the code for
>> "detecting" the alignment in e.g. pc_memory_plug. And I want to avoid
>> this because ...
>>
>>> So I'm no conviced it's necessary.
>>> we probably discussed it in previous revisions but could you reiterate
>>> it here WHY do you need this and 3/4
>>>   
>>
>> .. I want to get rid of the align parameter in the long run. Alignment
>> is some memory device specific property that can be easily detected
>> using a detection configuration (this patch). This approach looks much
>> cleaner to me. This way we can use the same alignment strategy for all
>> memory devices.
>>
>> In follow up series I want to factor out address assignment completely
>> into memory_device_pre_plug(). And I also don't want to have an align
>> parameter at that function. I want to avoid moving the same code around
>> two times (pc.c).
> 
> Lets look at what we have currently:
> 
>   1.1 PC: RAM backend target page size alignment (bug fixed by 92a37a04d
>       as non aligned addr is not valid at all)
>             align = TARGET_PAGE_SIZE
> 
>       but immediately following up commits a2b257d62 / 0c0de1b68
>       overwrites it unconditionally to QEMU_VMALLOC_ALIGN for 2.2 and later
> 
>       so
>       ..v2.1
>             address/size = not multiple of TARGET_PAGE_SIZE in QEMU-2.1 is broken
>                            (a2b257d62) and we don't care
> 
>             align = TARGET_PAGE_SIZE since QEMU-2.2 binary even for older machine types
>       and later
>             align = QEMU_VMALLOC_ALIGN
> 
>   1.2 SPAPR: RAM backend. memhotplug came after 1.1 so it has
>          v2.5
>             align = QEMU_VMALLOC_ALIGN
> 
>   2.1 PC: file backend
>           v2.1
>             align = TARGET_PAGE_SIZE
>           v2.2 .. 2.11
>             align = qemu_fd_getpagesize(fd)
>           v2.12 adds one more invariant
>             align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
>             
>   2.2 SPAPR: file backend
>           v2.5..2.11
>             align = qemu_fd_getpagesize(fd)
>           v2.12
>             align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
> 
> also there is s390 kvm invariant for file backend see: file_ram_alloc()
> 
>            block->mr->align = MAX(qemu_fd_getpagesize(fd), 'filebackend.align' option)
>            if (vkm)
>               block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
> 
> 
> to sum up they all have memory region based alignment except of v2.1 PC machine
> which is compat trick that I don't expect to be used anywhere else.
> So taking in account above and fact that it's backend property,
> I'm against of pushing it up to generic machine level, I'd try to keep
> compat hack local to PC machine along with enforce_aligned_dimm.

Problematic case is win32 qemu_anon_ram_alloc(), which does not fixup
the alignment. As far as I can see, the alignment will stay 0. So it
could happen that we have a 0 alignment, but I'll send a fix for that (I
don't think we have to worry about compat in windows here (changing
alignment from 0 to getpagesize())).

> 
> Moreover 3/4 patch where you are making memory-device.c build per target is no go,
> we are trying to minimize number of such files and not to add any without
> a good reason.

I agree, this is to be avoided.

> 
> Pushing align detection into common helper would be sufficient, 
> following could do the job with explicit comment inside that *align
> is compat hack for pc machine.
> 
>    memory_device_pre_plug(.... int *align)
>      use non null for pc-2.1 compat hack and NULL in all other cases
> 

Okay, in the first shot I'll do

pcdimm_pre_plug(... uint64_t *enforced_align ...)


-- 

Thanks,

David / dhildenb