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

[Qemu-devel] [PATCH v3 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

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

v2 -> v3:
- "util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()"
-- take allocation granularity into account
-- drop comment
- "pc-dimm: assign and verify the "addr" property during pre_plug"
-- compat_align -> legacy_align
-- shortened align detection as proposed by Igor
-- trace address asignment in pre_plug

v1 -> v2:
- dropped "machine: factor out enforce_aligned_dimm into memory_device_align"
- dropped "pc-dimm/memory-device: detect alignment internally"
- added "util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()"
-- indicate the alignment and therefor simplify compat handling for
   Windows as we can ...
- added "pc: drop memory region alignment check for 0"
-- ... drop a check for memory_region_get_alignment(mr), allowing us to
   factor out compat handling for pc DIMM/NVDIMM alignment in a clean way.
- "pc-dimm: assign and verify the "addr" property during pre_plug"
-- Default to "memory_region_get_alignment(mr)" if no compat alignment
   has been specified.

David Hildenbrand (4):
  pc-dimm: assign and verify the "slot" property during pre_plug
  util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()
  pc: drop memory region alignment check for 0
  pc-dimm: assign and verify the "addr" property during pre_plug

 hw/i386/pc.c             | 16 ++++-------
 hw/mem/pc-dimm.c         | 61 +++++++++++++++++++++-------------------
 hw/ppc/spapr.c           | 14 ++++++---
 include/hw/mem/pc-dimm.h |  5 ++--
 util/oslib-win32.c       | 15 ++++++++--
 5 files changed, 63 insertions(+), 48 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 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().

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             |  2 ++
 hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
 hw/ppc/spapr.c           |  9 ++++++++-
 include/hw/mem/pc-dimm.h |  1 +
 4 files changed, 29 insertions(+), 18 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 b32b971a14..bf012235b6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3191,6 +3191,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    Error *local_err = NULL;
     MemoryRegion *mr;
     uint64_t size;
     Object *memdev;
@@ -3216,7 +3217,13 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
                                       &error_abort);
     pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
-    spapr_check_pagesize(spapr, pagesize, errp);
+    spapr_check_pagesize(spapr, pagesize, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
 }
 
 struct sPAPRDIMMState {
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

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

[Auger Eric]

Hi David,

On 07/02/2018 11:37 AM, 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().
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c             |  2 ++
>  hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
>  hw/ppc/spapr.c           |  9 ++++++++-
>  include/hw/mem/pc-dimm.h |  1 +
>  4 files changed, 29 insertions(+), 18 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 b32b971a14..bf012235b6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3191,6 +3191,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +    Error *local_err = NULL;
>      MemoryRegion *mr;
>      uint64_t size;
>      Object *memdev;
> @@ -3216,7 +3217,13 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
>                                        &error_abort);
>      pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
> -    spapr_check_pagesize(spapr, pagesize, errp);
> +    spapr_check_pagesize(spapr, pagesize, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  }
>  
>  struct sPAPRDIMMState {
> 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);
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH v3 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[David Hildenbrand]

Let's set the alignment just like for the posix variant. This will
implicitly set the alignment of the underlying memory region and
therefore make memory_region_get_alignment(mr) return something > 0 for
all memory backends applicable to PCDIMM/NVDIMM.

The allocation granularity is ususally 64k, while the page size is 4k.
The documentation of VirtualAlloc is not really comprehensible in case
only MEM_COMMIT is specified without an address. We'll detect the actual
values and then go for the bigger one. The expection is, that it will
always be 64k aligned. (The assumption is that MEM_COMMIT does an
implicit MEM_RESERVE, so the address will always be aligned to the
allocation granularity. And the allocation granularity is always bigger
than the page size).

This will allow us to drop special handling in pc.c for
memory_region_get_alignment(mr) == 0, as we can then assume that it is
always set (and AFAICS >= getpagesize()).

For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
therefore alignment of DIMMs will not change, and therefore also not the
guest physical memory layout.

For spapr in spapr_memory_plug(), an alignment of 0 would have been used
until now. As QEMU_ALIGN_UP will crash with the alignment being 0, this
never worked, so we don't have to care about compatibility handling.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 util/oslib-win32.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index bb5ad28bd3..25dd1595ad 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -67,15 +67,24 @@ void *qemu_memalign(size_t alignment, size_t size)
     return qemu_oom_check(qemu_try_memalign(alignment, size));
 }
 
+static int get_allocation_granularity(void)
+{
+    SYSTEM_INFO system_info;
+
+    GetSystemInfo(&system_info);
+    return system_info.dwAllocationGranularity;
+}
+
 void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
 {
     void *ptr;
 
-    /* FIXME: this is not exactly optimal solution since VirtualAlloc
-       has 64Kb granularity, but at least it guarantees us that the
-       memory is page aligned. */
     ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
     trace_qemu_anon_ram_alloc(size, ptr);
+
+    if (ptr && align) {
+        *align = MAX(get_allocation_granularity(), getpagesize());
+    }
     return ptr;
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[Igor Mammedov]

On Mon,  2 Jul 2018 11:37:53 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's set the alignment just like for the posix variant. This will
> implicitly set the alignment of the underlying memory region and
> therefore make memory_region_get_alignment(mr) return something > 0 for
> all memory backends applicable to PCDIMM/NVDIMM.
> 
> The allocation granularity is ususally 64k, while the page size is 4k.
> The documentation of VirtualAlloc is not really comprehensible in case
> only MEM_COMMIT is specified without an address. We'll detect the actual
> values and then go for the bigger one. The expection is, that it will
> always be 64k aligned. (The assumption is that MEM_COMMIT does an
> implicit MEM_RESERVE, so the address will always be aligned to the
> allocation granularity. And the allocation granularity is always bigger
> than the page size).
> 
> This will allow us to drop special handling in pc.c for
> memory_region_get_alignment(mr) == 0, as we can then assume that it is
> always set (and AFAICS >= getpagesize()).
> 
> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
> therefore alignment of DIMMs will not change, and therefore also not the
> guest physical memory layout.
> 
> For spapr in spapr_memory_plug(), an alignment of 0 would have been used
> until now. As QEMU_ALIGN_UP will crash with the alignment being 0, this
> never worked, so we don't have to care about compatibility handling.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  util/oslib-win32.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index bb5ad28bd3..25dd1595ad 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -67,15 +67,24 @@ void *qemu_memalign(size_t alignment, size_t size)
>      return qemu_oom_check(qemu_try_memalign(alignment, size));
>  }
>  
> +static int get_allocation_granularity(void)
> +{
> +    SYSTEM_INFO system_info;
> +
> +    GetSystemInfo(&system_info);
> +    return system_info.dwAllocationGranularity;
> +}
> +
>  void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
>  {
>      void *ptr;
>  
> -    /* FIXME: this is not exactly optimal solution since VirtualAlloc
> -       has 64Kb granularity, but at least it guarantees us that the
> -       memory is page aligned. */
>      ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
>      trace_qemu_anon_ram_alloc(size, ptr);
> +
> +    if (ptr && align) {
> +        *align = MAX(get_allocation_granularity(), getpagesize());
> +    }
>      return ptr;
>  }
>  
Not really tested, but patch looks correct to me, hence:

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

Re: [Qemu-devel] [PATCH v3 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[Auger Eric]

Hi David,

On 07/02/2018 11:37 AM, David Hildenbrand wrote:
> Let's set the alignment just like for the posix variant. This will
> implicitly set the alignment of the underlying memory region and
> therefore make memory_region_get_alignment(mr) return something > 0 for
> all memory backends applicable to PCDIMM/NVDIMM.
> 
> The allocation granularity is ususally 64k, while the page size is 4k.
> The documentation of VirtualAlloc is not really comprehensible in case
> only MEM_COMMIT is specified without an address. We'll detect the actual
> values and then go for the bigger one. The expection is, that it will
> always be 64k aligned. (The assumption is that MEM_COMMIT does an
> implicit MEM_RESERVE, so the address will always be aligned to the
> allocation granularity. And the allocation granularity is always bigger
> than the page size).
> 
> This will allow us to drop special handling in pc.c for
> memory_region_get_alignment(mr) == 0, as we can then assume that it is
> always set (and AFAICS >= getpagesize()).
> 
> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
> therefore alignment of DIMMs will not change, and therefore also not the
> guest physical memory layout.
> 
> For spapr in spapr_memory_plug(), an alignment of 0 would have been used
> until now. As QEMU_ALIGN_UP will crash with the alignment being 0, this
> never worked, so we don't have to care about compatibility handling.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  util/oslib-win32.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index bb5ad28bd3..25dd1595ad 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -67,15 +67,24 @@ void *qemu_memalign(size_t alignment, size_t size)
>      return qemu_oom_check(qemu_try_memalign(alignment, size));
>  }
>  
> +static int get_allocation_granularity(void)
> +{
> +    SYSTEM_INFO system_info;
> +
> +    GetSystemInfo(&system_info);
> +    return system_info.dwAllocationGranularity;
> +}
> +
>  void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
>  {
>      void *ptr;
>  
> -    /* FIXME: this is not exactly optimal solution since VirtualAlloc
> -       has 64Kb granularity, but at least it guarantees us that the
> -       memory is page aligned. */
>      ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
>      trace_qemu_anon_ram_alloc(size, ptr);
> +
> +    if (ptr && align) {
> +        *align = MAX(get_allocation_granularity(), getpagesize());
> +    }
>      return ptr;
>  }
>  
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH v3 3/4] pc: drop memory region alignment check for 0

[David Hildenbrand]

All applicable memory regions always have an alignment > 0. All memory
backends result in file_ram_alloc() or qemu_anon_ram_alloc() getting
called, setting the alignment to > 0.

So a PCDIMM memory region always has an alignment > 0. NVDIMM copy the
alignment of the original memory memory region into the handcrafted memory
region that will be used at this place.

So the check for 0 can be dropped and we can reduce the special
handling.

Dropping this check makes factoring out of alignment handling easier as
compat handling only has to look at pcmc->enforce_aligned_dimm and not
care about the alignment of the memory region.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bf986baf91..934b7155b1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1712,7 +1712,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
     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 (pcmc->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 3/4] pc: drop memory region alignment check for 0

[Auger Eric]

Hi David,

On 07/02/2018 11:37 AM, David Hildenbrand wrote:
> All applicable memory regions always have an alignment > 0. All memory
> backends result in file_ram_alloc() or qemu_anon_ram_alloc() getting
> called, setting the alignment to > 0.
> 
> So a PCDIMM memory region always has an alignment > 0. NVDIMM copy the
> alignment of the original memory memory region into the handcrafted memory
> region that will be used at this place.
> 
> So the check for 0 can be dropped and we can reduce the special
> handling.
> 
> Dropping this check makes factoring out of alignment handling easier as
> compat handling only has to look at pcmc->enforce_aligned_dimm and not
> care about the alignment of the memory region.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bf986baf91..934b7155b1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1712,7 +1712,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>      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 (pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
>  
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH v3 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 for DIMMs, 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.

As all memory devices should use the alignment of the underlying memory
region for guest physical address asignment, do detection of the
alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
alignment for compatibility handling.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             | 16 ++++---------
 hw/mem/pc-dimm.c         | 50 +++++++++++++++++++++-------------------
 hw/ppc/spapr.c           |  7 +++---
 include/hw/mem/pc-dimm.h |  6 ++---
 4 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 934b7155b1..54f3c954a5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1678,7 +1678,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                Error **errp)
 {
     const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+    const uint64_t legacy_align = TARGET_PAGE_SIZE;
 
     /*
      * When -no-acpi is used with Q35 machine type, no ACPI is built,
@@ -1696,7 +1698,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev),
+                     pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
 }
 
 static void pc_memory_plug(HotplugHandler *hotplug_dev,
@@ -1705,18 +1708,9 @@ 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);
-    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 (pcmc->enforce_aligned_dimm) {
-        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/pc-dimm.c b/hw/mem/pc-dimm.c
index e56c4daef2..fb6bcaedc4 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -29,9 +29,14 @@
 
 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)
+void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
+                      const uint64_t *legacy_align, Error **errp)
 {
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     Error *local_err = NULL;
+    MemoryRegion *mr;
+    uint64_t addr, align;
     int slot;
 
     slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -43,44 +48,41 @@ 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 = legacy_align ? *legacy_align : memory_region_get_alignment(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;
+    }
+    trace_mhp_pc_dimm_assigned_address(addr);
+    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
+                             &error_abort);
 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);
-    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;
-    }
-    trace_mhp_pc_dimm_assigned_address(addr);
+    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
+                                    &error_abort);
 
     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)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bf012235b6..33543c6373 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3150,13 +3150,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;
     }
@@ -3223,7 +3222,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), NULL, errp);
 }
 
 struct sPAPRDIMMState {
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 7b120416d1..b382eb4303 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -79,8 +79,8 @@ 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_pre_plug(DeviceState *dev, MachineState *machine,
+                      const uint64_t *legacy_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

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

[Igor Mammedov]

On Mon,  2 Jul 2018 11:37:55 +0200
David Hildenbrand <david@redhat.com> wrote:

> We can assign and verify the slot before realizing and trying to plug.
s/slot/"addr"/

> reading/writing the address property should never fail for DIMMs, 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.
> 
> As all memory devices should use the alignment of the underlying memory
> region for guest physical address asignment, do detection of the
> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
> alignment for compatibility handling.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
with commit message fixup,

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
For future reference, I don't really like 2 things about patch
 1: mixes both error handling and functional changes (should be separate patches)
 2: that property setter may crash QEMU at preplug stage
    where it should gracefully error out (so put proper error check
    as follow up patch or respin this one maybe taking in account #1.)

> ---
>  hw/i386/pc.c             | 16 ++++---------
>  hw/mem/pc-dimm.c         | 50 +++++++++++++++++++++-------------------
>  hw/ppc/spapr.c           |  7 +++---
>  include/hw/mem/pc-dimm.h |  6 ++---
>  4 files changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 934b7155b1..54f3c954a5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1678,7 +1678,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>  {
>      const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +    const uint64_t legacy_align = TARGET_PAGE_SIZE;
>  
>      /*
>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
> @@ -1696,7 +1698,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev),
> +                     pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }
>  
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> @@ -1705,18 +1708,9 @@ 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);
> -    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 (pcmc->enforce_aligned_dimm) {
> -        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/pc-dimm.c b/hw/mem/pc-dimm.c
> index e56c4daef2..fb6bcaedc4 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -29,9 +29,14 @@
>  
>  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)
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *legacy_align, Error **errp)
>  {
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    uint64_t addr, align;
>      int slot;
>  
>      slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> @@ -43,44 +48,41 @@ 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 = legacy_align ? *legacy_align : memory_region_get_alignment(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;
> +    }
> +    trace_mhp_pc_dimm_assigned_address(addr);
> +    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> +                             &error_abort);
>  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);
> -    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;
> -    }
> -    trace_mhp_pc_dimm_assigned_address(addr);
> +    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> +                                    &error_abort);
>  
>      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)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf012235b6..33543c6373 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3150,13 +3150,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;
>      }
> @@ -3223,7 +3222,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), NULL, errp);
>  }
>  
>  struct sPAPRDIMMState {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 7b120416d1..b382eb4303 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -79,8 +79,8 @@ 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_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *legacy_align, Error **errp);
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
>  #endif

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

[David Hildenbrand]

On 02.07.2018 12:31, Igor Mammedov wrote:
> On Mon,  2 Jul 2018 11:37:55 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We can assign and verify the slot before realizing and trying to plug.
> s/slot/"addr"/
> 
>> reading/writing the address property should never fail for DIMMs, 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.
>>
>> As all memory devices should use the alignment of the underlying memory
>> region for guest physical address asignment, do detection of the
>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>> alignment for compatibility handling.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> with commit message fixup,
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> For future reference, I don't really like 2 things about patch
>  1: mixes both error handling and functional changes (should be separate patches)
>  2: that property setter may crash QEMU at preplug stage
>     where it should gracefully error out (so put proper error check
>     as follow up patch or respin this one maybe taking in account #1.)

Thanks for the review.

1. could have been factored out into a separate patch.

Regarding 2, I won't respin as long there is nothing else to take care
of. As long as we are in the pc-dimm world and we have static
properties, I don't see any reason to add error handling that cannot
happen. It will be different once we factor out more stuff into memory
device code. I assume you have a different opinion about that, but I
consider these nits.

-- 

Thanks,

David / dhildenb

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

[Igor Mammedov]

On Mon, 2 Jul 2018 12:39:43 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 02.07.2018 12:31, Igor Mammedov wrote:
> > On Mon,  2 Jul 2018 11:37:55 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> We can assign and verify the slot before realizing and trying to plug.  
> > s/slot/"addr"/
> >   
> >> reading/writing the address property should never fail for DIMMs, 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.
> >>
> >> As all memory devices should use the alignment of the underlying memory
> >> region for guest physical address asignment, do detection of the
> >> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
> >> alignment for compatibility handling.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>  
> > with commit message fixup,
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > For future reference, I don't really like 2 things about patch
> >  1: mixes both error handling and functional changes (should be separate patches)
> >  2: that property setter may crash QEMU at preplug stage
> >     where it should gracefully error out (so put proper error check
> >     as follow up patch or respin this one maybe taking in account #1.)  
> 
> Thanks for the review.
> 
> 1. could have been factored out into a separate patch.
> 
> Regarding 2, I won't respin as long there is nothing else to take care
> of. As long as we are in the pc-dimm world and we have static
> properties, I don't see any reason to add error handling that cannot
> happen. It will be different once we factor out more stuff into memory
> device code. I assume you have a different opinion about that, but I
> consider these nits.
it's not really nits. it happens to work now but static properties are
just current impl. detail which can easily change without amending
*plug handlers.
It's cleaner/safer to handle errors properly and keeping device model
separate from machine helpers as much as possible from maintainer pov,
even at expense of extra boiler plate.

I expect you to re-introduce proper error checking when you
factor out memory_device_pre_plug() part /it's condition for my ack
on this patch/.

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

[David Hildenbrand]

On 02.07.2018 14:44, Igor Mammedov wrote:
> On Mon, 2 Jul 2018 12:39:43 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 02.07.2018 12:31, Igor Mammedov wrote:
>>> On Mon,  2 Jul 2018 11:37:55 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> We can assign and verify the slot before realizing and trying to plug.  
>>> s/slot/"addr"/
>>>   
>>>> reading/writing the address property should never fail for DIMMs, 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.
>>>>
>>>> As all memory devices should use the alignment of the underlying memory
>>>> region for guest physical address asignment, do detection of the
>>>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>>>> alignment for compatibility handling.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>  
>>> with commit message fixup,
>>>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> For future reference, I don't really like 2 things about patch
>>>  1: mixes both error handling and functional changes (should be separate patches)
>>>  2: that property setter may crash QEMU at preplug stage
>>>     where it should gracefully error out (so put proper error check
>>>     as follow up patch or respin this one maybe taking in account #1.)  
>>
>> Thanks for the review.
>>
>> 1. could have been factored out into a separate patch.
>>
>> Regarding 2, I won't respin as long there is nothing else to take care
>> of. As long as we are in the pc-dimm world and we have static
>> properties, I don't see any reason to add error handling that cannot
>> happen. It will be different once we factor out more stuff into memory
>> device code. I assume you have a different opinion about that, but I
>> consider these nits.
> it's not really nits. it happens to work now but static properties are
> just current impl. detail which can easily change without amending
> *plug handlers.
> It's cleaner/safer to handle errors properly and keeping device model
> separate from machine helpers as much as possible from maintainer pov,
> even at expense of extra boiler plate.
> 
> I expect you to re-introduce proper error checking when you
> factor out memory_device_pre_plug() part /it's condition for my ack
> on this patch/.
> 
Yes, already contained.

-- 

Thanks,

David / dhildenb

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

[Auger Eric]

Hi David,
On 07/02/2018 02:47 PM, David Hildenbrand wrote:
> On 02.07.2018 14:44, Igor Mammedov wrote:
>> On Mon, 2 Jul 2018 12:39:43 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 02.07.2018 12:31, Igor Mammedov wrote:
>>>> On Mon,  2 Jul 2018 11:37:55 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>   
>>>>> We can assign and verify the slot before realizing and trying to plug.  
>>>> s/slot/"addr"/
>>>>   
>>>>> reading/writing the address property should never fail for DIMMs, 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.
>>>>>
>>>>> As all memory devices should use the alignment of the underlying memory
>>>>> region for guest physical address asignment, do detection of the
>>>>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>>>>> alignment for compatibility handling.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>  
>>>> with commit message fixup,
>>>>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>> For future reference, I don't really like 2 things about patch
>>>>  1: mixes both error handling and functional changes (should be separate patches)
>>>>  2: that property setter may crash QEMU at preplug stage
>>>>     where it should gracefully error out (so put proper error check
>>>>     as follow up patch or respin this one maybe taking in account #1.)  
>>>
>>> Thanks for the review.
>>>
>>> 1. could have been factored out into a separate patch.
>>>
>>> Regarding 2, I won't respin as long there is nothing else to take care
>>> of. As long as we are in the pc-dimm world and we have static
>>> properties, I don't see any reason to add error handling that cannot
>>> happen. It will be different once we factor out more stuff into memory
>>> device code. I assume you have a different opinion about that, but I
>>> consider these nits.
>> it's not really nits. it happens to work now but static properties are
>> just current impl. detail which can easily change without amending
>> *plug handlers.
>> It's cleaner/safer to handle errors properly and keeping device model
>> separate from machine helpers as much as possible from maintainer pov,
>> even at expense of extra boiler plate.
>>
>> I expect you to re-introduce proper error checking when you
>> factor out memory_device_pre_plug() part /it's condition for my ack
>> on this patch/.
>>
> Yes, already contained.
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

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

[Auger Eric]

Hi David,

On 07/02/2018 02:47 PM, David Hildenbrand wrote:
> On 02.07.2018 14:44, Igor Mammedov wrote:
>> On Mon, 2 Jul 2018 12:39:43 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 02.07.2018 12:31, Igor Mammedov wrote:
>>>> On Mon,  2 Jul 2018 11:37:55 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>   
>>>>> We can assign and verify the slot before realizing and trying to plug.  
>>>> s/slot/"addr"/
>>>>   
>>>>> reading/writing the address property should never fail for DIMMs, 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.
>>>>>
>>>>> As all memory devices should use the alignment of the underlying memory
>>>>> region for guest physical address asignment, do detection of the
>>>>> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
>>>>> alignment for compatibility handling.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>  
>>>> with commit message fixup,
>>>>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>> For future reference, I don't really like 2 things about patch
>>>>  1: mixes both error handling and functional changes (should be separate patches)
>>>>  2: that property setter may crash QEMU at preplug stage
>>>>     where it should gracefully error out (so put proper error check
>>>>     as follow up patch or respin this one maybe taking in account #1.)  
>>>
>>> Thanks for the review.
>>>
>>> 1. could have been factored out into a separate patch.
>>>
>>> Regarding 2, I won't respin as long there is nothing else to take care
>>> of. As long as we are in the pc-dimm world and we have static
>>> properties, I don't see any reason to add error handling that cannot
>>> happen. It will be different once we factor out more stuff into memory
>>> device code. I assume you have a different opinion about that, but I
>>> consider these nits.
>> it's not really nits. it happens to work now but static properties are
>> just current impl. detail which can easily change without amending
>> *plug handlers.
>> It's cleaner/safer to handle errors properly and keeping device model
>> separate from machine helpers as much as possible from maintainer pov,
>> even at expense of extra boiler plate.
>>
>> I expect you to re-introduce proper error checking when you
>> factor out memory_device_pre_plug() part /it's condition for my ack
>> on this patch/.
>>
> Yes, already contained.
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

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

[David Gibson]

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

On Mon, Jul 02, 2018 at 11:37:55AM +0200, David Hildenbrand wrote:
> We can assign and verify the slot before realizing and trying to plug.
> reading/writing the address property should never fail for DIMMs, 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.
> 
> As all memory devices should use the alignment of the underlying memory
> region for guest physical address asignment, do detection of the
> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
> alignment for compatibility handling.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

ppc parts

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

> ---
>  hw/i386/pc.c             | 16 ++++---------
>  hw/mem/pc-dimm.c         | 50 +++++++++++++++++++++-------------------
>  hw/ppc/spapr.c           |  7 +++---
>  include/hw/mem/pc-dimm.h |  6 ++---
>  4 files changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 934b7155b1..54f3c954a5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1678,7 +1678,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>  {
>      const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +    const uint64_t legacy_align = TARGET_PAGE_SIZE;
>  
>      /*
>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
> @@ -1696,7 +1698,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev),
> +                     pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }
>  
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> @@ -1705,18 +1708,9 @@ 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);
> -    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 (pcmc->enforce_aligned_dimm) {
> -        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/pc-dimm.c b/hw/mem/pc-dimm.c
> index e56c4daef2..fb6bcaedc4 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -29,9 +29,14 @@
>  
>  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)
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *legacy_align, Error **errp)
>  {
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    uint64_t addr, align;
>      int slot;
>  
>      slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> @@ -43,44 +48,41 @@ 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 = legacy_align ? *legacy_align : memory_region_get_alignment(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;
> +    }
> +    trace_mhp_pc_dimm_assigned_address(addr);
> +    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> +                             &error_abort);
>  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);
> -    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;
> -    }
> -    trace_mhp_pc_dimm_assigned_address(addr);
> +    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> +                                    &error_abort);
>  
>      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)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf012235b6..33543c6373 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3150,13 +3150,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;
>      }
> @@ -3223,7 +3222,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), NULL, errp);
>  }
>  
>  struct sPAPRDIMMState {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 7b120416d1..b382eb4303 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -79,8 +79,8 @@ 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_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *legacy_align, Error **errp);
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
>  #endif

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