[Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups

[Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups

[David Hildenbrand]

This is another set of cleanups as the result from
    [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on the two series
    [PATCH v1 0/2] memory: fix alignment checks/asserts
    [PATCH v2 0/6] spapr: machine hotplug handler cleanups

These cleanup are the last step before factoring out the
pre_plug, plug and unplug logic of memory devices completely, to make it
more independent from pc-dimm.

But these patches will already try to make an important point: Assigning
physical addresses of memory devices will not be done in pre_plug but
during plug. Other properties, like the "slot" property, however can be
handled during pre_plug and is done in this patch series, because they
don't realy on the device to be realized.

Igor proposed to move all property checks + assigmnets to the pre_plug
stage. Hovewer for determining a physical address of a memory device, we
need to know the exact size and the alignment of the underlying memory
region.

This region might not be available and initialized before the device has
been realized (e.g. for NVDIMM). So my point is: Accessing derived
"properties" ("memory region" set up based on "memdev" property and maybe
others e.g. for NVDIMM) via device class functions should not be done
before the device has been realized. These functions should not be
called during pre_plug.

Enforcing this, already leads to sime nice cleanup numbers in pc-dimm
code.

David Hildenbrand (11):
  pc-dimm: remove leftover "struct pc_dimms_capacity"
  nvdimm: no need to overwrite get_vmstate_memory_region()
  pc: factor out pc-dimm checks into pc_dimm_pre_plug()
  hostmem: drop error variable from host_memory_backend_get_memory()
  spapr: move memory hotplug size check into plug code
  pc-dimm: don't allow to access "size" before the device was realized
  pc-dimm: get_memory_region() can never fail
  pc-dimm: get_memory_region() will never return a NULL pointer
  pc-dimm: remove pc_dimm_get_vmstate_memory_region()
  pc-dimm: introduce and use pc_dimm_memory_pre_plug()
  pc-dimm: assign and verify the "slot" property during pre_plug

 backends/hostmem.c       |  3 +-
 hw/i386/pc.c             | 53 ++++++++++++++------------
 hw/mem/nvdimm.c          | 12 ++----
 hw/mem/pc-dimm.c         | 82 +++++++++++++++-------------------------
 hw/misc/ivshmem.c        |  3 +-
 hw/ppc/spapr.c           | 36 +++++-------------
 include/hw/mem/pc-dimm.h |  6 ++-
 include/sysemu/hostmem.h |  3 +-
 numa.c                   |  3 +-
 9 files changed, 81 insertions(+), 120 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 01/11] pc-dimm: remove leftover "struct pc_dimms_capacity"

[David Hildenbrand]

Not needed anymore, let's drop it.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 12da89d562..62b34a992e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,11 +27,6 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
-typedef struct pc_dimms_capacity {
-     uint64_t size;
-     Error    **errp;
-} pc_dimms_capacity;
-
 void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
                          uint64_t align, Error **errp)
 {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 01/11] pc-dimm: remove leftover "struct pc_dimms_capacity"

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:45PM +0200, David Hildenbrand wrote:
> Not needed anymore, let's drop it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 12da89d562..62b34a992e 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,11 +27,6 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> -typedef struct pc_dimms_capacity {
> -     uint64_t size;
> -     Error    **errp;
> -} pc_dimms_capacity;
> -
>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp)
>  {

-- 
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 01/11] pc-dimm: remove leftover "struct pc_dimms_capacity"

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:45 +0200
David Hildenbrand <david@redhat.com> wrote:

> Not needed anymore, let's drop it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 12da89d562..62b34a992e 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,11 +27,6 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> -typedef struct pc_dimms_capacity {
> -     uint64_t size;
> -     Error    **errp;
> -} pc_dimms_capacity;
> -
>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp)
>  {

[Qemu-devel] [PATCH v1 02/11] nvdimm: no need to overwrite get_vmstate_memory_region()

[David Hildenbrand]

Our parent class (PC_DIMM) provides exactly the same function.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 4087aca25e..f974accbdd 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,11 +166,6 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
     memory_region_set_dirty(mr, backend_offset, size);
 }
 
-static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
-}
-
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
@@ -178,7 +173,6 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
 
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
-    ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 02/11] nvdimm: no need to overwrite get_vmstate_memory_region()

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:46PM +0200, David Hildenbrand wrote:
> Our parent class (PC_DIMM) provides exactly the same function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/nvdimm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 4087aca25e..f974accbdd 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,11 +166,6 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>      memory_region_set_dirty(mr, backend_offset, size);
>  }
>  
> -static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> -{
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> -}
> -
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> @@ -178,7 +173,6 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
>  
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
> -    ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;

-- 
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 02/11] nvdimm: no need to overwrite get_vmstate_memory_region()

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:46 +0200
David Hildenbrand <david@redhat.com> wrote:

> Our parent class (PC_DIMM) provides exactly the same function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/nvdimm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 4087aca25e..f974accbdd 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,11 +166,6 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>      memory_region_set_dirty(mr, backend_offset, size);
>  }
>  
> -static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> -{
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> -}
> -
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> @@ -178,7 +173,6 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
>  
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
> -    ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;

[Qemu-devel] [PATCH v1 03/11] pc: factor out pc-dimm checks into pc_dimm_pre_plug()

[David Hildenbrand]

We can perform these checks before the device is actually realized.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3befe6721..85c040482e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1674,6 +1674,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
 }
 
+static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                             Error **errp)
+{
+    const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+
+    /*
+     * When -no-acpi is used with Q35 machine type, no ACPI is built,
+     * but pcms->acpi_dev is still created. Check !acpi_enabled in
+     * addition to cover this case.
+     */
+    if (!pcms->acpi_dev || !acpi_enabled) {
+        error_setg(errp,
+                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
+        return;
+    }
+
+    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
+        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+        return;
+    }
+}
+
 static void pc_dimm_plug(HotplugHandler *hotplug_dev,
                          DeviceState *dev, Error **errp)
 {
@@ -1696,23 +1719,6 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         align = memory_region_get_alignment(mr);
     }
 
-    /*
-     * When -no-acpi is used with Q35 machine type, no ACPI is built,
-     * but pcms->acpi_dev is still created. Check !acpi_enabled in
-     * addition to cover this case.
-     */
-    if (!pcms->acpi_dev || !acpi_enabled) {
-        error_setg(&local_err,
-                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
-        goto out;
-    }
-
-    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
-        error_setg(&local_err,
-                   "nvdimm is not enabled: missing 'nvdimm' in '-M'");
-        goto out;
-    }
-
     pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
     if (local_err) {
         goto out;
@@ -2006,7 +2012,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        pc_dimm_pre_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, errp);
     }
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 03/11] pc: factor out pc-dimm checks into pc_dimm_pre_plug()

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:47PM +0200, David Hildenbrand wrote:
> We can perform these checks before the device is actually realized.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/i386/pc.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3befe6721..85c040482e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1674,6 +1674,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>      }
>  }
>  
> +static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                             Error **errp)
> +{
> +    const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
> +        error_setg(errp,
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
> +        return;
> +    }
> +
> +    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +}
> +
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>                           DeviceState *dev, Error **errp)
>  {
> @@ -1696,23 +1719,6 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    /*
> -     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> -     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> -     * addition to cover this case.
> -     */
> -    if (!pcms->acpi_dev || !acpi_enabled) {
> -        error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
> -        goto out;
> -    }
> -
> -    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> -        error_setg(&local_err,
> -                   "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        goto out;
> -    }
> -
>      pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
>      if (local_err) {
>          goto out;
> @@ -2006,7 +2012,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        pc_dimm_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_pre_plug(hotplug_dev, dev, errp);
>      }
>  }

-- 
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 03/11] pc: factor out pc-dimm checks into pc_dimm_pre_plug()

[Igor Mammedov]

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

> We can perform these checks before the device is actually realized.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/i386/pc.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3befe6721..85c040482e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1674,6 +1674,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>      }
>  }
>  
> +static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                             Error **errp)
> +{
> +    const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
> +        error_setg(errp,
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
> +        return;
> +    }
> +
> +    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +}
> +
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>                           DeviceState *dev, Error **errp)
>  {
> @@ -1696,23 +1719,6 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    /*
> -     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> -     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> -     * addition to cover this case.
> -     */
> -    if (!pcms->acpi_dev || !acpi_enabled) {
> -        error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
> -        goto out;
> -    }
> -
> -    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
> -        error_setg(&local_err,
> -                   "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        goto out;
> -    }
> -
>      pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
>      if (local_err) {
>          goto out;
> @@ -2006,7 +2012,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        pc_dimm_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_pre_plug(hotplug_dev, dev, errp);
>      }
>  }

[Qemu-devel] [PATCH v1 04/11] hostmem: drop error variable from host_memory_backend_get_memory()

[David Hildenbrand]

Unused, so let's remove it.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 backends/hostmem.c       | 3 +--
 hw/mem/nvdimm.c          | 4 ++--
 hw/mem/pc-dimm.c         | 4 ++--
 hw/misc/ivshmem.c        | 3 +--
 include/sysemu/hostmem.h | 3 +--
 numa.c                   | 3 +--
 6 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 3627e61584..4908946cd3 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -247,8 +247,7 @@ bool host_memory_backend_mr_inited(HostMemoryBackend *backend)
     return memory_region_size(&backend->mr) != 0;
 }
 
-MemoryRegion *
-host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend)
 {
     return host_memory_backend_mr_inited(backend) ? &backend->mr : NULL;
 }
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index f974accbdd..df9716231f 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -105,7 +105,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 
 static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
 {
-    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
     uint64_t align, pmem_size, size = memory_region_size(mr);
 
@@ -161,7 +161,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
 
     memcpy(nvdimm->label_data + offset, buf, size);
 
-    mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    mr = host_memory_backend_get_memory(dimm->hostmem);
     backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
     memory_region_set_dirty(mr, backend_offset, size);
 }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 62b34a992e..86fbcf2d0c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -224,12 +224,12 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
         return NULL;
     }
 
-    return host_memory_backend_get_memory(dimm->hostmem, errp);
+    return host_memory_backend_get_memory(dimm->hostmem);
 }
 
 static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
 {
-    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    return host_memory_backend_get_memory(dimm->hostmem);
 }
 
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 16f03701b7..ee01c5e66b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -909,8 +909,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
     if (s->hostmem != NULL) {
         IVSHMEM_DPRINTF("using hostmem\n");
 
-        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem,
-                                                         &error_abort);
+        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
     } else {
         Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
         assert(chr);
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 5beb0ef8ab..6e6bd2c1cb 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -62,8 +62,7 @@ struct HostMemoryBackend {
 };
 
 bool host_memory_backend_mr_inited(HostMemoryBackend *backend);
-MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
-                                             Error **errp);
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend);
 
 void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
 bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
diff --git a/numa.c b/numa.c
index 33572bfa74..94f758c757 100644
--- a/numa.c
+++ b/numa.c
@@ -523,8 +523,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
         if (!backend) {
             continue;
         }
-        MemoryRegion *seg = host_memory_backend_get_memory(backend,
-                                                           &error_fatal);
+        MemoryRegion *seg = host_memory_backend_get_memory(backend);
 
         if (memory_region_is_mapped(seg)) {
             char *path = object_get_canonical_path_component(OBJECT(backend));
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 04/11] hostmem: drop error variable from host_memory_backend_get_memory()

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:48PM +0200, David Hildenbrand wrote:
> Unused, so let's remove it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  backends/hostmem.c       | 3 +--
>  hw/mem/nvdimm.c          | 4 ++--
>  hw/mem/pc-dimm.c         | 4 ++--
>  hw/misc/ivshmem.c        | 3 +--
>  include/sysemu/hostmem.h | 3 +--
>  numa.c                   | 3 +--
>  6 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 3627e61584..4908946cd3 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -247,8 +247,7 @@ bool host_memory_backend_mr_inited(HostMemoryBackend *backend)
>      return memory_region_size(&backend->mr) != 0;
>  }
>  
> -MemoryRegion *
> -host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
> +MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend)
>  {
>      return host_memory_backend_mr_inited(backend) ? &backend->mr : NULL;
>  }
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index f974accbdd..df9716231f 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -105,7 +105,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  
>  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>  {
> -    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
> +    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>      uint64_t align, pmem_size, size = memory_region_size(mr);
>  
> @@ -161,7 +161,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>  
>      memcpy(nvdimm->label_data + offset, buf, size);
>  
> -    mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    mr = host_memory_backend_get_memory(dimm->hostmem);
>      backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
>      memory_region_set_dirty(mr, backend_offset, size);
>  }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 62b34a992e..86fbcf2d0c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -224,12 +224,12 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>          return NULL;
>      }
>  
> -    return host_memory_backend_get_memory(dimm->hostmem, errp);
> +    return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
>  static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>  {
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 16f03701b7..ee01c5e66b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -909,8 +909,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>      if (s->hostmem != NULL) {
>          IVSHMEM_DPRINTF("using hostmem\n");
>  
> -        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem,
> -                                                         &error_abort);
> +        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
>      } else {
>          Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
>          assert(chr);
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 5beb0ef8ab..6e6bd2c1cb 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -62,8 +62,7 @@ struct HostMemoryBackend {
>  };
>  
>  bool host_memory_backend_mr_inited(HostMemoryBackend *backend);
> -MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
> -                                             Error **errp);
> +MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend);
>  
>  void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
>  bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
> diff --git a/numa.c b/numa.c
> index 33572bfa74..94f758c757 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -523,8 +523,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>          if (!backend) {
>              continue;
>          }
> -        MemoryRegion *seg = host_memory_backend_get_memory(backend,
> -                                                           &error_fatal);
> +        MemoryRegion *seg = host_memory_backend_get_memory(backend);
>  
>          if (memory_region_is_mapped(seg)) {
>              char *path = object_get_canonical_path_component(OBJECT(backend));

-- 
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 04/11] hostmem: drop error variable from host_memory_backend_get_memory()

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:48 +0200
David Hildenbrand <david@redhat.com> wrote:

> Unused, so let's remove it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  backends/hostmem.c       | 3 +--
>  hw/mem/nvdimm.c          | 4 ++--
>  hw/mem/pc-dimm.c         | 4 ++--
>  hw/misc/ivshmem.c        | 3 +--
>  include/sysemu/hostmem.h | 3 +--
>  numa.c                   | 3 +--
>  6 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 3627e61584..4908946cd3 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -247,8 +247,7 @@ bool host_memory_backend_mr_inited(HostMemoryBackend *backend)
>      return memory_region_size(&backend->mr) != 0;
>  }
>  
> -MemoryRegion *
> -host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
> +MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend)
>  {
>      return host_memory_backend_mr_inited(backend) ? &backend->mr : NULL;
>  }
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index f974accbdd..df9716231f 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -105,7 +105,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  
>  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>  {
> -    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
> +    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>      uint64_t align, pmem_size, size = memory_region_size(mr);
>  
> @@ -161,7 +161,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>  
>      memcpy(nvdimm->label_data + offset, buf, size);
>  
> -    mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    mr = host_memory_backend_get_memory(dimm->hostmem);
>      backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
>      memory_region_set_dirty(mr, backend_offset, size);
>  }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 62b34a992e..86fbcf2d0c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -224,12 +224,12 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>          return NULL;
>      }
>  
> -    return host_memory_backend_get_memory(dimm->hostmem, errp);
> +    return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
>  static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
>  {
> -    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> +    return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 16f03701b7..ee01c5e66b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -909,8 +909,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>      if (s->hostmem != NULL) {
>          IVSHMEM_DPRINTF("using hostmem\n");
>  
> -        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem,
> -                                                         &error_abort);
> +        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
>      } else {
>          Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
>          assert(chr);
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 5beb0ef8ab..6e6bd2c1cb 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -62,8 +62,7 @@ struct HostMemoryBackend {
>  };
>  
>  bool host_memory_backend_mr_inited(HostMemoryBackend *backend);
> -MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
> -                                             Error **errp);
> +MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend);
>  
>  void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
>  bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
> diff --git a/numa.c b/numa.c
> index 33572bfa74..94f758c757 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -523,8 +523,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>          if (!backend) {
>              continue;
>          }
> -        MemoryRegion *seg = host_memory_backend_get_memory(backend,
> -                                                           &error_fatal);
> +        MemoryRegion *seg = host_memory_backend_get_memory(backend);
>  
>          if (memory_region_is_mapped(seg)) {
>              char *path = object_get_canonical_path_component(OBJECT(backend));

[Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Hildenbrand]

This might look like a step backwards, but it is not. get_memory_region()
should not be called on uninititalized devices. In general, only
properties should be access, but no "derived" satte like the memory
region.

1. We need duplicate error checks if memdev is actually already set.
   realize() performs these checks, no need to duplicate.
2. This is bad practise as one can see when looking at the NVDIMM
   implemetation. The call does not return sane data before the device
   is realized. Although spapr does not use NVDIMM, conceptually it is
   wrong.

So let's just move this call to the right place. We can then cleanup
get_memory_region().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daac..a5f1bbd58a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3153,6 +3153,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
+    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(&local_err, "Hotplugged memory size must be a multiple of "
+                   "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+        goto out;
+    }
+
     pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
     if (local_err) {
         goto out;
@@ -3186,9 +3192,6 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 {
     const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
-    uint64_t size;
     char *mem_dev;
 
     if (!smc->dr_lmb_enabled) {
@@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    mr = ddc->get_memory_region(dimm, errp);
-    if (!mr) {
-        return;
-    }
-    size = memory_region_size(mr);
-
-    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
-        error_setg(errp, "Hotplugged memory size must be a multiple of "
-                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
-        return;
-    }
-
     mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
     if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
         error_setg(errp, "Memory backend has bad page size. "
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:49PM +0200, David Hildenbrand wrote:
> This might look like a step backwards, but it is not. get_memory_region()
> should not be called on uninititalized devices. In general, only
> properties should be access, but no "derived" satte like the memory
> region.
> 
> 1. We need duplicate error checks if memdev is actually already set.
>    realize() performs these checks, no need to duplicate.
> 2. This is bad practise as one can see when looking at the NVDIMM
>    implemetation. The call does not return sane data before the device
>    is realized. Although spapr does not use NVDIMM, conceptually it is
>    wrong.
> 
> So let's just move this call to the right place. We can then cleanup
> get_memory_region().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/ppc/spapr.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..a5f1bbd58a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,6 +3153,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> +    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> +        error_setg(&local_err, "Hotplugged memory size must be a multiple of "
> +                   "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +        goto out;
> +    }
> +
>      pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
>      if (local_err) {
>          goto out;
> @@ -3186,9 +3192,6 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  {
>      const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> -    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> -    uint64_t size;
>      char *mem_dev;
>  
>      if (!smc->dr_lmb_enabled) {
> @@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm, errp);
> -    if (!mr) {
> -        return;
> -    }
> -    size = memory_region_size(mr);
> -
> -    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> -        error_setg(errp, "Hotplugged memory size must be a multiple of "
> -                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> -        return;
> -    }
> -
>      mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
>      if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
>          error_setg(errp, "Memory backend has bad page size. "

-- 
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 05/11] spapr: move memory hotplug size check into plug code

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> This might look like a step backwards, but it is not. get_memory_region()
> should not be called on uninititalized devices. In general, only
> properties should be access, but no "derived" satte like the memory
> region.
>
> 1. We need duplicate error checks if memdev is actually already set.
>    realize() performs these checks, no need to duplicate.
it's not duplicate, if a machine doesn't access to memory region
in preplug time (hence doesn't check), then device itself would check it,
check won't be missed by accident.
(yep it's more code but more robust at the same time, so I'd leave it as is)

> 2. This is bad practise as one can see when looking at the NVDIMM
>    implemetation. The call does not return sane data before the device
>    is realized. Although spapr does not use NVDIMM, conceptually it is
>    wrong.
> 
> So let's just move this call to the right place. We can then cleanup
> get_memory_region().
So I have to say no to this particular patch.
It is indeed a step backwards and it looks like workaround for broken nvdimm impl.

Firstly, memdev property must be set for dimm device and
a user accessing memory region first must check for error.
More details below.

[...]

> @@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm, errp);
> -    if (!mr) {
here 2 bugs are colliding and leading to invalid code path
'if(!mr)' check happens to work for pc-dimm as it returns NULL on error
and error is reported to user.

however in nvdimm case, nvdimm_get_memory_region() unconditionally
returns pointer to not initialized memory alias without any checks

1st issue here is that spapr_memory_pre_plug() should check for error
like spapr_memory_plug() does when calling the same function.

2nd, nvdimm should (re)initialize nvdimm_mr alias whenever hostmem/label_size
properties are set (it's doable but could be tricky. however device model
shouldn't push its issues up to the stack).
There are other places in nvdimm that access uninitialized nvdimm_mr
during properties setting (I suppose all this sites should be fixed
as part of 2nd bugfix).
CCing author & co of nvdimm_mr, so that they could fix issue



> -        return;
> -    }
> -    size = memory_region_size(mr);
> -
> -    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> -        error_setg(errp, "Hotplugged memory size must be a multiple of "
> -                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> -        return;
> -    }
> -
>      mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
>      if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
>          error_setg(errp, "Memory backend has bad page size. "

Re: [Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Hildenbrand]

On 13.06.2018 13:01, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:49 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> This might look like a step backwards, but it is not. get_memory_region()
>> should not be called on uninititalized devices. In general, only
>> properties should be access, but no "derived" satte like the memory
>> region.
>>
>> 1. We need duplicate error checks if memdev is actually already set.
>>    realize() performs these checks, no need to duplicate.
> it's not duplicate, if a machine doesn't access to memory region
> in preplug time (hence doesn't check), then device itself would check it,
> check won't be missed by accident.
> (yep it's more code but more robust at the same time, so I'd leave it as is)

Checking at two places for the same thing == duplicate checks

> 
>> 2. This is bad practise as one can see when looking at the NVDIMM
>>    implemetation. The call does not return sane data before the device
>>    is realized. Although spapr does not use NVDIMM, conceptually it is
>>    wrong.
>>
>> So let's just move this call to the right place. We can then cleanup
>> get_memory_region().
> So I have to say no to this particular patch.
> It is indeed a step backwards and it looks like workaround for broken nvdimm impl.
> 
> Firstly, memdev property must be set for dimm device and
> a user accessing memory region first must check for error.
> More details below.
> 

You assume that any class function can be called at any time. And I
don't think this is the way to go.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[Igor Mammedov]

On Wed, 13 Jun 2018 13:05:53 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.06.2018 13:01, Igor Mammedov wrote:
> > On Mon, 11 Jun 2018 14:16:49 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> This might look like a step backwards, but it is not. get_memory_region()
> >> should not be called on uninititalized devices. In general, only
> >> properties should be access, but no "derived" satte like the memory
> >> region.
> >>
> >> 1. We need duplicate error checks if memdev is actually already set.
> >>    realize() performs these checks, no need to duplicate.  
> > it's not duplicate, if a machine doesn't access to memory region
> > in preplug time (hence doesn't check), then device itself would check it,
> > check won't be missed by accident.
> > (yep it's more code but more robust at the same time, so I'd leave it as is)  
> 
> Checking at two places for the same thing == duplicate checks
device models and there users are separate entities hence I consider
checks are separate. If user code can be written without adding extra checks
it's fine. But if device model doesn't have its own checks when and is
used in by new user code without checks also, it's going to break.

So it would be hard to convince me that consolidating error handling
between in-depended layers is a good idea in general and particularly
in this case.

I'd just drop this error cleanups altogether so that they won't get
in the way of actual changes you are aiming for (unless you have to do it).

> >> 2. This is bad practise as one can see when looking at the NVDIMM
> >>    implemetation. The call does not return sane data before the device
> >>    is realized. Although spapr does not use NVDIMM, conceptually it is
> >>    wrong.
> >>
> >> So let's just move this call to the right place. We can then cleanup
> >> get_memory_region().  
> > So I have to say no to this particular patch.
> > It is indeed a step backwards and it looks like workaround for broken nvdimm impl.
> > 
> > Firstly, memdev property must be set for dimm device and
> > a user accessing memory region first must check for error.
> > More details below.
> >   
> 
> You assume that any class function can be called at any time. And I
> don't think this is the way to go.
Not any time, in the case of get_memory_region() it should work at
pre_plug() time as all the pieces for it are already there.
So we should make it work correctly for NVDIMM instead of 
succumbing to it and running wild.
we should stick to canonical sequence
  object_new -> set props -> realize
I don't see any reason to violate it in this case other than laziness.

Re: [Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Hildenbrand]

On 13.06.2018 15:57, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 13:05:53 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.06.2018 13:01, Igor Mammedov wrote:
>>> On Mon, 11 Jun 2018 14:16:49 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> This might look like a step backwards, but it is not. get_memory_region()
>>>> should not be called on uninititalized devices. In general, only
>>>> properties should be access, but no "derived" satte like the memory
>>>> region.
>>>>
>>>> 1. We need duplicate error checks if memdev is actually already set.
>>>>    realize() performs these checks, no need to duplicate.  
>>> it's not duplicate, if a machine doesn't access to memory region
>>> in preplug time (hence doesn't check), then device itself would check it,
>>> check won't be missed by accident.
>>> (yep it's more code but more robust at the same time, so I'd leave it as is)  
>>
>> Checking at two places for the same thing == duplicate checks
> device models and there users are separate entities hence I consider
> checks are separate. If user code can be written without adding extra checks
> it's fine. But if device model doesn't have its own checks when and is
> used in by new user code without checks also, it's going to break.
> 
> So it would be hard to convince me that consolidating error handling
> between in-depended layers is a good idea in general and particularly
> in this case.
> 
> I'd just drop this error cleanups altogether so that they won't get
> in the way of actual changes you are aiming for (unless you have to do it).
> 
>>>> 2. This is bad practise as one can see when looking at the NVDIMM
>>>>    implemetation. The call does not return sane data before the device
>>>>    is realized. Although spapr does not use NVDIMM, conceptually it is
>>>>    wrong.
>>>>
>>>> So let's just move this call to the right place. We can then cleanup
>>>> get_memory_region().  
>>> So I have to say no to this particular patch.
>>> It is indeed a step backwards and it looks like workaround for broken nvdimm impl.
>>>
>>> Firstly, memdev property must be set for dimm device and
>>> a user accessing memory region first must check for error.
>>> More details below.
>>>   
>>
>> You assume that any class function can be called at any time. And I
>> don't think this is the way to go.
> Not any time, in the case of get_memory_region() it should work at
> pre_plug() time as all the pieces for it are already there.
> So we should make it work correctly for NVDIMM instead of 
> succumbing to it and running wild.
> we should stick to canonical sequence
>   object_new -> set props -> realize
> I don't see any reason to violate it in this case other than laziness.
> 

See my replay to patch nr. 7.

In contrast to what you suggest, I propose converting all nvdimm
properties to static properties (because that's what they actually are).
The validation/initialization of them should happen at a central place,
not at various places (realize(), property setters, or even
get_memory_region()) what you suggest.

I propose a separate function for this (prepare() on DeviceClass), where
we can split of the applicable parts of realize() that just perform
property checks + basic initialization (e.g. of derived properties like
the memory region in case of NVDIMM).

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Hildenbrand]

"size" should not be queried before the device was realized. Let' make
that explicit.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 86fbcf2d0c..5294734529 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
     PCDIMMDevice *dimm = PC_DIMM(obj);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
 
+    if (!DEVICE(obj)->realized) {
+        error_setg(errp, "Property \"%s\" not accessible before realized",
+                   name);
+        return;
+    }
+
     mr = ddc->get_memory_region(dimm, errp);
     if (!mr) {
         return;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:50PM +0200, David Hildenbrand wrote:
> "size" should not be queried before the device was realized. Let' make
> that explicit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 86fbcf2d0c..5294734529 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>      PCDIMMDevice *dimm = PC_DIMM(obj);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>  
> +    if (!DEVICE(obj)->realized) {
> +        error_setg(errp, "Property \"%s\" not accessible before realized",
> +                   name);
> +        return;
> +    }
> +
>      mr = ddc->get_memory_region(dimm, errp);
>      if (!mr) {
>          return;

-- 
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 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:50 +0200
David Hildenbrand <david@redhat.com> wrote:

> "size" should not be queried before the device was realized. Let' make
> that explicit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
It's generic property getter, it should work even before realize is called.

if issues described in 5/11 are properly fixed, this patch won't be needed.

So drop this patch


> ---
>  hw/mem/pc-dimm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 86fbcf2d0c..5294734529 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>      PCDIMMDevice *dimm = PC_DIMM(obj);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>  
> +    if (!DEVICE(obj)->realized) {
> +        error_setg(errp, "Property \"%s\" not accessible before realized",
> +                   name);
> +        return;
> +    }
> +
>      mr = ddc->get_memory_region(dimm, errp);
>      if (!mr) {
>          return;

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Hildenbrand]

On 13.06.2018 14:56, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> "size" should not be queried before the device was realized. Let' make
>> that explicit.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> It's generic property getter, it should work even before realize is called.

That's the point, as we can see in NVDIMM code , it is *not* a generic
property getter.

> 
> if issues described in 5/11 are properly fixed, this patch won't be needed.
> 
> So drop this patch
> 
> 
>> ---
>>  hw/mem/pc-dimm.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 86fbcf2d0c..5294734529 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>>      PCDIMMDevice *dimm = PC_DIMM(obj);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>>  
>> +    if (!DEVICE(obj)->realized) {
>> +        error_setg(errp, "Property \"%s\" not accessible before realized",
>> +                   name);
>> +        return;
>> +    }
>> +
>>      mr = ddc->get_memory_region(dimm, errp);
>>      if (!mr) {
>>          return;
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 04:03:35PM +0200, David Hildenbrand wrote:
> On 13.06.2018 14:56, Igor Mammedov wrote:
> > On Mon, 11 Jun 2018 14:16:50 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> > 
> >> "size" should not be queried before the device was realized. Let' make
> >> that explicit.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> > It's generic property getter, it should work even before realize is called.
> 
> That's the point, as we can see in NVDIMM code , it is *not* a generic
> property getter.

If it were a writeable property, I would agree with Igor because
it would make it impossible to find out what's the default value
for the property.

As it's a read-only property, returning an error seems harmless.
Igor, why exactly do you think it's a problem?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Igor Mammedov]

On Wed, 13 Jun 2018 18:33:37 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 13, 2018 at 04:03:35PM +0200, David Hildenbrand wrote:
> > On 13.06.2018 14:56, Igor Mammedov wrote:  
> > > On Mon, 11 Jun 2018 14:16:50 +0200
> > > David Hildenbrand <david@redhat.com> wrote:
> > >   
> > >> "size" should not be queried before the device was realized. Let' make
> > >> that explicit.
> > >>
> > >> Signed-off-by: David Hildenbrand <david@redhat.com>  
> > > It's generic property getter, it should work even before realize is called.  
> > 
> > That's the point, as we can see in NVDIMM code , it is *not* a generic
> > property getter.  
> 
> If it were a writeable property, I would agree with Igor because
> it would make it impossible to find out what's the default value
> for the property.
> 
> As it's a read-only property, returning an error seems harmless.
> Igor, why exactly do you think it's a problem?
I'm not welcoming putting random restrictions on when property could be read.
For me it's just another qom property and it should return valid value
that corresponds to the current state of the object or error out if
it can't return anything useful at the moment.

In this particular case it should error out if 'memdev' isn't set and
return a value corresponding to current state of [pc|nv]dimm object
if 'memdev' is set.

This patch is obviously wrong, putting restriction based on "realized"
as getter might be easily accessed before Device::realized becomes True,
i.e. during setting properties or within device_set_realized() time.

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Igor Mammedov]

On Wed, 13 Jun 2018 16:03:35 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.06.2018 14:56, Igor Mammedov wrote:
> > On Mon, 11 Jun 2018 14:16:50 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> "size" should not be queried before the device was realized. Let' make
> >> that explicit.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>  
> > It's generic property getter, it should work even before realize is called.  
> 
> That's the point, as we can see in NVDIMM code , it is *not* a generic
> property getter.
it is added by 
    object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size,      
                        NULL, NULL, NULL, &error_abort); 
hence it's generic QOM property as opposed to memory device class specific callbacks.

> 
> > 
> > if issues described in 5/11 are properly fixed, this patch won't be needed.
> > 
> > So drop this patch
> > 
> >   
> >> ---
> >>  hw/mem/pc-dimm.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >> index 86fbcf2d0c..5294734529 100644
> >> --- a/hw/mem/pc-dimm.c
> >> +++ b/hw/mem/pc-dimm.c
> >> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> >>      PCDIMMDevice *dimm = PC_DIMM(obj);
> >>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
> >>  
> >> +    if (!DEVICE(obj)->realized) {
> >> +        error_setg(errp, "Property \"%s\" not accessible before realized",
> >> +                   name);
> >> +        return;
> >> +    }
> >> +
> >>      mr = ddc->get_memory_region(dimm, errp);
just make ddc->get_memory_region return error if 'memdev' isn't set yet
and add local_error handling here as proper property assessor should do.

> >>      if (!mr) {
> >>          return;  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Hildenbrand]

On 14.06.2018 15:24, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 16:03:35 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.06.2018 14:56, Igor Mammedov wrote:
>>> On Mon, 11 Jun 2018 14:16:50 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> "size" should not be queried before the device was realized. Let' make
>>>> that explicit.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>  
>>> It's generic property getter, it should work even before realize is called.  
>>
>> That's the point, as we can see in NVDIMM code , it is *not* a generic
>> property getter.
> it is added by 
>     object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size,      
>                         NULL, NULL, NULL, &error_abort); 
> hence it's generic QOM property as opposed to memory device class specific callbacks.
> 
>>
>>>
>>> if issues described in 5/11 are properly fixed, this patch won't be needed.
>>>
>>> So drop this patch
>>>
>>>   
>>>> ---
>>>>  hw/mem/pc-dimm.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>>>> index 86fbcf2d0c..5294734529 100644
>>>> --- a/hw/mem/pc-dimm.c
>>>> +++ b/hw/mem/pc-dimm.c
>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>>>>      PCDIMMDevice *dimm = PC_DIMM(obj);
>>>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>>>>  
>>>> +    if (!DEVICE(obj)->realized) {
>>>> +        error_setg(errp, "Property \"%s\" not accessible before realized",
>>>> +                   name);
>>>> +        return;
>>>> +    }
>>>> +
>>>>      mr = ddc->get_memory_region(dimm, errp);
> just make ddc->get_memory_region return error if 'memdev' isn't set yet
> and add local_error handling here as proper property assessor should do.
> 

Does not help for NVDIMM.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Igor Mammedov]

On Thu, 14 Jun 2018 16:10:14 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 14.06.2018 15:24, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 16:03:35 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 13.06.2018 14:56, Igor Mammedov wrote:  
> >>> On Mon, 11 Jun 2018 14:16:50 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> "size" should not be queried before the device was realized. Let' make
> >>>> that explicit.
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>    
> >>> It's generic property getter, it should work even before realize is called.    
> >>
> >> That's the point, as we can see in NVDIMM code , it is *not* a generic
> >> property getter.  
> > it is added by 
> >     object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size,      
> >                         NULL, NULL, NULL, &error_abort); 
> > hence it's generic QOM property as opposed to memory device class specific callbacks.
> >   
> >>  
> >>>
> >>> if issues described in 5/11 are properly fixed, this patch won't be needed.
> >>>
> >>> So drop this patch
> >>>
> >>>     
> >>>> ---
> >>>>  hw/mem/pc-dimm.c | 6 ++++++
> >>>>  1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >>>> index 86fbcf2d0c..5294734529 100644
> >>>> --- a/hw/mem/pc-dimm.c
> >>>> +++ b/hw/mem/pc-dimm.c
> >>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> >>>>      PCDIMMDevice *dimm = PC_DIMM(obj);
> >>>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
> >>>>  
> >>>> +    if (!DEVICE(obj)->realized) {
> >>>> +        error_setg(errp, "Property \"%s\" not accessible before realized",
> >>>> +                   name);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>>      mr = ddc->get_memory_region(dimm, errp);  
> > just make ddc->get_memory_region return error if 'memdev' isn't set yet
> > and add local_error handling here as proper property assessor should do.
> >   
> 
> Does not help for NVDIMM.
NVDIMM should be fixed as it's broken now and works just by accident.
I don't like tailoring infrastructure changes to a particular broken
device especially when the later could be made to work within current
framework.

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Hildenbrand]

On 15.06.2018 11:16, Igor Mammedov wrote:
> On Thu, 14 Jun 2018 16:10:14 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 14.06.2018 15:24, Igor Mammedov wrote:
>>> On Wed, 13 Jun 2018 16:03:35 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 13.06.2018 14:56, Igor Mammedov wrote:  
>>>>> On Mon, 11 Jun 2018 14:16:50 +0200
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>     
>>>>>> "size" should not be queried before the device was realized. Let' make
>>>>>> that explicit.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>    
>>>>> It's generic property getter, it should work even before realize is called.    
>>>>
>>>> That's the point, as we can see in NVDIMM code , it is *not* a generic
>>>> property getter.  
>>> it is added by 
>>>     object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size,      
>>>                         NULL, NULL, NULL, &error_abort); 
>>> hence it's generic QOM property as opposed to memory device class specific callbacks.
>>>   
>>>>  
>>>>>
>>>>> if issues described in 5/11 are properly fixed, this patch won't be needed.
>>>>>
>>>>> So drop this patch
>>>>>
>>>>>     
>>>>>> ---
>>>>>>  hw/mem/pc-dimm.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>>>>>> index 86fbcf2d0c..5294734529 100644
>>>>>> --- a/hw/mem/pc-dimm.c
>>>>>> +++ b/hw/mem/pc-dimm.c
>>>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>>>>>>      PCDIMMDevice *dimm = PC_DIMM(obj);
>>>>>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>>>>>>  
>>>>>> +    if (!DEVICE(obj)->realized) {
>>>>>> +        error_setg(errp, "Property \"%s\" not accessible before realized",
>>>>>> +                   name);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>>      mr = ddc->get_memory_region(dimm, errp);  
>>> just make ddc->get_memory_region return error if 'memdev' isn't set yet
>>> and add local_error handling here as proper property assessor should do.
>>>   
>>
>> Does not help for NVDIMM.
> NVDIMM should be fixed as it's broken now and works just by accident.
> I don't like tailoring infrastructure changes to a particular broken
> device especially when the later could be made to work within current
> framework.
> 

Yes, but I will go the path of static properties for NVDIMM instead.
Trying to initialize the memory region in a property setter
("label-size") and then bailing out because another property (memdev) is
not set looks strange. I will do checks+initialization in realize() and
in get_memory_region(). This looks better in my point of view and looks
like a nice NVDIMM refactoring (at least judging from the patches I have
on my list now :) ).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[Igor Mammedov]

On Fri, 15 Jun 2018 11:25:59 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 15.06.2018 11:16, Igor Mammedov wrote:
> > On Thu, 14 Jun 2018 16:10:14 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 14.06.2018 15:24, Igor Mammedov wrote:  
> >>> On Wed, 13 Jun 2018 16:03:35 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> On 13.06.2018 14:56, Igor Mammedov wrote:    
> >>>>> On Mon, 11 Jun 2018 14:16:50 +0200
> >>>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>>       
> >>>>>> "size" should not be queried before the device was realized. Let' make
> >>>>>> that explicit.
> >>>>>>
> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>      
> >>>>> It's generic property getter, it should work even before realize is called.      
> >>>>
> >>>> That's the point, as we can see in NVDIMM code , it is *not* a generic
> >>>> property getter.    
> >>> it is added by 
> >>>     object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size,      
> >>>                         NULL, NULL, NULL, &error_abort); 
> >>> hence it's generic QOM property as opposed to memory device class specific callbacks.
> >>>     
> >>>>    
> >>>>>
> >>>>> if issues described in 5/11 are properly fixed, this patch won't be needed.
> >>>>>
> >>>>> So drop this patch
> >>>>>
> >>>>>       
> >>>>>> ---
> >>>>>>  hw/mem/pc-dimm.c | 6 ++++++
> >>>>>>  1 file changed, 6 insertions(+)
> >>>>>>
> >>>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >>>>>> index 86fbcf2d0c..5294734529 100644
> >>>>>> --- a/hw/mem/pc-dimm.c
> >>>>>> +++ b/hw/mem/pc-dimm.c
> >>>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
> >>>>>>      PCDIMMDevice *dimm = PC_DIMM(obj);
> >>>>>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
> >>>>>>  
> >>>>>> +    if (!DEVICE(obj)->realized) {
> >>>>>> +        error_setg(errp, "Property \"%s\" not accessible before realized",
> >>>>>> +                   name);
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>>      mr = ddc->get_memory_region(dimm, errp);    
> >>> just make ddc->get_memory_region return error if 'memdev' isn't set yet
> >>> and add local_error handling here as proper property assessor should do.
> >>>     
> >>
> >> Does not help for NVDIMM.  
> > NVDIMM should be fixed as it's broken now and works just by accident.
> > I don't like tailoring infrastructure changes to a particular broken
> > device especially when the later could be made to work within current
> > framework.
> >   
> 
> Yes, but I will go the path of static properties for NVDIMM instead.
> Trying to initialize the memory region in a property setter
> ("label-size") and then bailing out because another property (memdev) is
> not set looks strange. I will do checks+initialization in realize() and
> in get_memory_region(). This looks better in my point of view and looks
> like a nice NVDIMM refactoring (at least judging from the patches I have
> on my list now :) ).
sure, I worry less about how it's implemented inside of device model as
far as it behaves nicely to external user.

[Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand]

We already verify when realizing that the memdev property has been
set. We have no more accesses to get_memory_region() before the device
is realized.

So this function will never fail. Remove the stale check and the
error variable. Add a comment to the functions stating that they should
never be called on uninitialized devices.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             |  7 +------
 hw/mem/nvdimm.c          |  2 +-
 hw/mem/pc-dimm.c         | 21 ++++++---------------
 hw/ppc/spapr.c           | 14 +++-----------
 include/hw/mem/pc-dimm.h |  4 +++-
 5 files changed, 14 insertions(+), 34 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 85c040482e..017396fe84 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
     uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index df9716231f..b2dc2bbb50 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj)
                              nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
 }
 
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
 {
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 5294734529..7bb6ce509c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
     Error *local_err = NULL;
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
     uint64_t addr;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     addr = object_property_get_uint(OBJECT(dimm),
                                     PC_DIMM_ADDR_PROP, &local_err);
     if (local_err) {
@@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
 
     memory_device_unplug_region(machine, mr);
     vmstate_unregister_ram(vmstate_mr, dev);
@@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    mr = ddc->get_memory_region(dimm, errp);
+    mr = ddc->get_memory_region(dimm);
     if (!mr) {
         return;
     }
@@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
     host_memory_backend_set_mapped(dimm->hostmem, false);
 }
 
-static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
 {
-    if (!dimm->hostmem) {
-        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
-        return NULL;
-    }
-
+    g_assert(dimm->hostmem);
     return host_memory_backend_get_memory(dimm->hostmem);
 }
 
@@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
     const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
     MemoryRegion *mr;
 
-    mr = ddc->get_memory_region(dimm, &error_abort);
+    mr = ddc->get_memory_region(dimm);
     if (!mr) {
         return 0;
     }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a5f1bbd58a..214286fd2f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
     uint64_t align, size, addr;
     uint32_t node;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
     align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
@@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 {
     sPAPRDRConnector *drc;
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
     uint64_t size = memory_region_size(mr);
     uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
     uint32_t avail_lmbs = 0;
@@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm);
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
     int i;
     sPAPRDRConnector *drc;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
     size = memory_region_size(mr);
     nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
 
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 627c8601d9..f0e6867803 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass {
 
     /* public */
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
-    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
+
+    /* functions should not be called before the device was realized */
+    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:51PM +0200, David Hildenbrand wrote:
> We already verify when realizing that the memdev property has been
> set. We have no more accesses to get_memory_region() before the device
> is realized.
> 
> So this function will never fail. Remove the stale check and the
> error variable. Add a comment to the functions stating that they should
> never be called on uninitialized devices.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

and ppc parts

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

> ---
>  hw/i386/pc.c             |  7 +------
>  hw/mem/nvdimm.c          |  2 +-
>  hw/mem/pc-dimm.c         | 21 ++++++---------------
>  hw/ppc/spapr.c           | 14 +++-----------
>  include/hw/mem/pc-dimm.h |  4 +++-
>  5 files changed, 14 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 85c040482e..017396fe84 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index df9716231f..b2dc2bbb50 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj)
>                               nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
>  }
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 5294734529..7bb6ce509c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t addr;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      addr = object_property_get_uint(OBJECT(dimm),
>                                      PC_DIMM_ADDR_PROP, &local_err);
>      if (local_err) {
> @@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>  
>      memory_device_unplug_region(machine, mr);
>      vmstate_unregister_ram(vmstate_mr, dev);
> @@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm, errp);
> +    mr = ddc->get_memory_region(dimm);
>      if (!mr) {
>          return;
>      }
> @@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
>      host_memory_backend_set_mapped(dimm->hostmem, false);
>  }
>  
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
>  {
> -    if (!dimm->hostmem) {
> -        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> -        return NULL;
> -    }
> -
> +    g_assert(dimm->hostmem);
>      return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
> @@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
>      const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
>      MemoryRegion *mr;
>  
> -    mr = ddc->get_memory_region(dimm, &error_abort);
> +    mr = ddc->get_memory_region(dimm);
>      if (!mr) {
>          return 0;
>      }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a5f1bbd58a..214286fd2f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t align, size, addr;
>      uint32_t node;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> @@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  {
>      sPAPRDRConnector *drc;
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t size = memory_region_size(mr);
>      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>      uint32_t avail_lmbs = 0;
> @@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
>      int i;
>      sPAPRDRConnector *drc;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      size = memory_region_size(mr);
>      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>  
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..f0e6867803 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass {
>  
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> +
> +    /* functions should not be called before the device was realized */
> +    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  

-- 
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 07/11] pc-dimm: get_memory_region() can never fail

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> We already verify when realizing that the memdev property has been
> set. We have no more accesses to get_memory_region() before the device
> is realized.
this stems from assumption that get_memory_region shouldn't be called
before devices realize is executed, which I don't agree to begin with.

However wrt error handling, we should probably leave device internal error
up to devices and make check for error only in pre_plug handler
(since pre_plug was successful, the rest machine helpers would have
access to the same region until device is removed.


> So this function will never fail. Remove the stale check and the
> error variable. Add a comment to the functions stating that they should
> never be called on uninitialized devices.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c             |  7 +------
>  hw/mem/nvdimm.c          |  2 +-
>  hw/mem/pc-dimm.c         | 21 ++++++---------------
>  hw/ppc/spapr.c           | 14 +++-----------
>  include/hw/mem/pc-dimm.h |  4 +++-
>  5 files changed, 14 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 85c040482e..017396fe84 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index df9716231f..b2dc2bbb50 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj)
>                               nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
>  }
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 5294734529..7bb6ce509c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t addr;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      addr = object_property_get_uint(OBJECT(dimm),
>                                      PC_DIMM_ADDR_PROP, &local_err);
>      if (local_err) {
> @@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>  
>      memory_device_unplug_region(machine, mr);
>      vmstate_unregister_ram(vmstate_mr, dev);
> @@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm, errp);
> +    mr = ddc->get_memory_region(dimm);
>      if (!mr) {
>          return;
>      }
> @@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
>      host_memory_backend_set_mapped(dimm->hostmem, false);
>  }
>  
> -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
>  {
> -    if (!dimm->hostmem) {
> -        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> -        return NULL;
> -    }
> -
> +    g_assert(dimm->hostmem);
>      return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
> @@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
>      const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
>      MemoryRegion *mr;
>  
> -    mr = ddc->get_memory_region(dimm, &error_abort);
> +    mr = ddc->get_memory_region(dimm);
>      if (!mr) {
>          return 0;
>      }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a5f1bbd58a..214286fd2f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t align, size, addr;
>      uint32_t node;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> @@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>  {
>      sPAPRDRConnector *drc;
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint64_t size = memory_region_size(mr);
>      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>      uint32_t avail_lmbs = 0;
> @@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
>      int i;
>      sPAPRDRConnector *drc;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      size = memory_region_size(mr);
>      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>  
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..f0e6867803 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass {
>  
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> +
> +    /* functions should not be called before the device was realized */
> +    MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand]

On 13.06.2018 15:03, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We already verify when realizing that the memdev property has been
>> set. We have no more accesses to get_memory_region() before the device
>> is realized.
> this stems from assumption that get_memory_region shouldn't be called
> before devices realize is executed, which I don't agree to begin with.
> 
> However wrt error handling, we should probably leave device internal error
> up to devices and make check for error only in pre_plug handler
> (since pre_plug was successful, the rest machine helpers would have
> access to the same region until device is removed.
> 

Something like a generic Device "validate()"/"pre_realize()" function,
that can be called before realize() and validates all properties
(+initializes derived properties) would be something I could agree to.

Then we could drop all error handling from access functions (as they
have been validated early during pre_plug())

Moving all checks out of realize into pre_plug() looks ugly, because we
have implementation details split across c-files.

> 
>> So this function will never fail. Remove the stale check and the
>> error variable. Add a comment to the functions stating that they should
>> never be called on uninitialized devices.
>>


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand]

On 13.06.2018 16:07, David Hildenbrand wrote:
> On 13.06.2018 15:03, Igor Mammedov wrote:
>> On Mon, 11 Jun 2018 14:16:51 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> We already verify when realizing that the memdev property has been
>>> set. We have no more accesses to get_memory_region() before the device
>>> is realized.
>> this stems from assumption that get_memory_region shouldn't be called
>> before devices realize is executed, which I don't agree to begin with.
>>
>> However wrt error handling, we should probably leave device internal error
>> up to devices and make check for error only in pre_plug handler
>> (since pre_plug was successful, the rest machine helpers would have
>> access to the same region until device is removed.
>>
> 
> Something like a generic Device "validate()"/"pre_realize()" function,
> that can be called before realize() and validates all properties
> (+initializes derived properties) would be something I could agree to.
> 
> Then we could drop all error handling from access functions (as they
> have been validated early during pre_plug())
> 
> Moving all checks out of realize into pre_plug() looks ugly, because we
> have implementation details split across c-files.
> 

I am thinking about something like this

>From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 13 Jun 2018 16:41:12 +0200
Subject: [PATCH RFC] device: add "prepare()" function

The realize() function usually does several things. It validates
properties, inititalizes state based on properties and performs
e.g. registrations in the system that require "unrealize()" to be called
when removing the device.

In a pre_plug hotplug handler, we usually want to access certain
properties or derived properties, e.g. to perform advanced checks
(for resource asignment). Now we have the problem, that realize() was
not called yet once we reach pre_plug(). So we theoretically have to
duplicate checks and add error paths for cases that can
theoretically never happen.

Let's add the "prepare()" callback, which can be used to validate
properties and inititalize some state based on these. It will be called
once all static properties have been inititalized, but before the
pre_plug hotplug handler is activated. The pre_plug handler can than
rely on the fact that basic checks already were performed.

In contrast to "realize()", "prepare()" should not perform any actions
that would have to be rolled back in case e.g. pre_plug fails.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/qdev.c         |  7 +++++++
 include/hw/qdev-core.h | 14 ++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ffec461791..3bfc7e0d54 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -814,6 +814,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             g_free(name);
         }
 
+        if (dc->prepare) {
+            dc->prepare(dev, &local_err);
+            if (local_err != NULL) {
+                goto fail;
+            }
+        }
+
         hotplug_ctrl = qdev_get_hotplug_handler(dev);
         if (hotplug_ctrl) {
             hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f1fd0f8736..63520c1a55 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -29,6 +29,7 @@ typedef enum DeviceCategory {
     DEVICE_CATEGORY_MAX
 } DeviceCategory;
 
+typedef void (*DevicePrepare)(DeviceState *dev, Error **errp);
 typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceReset)(DeviceState *dev);
@@ -40,8 +41,11 @@ struct VMStateDescription;
 /**
  * DeviceClass:
  * @props: Properties accessing state fields.
+ * @prepare: Callback function invoked when the #DeviceState:realized
+ * property is changed to %true, before invoking the pre_plug hotplug handler.
  * @realize: Callback function invoked when the #DeviceState:realized
- * property is changed to %true.
+ * property is changed to %true, after invoking the pre_plug hotplug handler
+ * but before invoking the plug handler.
  * @unrealize: Callback function invoked when the #DeviceState:realized
  * property is changed to %false.
  * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
@@ -54,7 +58,8 @@ struct VMStateDescription;
  * The former may not fail (it might assert or exit), the latter may return
  * error information to the caller and must be re-entrant.
  * Trivial field initializations should go into #TypeInfo.instance_init.
- * Operations depending on @props static properties should go into @realize.
+ * Operations depending on @props static properties should go into @prepare
+ * or @realize.
  * After successful realization, setting static properties will fail.
  *
  * As an interim step, the #DeviceState:realized property can also be
@@ -73,8 +78,8 @@ struct VMStateDescription;
  *
  * <note>
  *   <para>
- * Since TYPE_DEVICE doesn't implement @realize and @unrealize, types
- * derived directly from it need not call their parent's @realize and
+ * Since TYPE_DEVICE doesn't implement @prepare, @realize and @unrealize, types
+ * derived directly from it need not call their parent's @prepare, @realize and
  * @unrealize.
  * For other types consult the documentation and implementation of the
  * respective parent types.
@@ -107,6 +112,7 @@ typedef struct DeviceClass {
 
     /* callbacks */
     DeviceReset reset;
+    DevicePrepare prepare;
     DeviceRealize realize;
     DeviceUnrealize unrealize;
 
-- 
2.17.1


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[Igor Mammedov]

On Wed, 13 Jun 2018 16:50:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.06.2018 16:07, David Hildenbrand wrote:
> > On 13.06.2018 15:03, Igor Mammedov wrote:  
> >> On Mon, 11 Jun 2018 14:16:51 +0200
> >> David Hildenbrand <david@redhat.com> wrote:
> >>  
> >>> We already verify when realizing that the memdev property has been
> >>> set. We have no more accesses to get_memory_region() before the device
> >>> is realized.  
> >> this stems from assumption that get_memory_region shouldn't be called
> >> before devices realize is executed, which I don't agree to begin with.
> >>
> >> However wrt error handling, we should probably leave device internal error
> >> up to devices and make check for error only in pre_plug handler
> >> (since pre_plug was successful, the rest machine helpers would have
> >> access to the same region until device is removed.
> >>  
> > 
> > Something like a generic Device "validate()"/"pre_realize()" function,
> > that can be called before realize() and validates all properties
> > (+initializes derived properties) would be something I could agree to.
> > 
> > Then we could drop all error handling from access functions (as they
> > have been validated early during pre_plug())
> > 
> > Moving all checks out of realize into pre_plug() looks ugly, because we
> > have implementation details split across c-files.
> >   
> 
> I am thinking about something like this
> 
> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 13 Jun 2018 16:41:12 +0200
> Subject: [PATCH RFC] device: add "prepare()" function
> 
> The realize() function usually does several things. It validates
> properties, inititalizes state based on properties and performs
> e.g. registrations in the system that require "unrealize()" to be called
> when removing the device.
> 
> In a pre_plug hotplug handler, we usually want to access certain
> properties or derived properties, e.g. to perform advanced checks
> (for resource asignment). Now we have the problem, that realize() was
> not called yet once we reach pre_plug(). So we theoretically have to
> duplicate checks and add error paths for cases that can
> theoretically never happen.
I care less about duplicate checks in completely different parts of code,
and I'd even insist on device:realize checks being self contained and not
rely on any other external checks performed by users of device. And vice
verse layer that uses device should do it's own checks when necessary
and not rely on device's verification. That way loosely coupled code
wouldn't fall apart whenever we drop or change checks in one of the parts.

The only case where I'd make concession is minimizing error handling
on hot path for performance reasons and this is not the case here.

> Let's add the "prepare()" callback, which can be used to validate
> properties and inititalize some state based on these. It will be called
> once all static properties have been inititalized, but before the
> pre_plug hotplug handler is activated. The pre_plug handler can than
> rely on the fact that basic checks already were performed.

pre_plug isn't part of device, it's a separate part that might vary
depending on machine and which might modify device properties along
the way and then exaggerating we would need 'prepare2()' and after
that 'pre_plug2()' and ...

Hence I dislike idea of adding more callbacks. I'd rather have a property
setter do the job of initializing state of device when necessary instead
of adding more callbacks. Which is in nvdimm case a bit more code compared
to doing it in realize() but should be possible to implement.

> In contrast to "realize()", "prepare()" should not perform any actions
> that would have to be rolled back in case e.g. pre_plug fails.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/qdev.c         |  7 +++++++
>  include/hw/qdev-core.h | 14 ++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ffec461791..3bfc7e0d54 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -814,6 +814,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              g_free(name);
>          }
>  
> +        if (dc->prepare) {
> +            dc->prepare(dev, &local_err);
> +            if (local_err != NULL) {
> +                goto fail;
> +            }
> +        }
> +
>          hotplug_ctrl = qdev_get_hotplug_handler(dev);
>          if (hotplug_ctrl) {
>              hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index f1fd0f8736..63520c1a55 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -29,6 +29,7 @@ typedef enum DeviceCategory {
>      DEVICE_CATEGORY_MAX
>  } DeviceCategory;
>  
> +typedef void (*DevicePrepare)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
>  typedef void (*DeviceReset)(DeviceState *dev);
> @@ -40,8 +41,11 @@ struct VMStateDescription;
>  /**
>   * DeviceClass:
>   * @props: Properties accessing state fields.
> + * @prepare: Callback function invoked when the #DeviceState:realized
> + * property is changed to %true, before invoking the pre_plug hotplug handler.
>   * @realize: Callback function invoked when the #DeviceState:realized
> - * property is changed to %true.
> + * property is changed to %true, after invoking the pre_plug hotplug handler
> + * but before invoking the plug handler.
>   * @unrealize: Callback function invoked when the #DeviceState:realized
>   * property is changed to %false.
>   * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
> @@ -54,7 +58,8 @@ struct VMStateDescription;
>   * The former may not fail (it might assert or exit), the latter may return
>   * error information to the caller and must be re-entrant.
>   * Trivial field initializations should go into #TypeInfo.instance_init.
> - * Operations depending on @props static properties should go into @realize.
> + * Operations depending on @props static properties should go into @prepare
> + * or @realize.
>   * After successful realization, setting static properties will fail.
>   *
>   * As an interim step, the #DeviceState:realized property can also be
> @@ -73,8 +78,8 @@ struct VMStateDescription;
>   *
>   * <note>
>   *   <para>
> - * Since TYPE_DEVICE doesn't implement @realize and @unrealize, types
> - * derived directly from it need not call their parent's @realize and
> + * Since TYPE_DEVICE doesn't implement @prepare, @realize and @unrealize, types
> + * derived directly from it need not call their parent's @prepare, @realize and
>   * @unrealize.
>   * For other types consult the documentation and implementation of the
>   * respective parent types.
> @@ -107,6 +112,7 @@ typedef struct DeviceClass {
>  
>      /* callbacks */
>      DeviceReset reset;
> +    DevicePrepare prepare;
>      DeviceRealize realize;
>      DeviceUnrealize unrealize;
>  

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand]

On 14.06.2018 17:00, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 16:50:54 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.06.2018 16:07, David Hildenbrand wrote:
>>> On 13.06.2018 15:03, Igor Mammedov wrote:  
>>>> On Mon, 11 Jun 2018 14:16:51 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>  
>>>>> We already verify when realizing that the memdev property has been
>>>>> set. We have no more accesses to get_memory_region() before the device
>>>>> is realized.  
>>>> this stems from assumption that get_memory_region shouldn't be called
>>>> before devices realize is executed, which I don't agree to begin with.
>>>>
>>>> However wrt error handling, we should probably leave device internal error
>>>> up to devices and make check for error only in pre_plug handler
>>>> (since pre_plug was successful, the rest machine helpers would have
>>>> access to the same region until device is removed.
>>>>  
>>>
>>> Something like a generic Device "validate()"/"pre_realize()" function,
>>> that can be called before realize() and validates all properties
>>> (+initializes derived properties) would be something I could agree to.
>>>
>>> Then we could drop all error handling from access functions (as they
>>> have been validated early during pre_plug())
>>>
>>> Moving all checks out of realize into pre_plug() looks ugly, because we
>>> have implementation details split across c-files.
>>>   
>>
>> I am thinking about something like this
>>
>> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@redhat.com>
>> Date: Wed, 13 Jun 2018 16:41:12 +0200
>> Subject: [PATCH RFC] device: add "prepare()" function
>>
>> The realize() function usually does several things. It validates
>> properties, inititalizes state based on properties and performs
>> e.g. registrations in the system that require "unrealize()" to be called
>> when removing the device.
>>
>> In a pre_plug hotplug handler, we usually want to access certain
>> properties or derived properties, e.g. to perform advanced checks
>> (for resource asignment). Now we have the problem, that realize() was
>> not called yet once we reach pre_plug(). So we theoretically have to
>> duplicate checks and add error paths for cases that can
>> theoretically never happen.
> I care less about duplicate checks in completely different parts of code,
> and I'd even insist on device:realize checks being self contained and not
> rely on any other external checks performed by users of device. And vice
> verse layer that uses device should do it's own checks when necessary
> and not rely on device's verification. That way loosely coupled code
> wouldn't fall apart whenever we drop or change checks in one of the parts.
> 
> The only case where I'd make concession is minimizing error handling
> on hot path for performance reasons and this is not the case here.
> 
>> Let's add the "prepare()" callback, which can be used to validate
>> properties and inititalize some state based on these. It will be called
>> once all static properties have been inititalized, but before the
>> pre_plug hotplug handler is activated. The pre_plug handler can than
>> rely on the fact that basic checks already were performed.
> 
> pre_plug isn't part of device, it's a separate part that might vary
> depending on machine and which might modify device properties along
> the way and then exaggerating we would need 'prepare2()' and after
> that 'pre_plug2()' and ...

That's how two parties (device vs hotplug handler) work together to get
results done ... Just like inititalize() -> realized() vs. pre_plug ->
plug(). There has to be some hand shaking.

> 
> Hence I dislike idea of adding more callbacks. I'd rather have a property
> setter do the job of initializing state of device when necessary instead
> of adding more callbacks. Which is in nvdimm case a bit more code compared
> to doing it in realize() but should be possible to implement.

Although I strongly disagree (especially about the fact of calling class
functions *anytime* after a device has been created but not initialized)
I will implement it like that for now (otherwise I won't be making any
progress here but instead be creating more and more problems to solve).


nvdimm will only have static properties. When realizing or when calling
get_memory_region(), properties will be checked and the nvdimm_mr
inititalized, if not already done.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[Igor Mammedov]

On Thu, 14 Jun 2018 17:11:38 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 14.06.2018 17:00, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 16:50:54 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 13.06.2018 16:07, David Hildenbrand wrote:  
> >>> On 13.06.2018 15:03, Igor Mammedov wrote:    
> >>>> On Mon, 11 Jun 2018 14:16:51 +0200
> >>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>    
> >>>>> We already verify when realizing that the memdev property has been
> >>>>> set. We have no more accesses to get_memory_region() before the device
> >>>>> is realized.    
> >>>> this stems from assumption that get_memory_region shouldn't be called
> >>>> before devices realize is executed, which I don't agree to begin with.
> >>>>
> >>>> However wrt error handling, we should probably leave device internal error
> >>>> up to devices and make check for error only in pre_plug handler
> >>>> (since pre_plug was successful, the rest machine helpers would have
> >>>> access to the same region until device is removed.
> >>>>    
> >>>
> >>> Something like a generic Device "validate()"/"pre_realize()" function,
> >>> that can be called before realize() and validates all properties
> >>> (+initializes derived properties) would be something I could agree to.
> >>>
> >>> Then we could drop all error handling from access functions (as they
> >>> have been validated early during pre_plug())
> >>>
> >>> Moving all checks out of realize into pre_plug() looks ugly, because we
> >>> have implementation details split across c-files.
> >>>     
> >>
> >> I am thinking about something like this
> >>
> >> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001
> >> From: David Hildenbrand <david@redhat.com>
> >> Date: Wed, 13 Jun 2018 16:41:12 +0200
> >> Subject: [PATCH RFC] device: add "prepare()" function
> >>
> >> The realize() function usually does several things. It validates
> >> properties, inititalizes state based on properties and performs
> >> e.g. registrations in the system that require "unrealize()" to be called
> >> when removing the device.
> >>
> >> In a pre_plug hotplug handler, we usually want to access certain
> >> properties or derived properties, e.g. to perform advanced checks
> >> (for resource asignment). Now we have the problem, that realize() was
> >> not called yet once we reach pre_plug(). So we theoretically have to
> >> duplicate checks and add error paths for cases that can
> >> theoretically never happen.  
> > I care less about duplicate checks in completely different parts of code,
> > and I'd even insist on device:realize checks being self contained and not
> > rely on any other external checks performed by users of device. And vice
> > verse layer that uses device should do it's own checks when necessary
> > and not rely on device's verification. That way loosely coupled code
> > wouldn't fall apart whenever we drop or change checks in one of the parts.
> > 
> > The only case where I'd make concession is minimizing error handling
> > on hot path for performance reasons and this is not the case here.
> >   
> >> Let's add the "prepare()" callback, which can be used to validate
> >> properties and inititalize some state based on these. It will be called
> >> once all static properties have been inititalized, but before the
> >> pre_plug hotplug handler is activated. The pre_plug handler can than
> >> rely on the fact that basic checks already were performed.  
> > 
> > pre_plug isn't part of device, it's a separate part that might vary
> > depending on machine and which might modify device properties along
> > the way and then exaggerating we would need 'prepare2()' and after
> > that 'pre_plug2()' and ...  
> 
> That's how two parties (device vs hotplug handler) work together to get
> results done ... Just like inititalize() -> realized() vs. pre_plug ->
> plug(). There has to be some hand shaking.
In general hotplug handler is optional, for example a board might
create initial memory wit fixed layout as:
  dev = object_new(dimm)
  set memdev + set addr
  qdev_init_no_fail()

generalized pc-dimm helpers for hotplug handler is just impl. detail,
a board might have other ideas how to use pc-dimm and choose to
implement wiring differently. So device model should do sanity checks
independently from whomever uses it.

> > 
> > Hence I dislike idea of adding more callbacks. I'd rather have a property
> > setter do the job of initializing state of device when necessary instead
> > of adding more callbacks. Which is in nvdimm case a bit more code compared
> > to doing it in realize() but should be possible to implement.  
> 
> Although I strongly disagree (especially about the fact of calling class
> functions *anytime* after a device has been created but not initialized)
> I will implement it like that for now (otherwise I won't be making any
> progress here but instead be creating more and more problems to solve).
> 
> 
> nvdimm will only have static properties. When realizing or when calling
> get_memory_region(), properties will be checked and the nvdimm_mr
> inititalized, if not already done.
I'm not a fun of using class callbacks (due undefined semantic),
but since we are already using it here, that might just work
in this case.

Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand]

>>> pre_plug isn't part of device, it's a separate part that might vary
>>> depending on machine and which might modify device properties along
>>> the way and then exaggerating we would need 'prepare2()' and after
>>> that 'pre_plug2()' and ...  
>>
>> That's how two parties (device vs hotplug handler) work together to get
>> results done ... Just like inititalize() -> realized() vs. pre_plug ->
>> plug(). There has to be some hand shaking.
> In general hotplug handler is optional, for example a board might
> create initial memory wit fixed layout as:
>   dev = object_new(dimm)
>   set memdev + set addr
>   qdev_init_no_fail()
> 
> generalized pc-dimm helpers for hotplug handler is just impl. detail,
> a board might have other ideas how to use pc-dimm and choose to
> implement wiring differently. So device model should do sanity checks
> independently from whomever uses it.

In general, I am not a fan of implementing things the "what if someday
..." way. It overcomplicates code and you can be sure that usually
something is missed either way (as there is no concrete use case, not
even in somebodys mind).

But I can see that for pc-dimms that can actually make sense (in
contrast to other, more specific devices).

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 08/11] pc-dimm: get_memory_region() will never return a NULL pointer

[David Hildenbrand]

This is guaranteed by passing into host_memory_backend_get_memory() a
value that is not NULL - which is what we always do.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 7bb6ce509c..9a0da5d441 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -157,7 +157,6 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
                              void *opaque, Error **errp)
 {
     uint64_t value;
-    MemoryRegion *mr;
     PCDIMMDevice *dimm = PC_DIMM(obj);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
 
@@ -167,11 +166,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    mr = ddc->get_memory_region(dimm);
-    if (!mr) {
-        return;
-    }
-    value = memory_region_size(mr);
+    value = memory_region_size(ddc->get_memory_region(dimm));
 
     visit_type_uint64(v, name, &value, errp);
 }
@@ -241,14 +236,8 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
     /* dropping const here is fine as we don't touch the memory region */
     PCDIMMDevice *dimm = PC_DIMM(md);
     const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
-    MemoryRegion *mr;
-
-    mr = ddc->get_memory_region(dimm);
-    if (!mr) {
-        return 0;
-    }
 
-    return memory_region_size(mr);
+    return memory_region_size(ddc->get_memory_region(dimm));
 }
 
 static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 08/11] pc-dimm: get_memory_region() will never return a NULL pointer

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:52PM +0200, David Hildenbrand wrote:
> This is guaranteed by passing into host_memory_backend_get_memory() a
> value that is not NULL - which is what we always do.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 7bb6ce509c..9a0da5d441 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -157,7 +157,6 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>                               void *opaque, Error **errp)
>  {
>      uint64_t value;
> -    MemoryRegion *mr;
>      PCDIMMDevice *dimm = PC_DIMM(obj);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>  
> @@ -167,11 +166,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm);
> -    if (!mr) {
> -        return;
> -    }
> -    value = memory_region_size(mr);
> +    value = memory_region_size(ddc->get_memory_region(dimm));
>  
>      visit_type_uint64(v, name, &value, errp);
>  }
> @@ -241,14 +236,8 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
>      /* dropping const here is fine as we don't touch the memory region */
>      PCDIMMDevice *dimm = PC_DIMM(md);
>      const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
> -    MemoryRegion *mr;
> -
> -    mr = ddc->get_memory_region(dimm);
> -    if (!mr) {
> -        return 0;
> -    }
>  
> -    return memory_region_size(mr);
> +    return memory_region_size(ddc->get_memory_region(dimm));
>  }
>  
>  static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,

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

[Qemu-devel] [PATCH v1 09/11] pc-dimm: remove pc_dimm_get_vmstate_memory_region()

[David Hildenbrand]

We can reuse pc_dimm_get_memory_region() now, as both functions are
(besides the assert which is also correct), equal.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9a0da5d441..bc79dd04d8 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -219,11 +219,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem);
 }
 
-static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-    return host_memory_backend_get_memory(dimm->hostmem);
-}
-
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
 {
     const PCDIMMDevice *dimm = PC_DIMM(md);
@@ -282,7 +277,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     dc->desc = "DIMM memory module";
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
-    ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+    ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
 
     mdc->get_addr = pc_dimm_md_get_addr;
     /* for a dimm plugged_size == region_size */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 09/11] pc-dimm: remove pc_dimm_get_vmstate_memory_region()

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:53PM +0200, David Hildenbrand wrote:
> We can reuse pc_dimm_get_memory_region() now, as both functions are
> (besides the assert which is also correct), equal.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9a0da5d441..bc79dd04d8 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -219,11 +219,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
>      return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
> -static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> -{
> -    return host_memory_backend_get_memory(dimm->hostmem);
> -}
> -
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>  {
>      const PCDIMMDevice *dimm = PC_DIMM(md);
> @@ -282,7 +277,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      dc->desc = "DIMM memory module";
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
> -    ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> +    ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
>      /* for a dimm plugged_size == region_size */

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

[Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[David Hildenbrand]

We'll be factoring out some pc-dimm specific and some memory-device
checks next.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 017396fe84..dc8e7b033b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
         return;
     }
+
+    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
 }
 
 static void pc_dimm_plug(HotplugHandler *hotplug_dev,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bc79dd04d8..995ce22d8d 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,6 +27,11 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
+void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
+                             Error **errp)
+{
+}
+
 void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
                          uint64_t align, Error **errp)
 {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 214286fd2f..54eddc0069 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
+    pc_dimm_memory_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 f0e6867803..7d46a0a0cb 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass {
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
+void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
+                             Error **errp);
 void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
                          uint64_t align, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:54PM +0200, David Hildenbrand wrote:
> We'll be factoring out some pc-dimm specific and some memory-device
> checks next.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

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

> ---
>  hw/i386/pc.c             | 2 ++
>  hw/mem/pc-dimm.c         | 5 +++++
>  hw/ppc/spapr.c           | 1 +
>  include/hw/mem/pc-dimm.h | 2 ++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 017396fe84..dc8e7b033b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>          return;
>      }
> +
> +    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  }
>  
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index bc79dd04d8..995ce22d8d 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,6 +27,11 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
> +                             Error **errp)
> +{
> +}
> +
>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 214286fd2f..54eddc0069 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    pc_dimm_memory_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 f0e6867803..7d46a0a0cb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
> +                             Error **errp);
>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp);
>  void pc_dimm_memory_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 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> We'll be factoring out some pc-dimm specific and some memory-device
> checks next.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c             | 2 ++
>  hw/mem/pc-dimm.c         | 5 +++++
>  hw/ppc/spapr.c           | 1 +
>  include/hw/mem/pc-dimm.h | 2 ++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 017396fe84..dc8e7b033b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
keeping                              ^^^^^
similar to newly introduced pc_dimm_memory_pre_plug()
and I have no clue what to suggest as alternative

>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>          return;
>      }
> +
> +    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
>  }
>  
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index bc79dd04d8..995ce22d8d 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,6 +27,11 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
> +                             Error **errp)
> +{
> +}
why introducing shim without anything?
I'd just merge it with following patch and clarify (in commit message)
why the rest of pre_plug code isn't moved here as well.

>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 214286fd2f..54eddc0069 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    pc_dimm_memory_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 f0e6867803..7d46a0a0cb 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
> +                             Error **errp);
>  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>                           uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);

Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[David Hildenbrand]

On 13.06.2018 15:10, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:54 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We'll be factoring out some pc-dimm specific and some memory-device
>> checks next.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/i386/pc.c             | 2 ++
>>  hw/mem/pc-dimm.c         | 5 +++++
>>  hw/ppc/spapr.c           | 1 +
>>  include/hw/mem/pc-dimm.h | 2 ++
>>  4 files changed, 10 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 017396fe84..dc8e7b033b 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> keeping                              ^^^^^
> similar to newly introduced pc_dimm_memory_pre_plug()
> and I have no clue what to suggest as alternative

Can you elaborate?

In pc.c we now have:
- static void pc_dimm_plug()
- static void pc_dimm_unplug_request()
- static void pc_dimm_unplug()

And I add
- static void pc_dimm_pre_plug()

I am sticking to the existing naming scheme.

(maybe the problem is that PC_DIMM should never have been named PC_DIMM
but simply DIMM, then the "pc_" prefix would be unique)

> 
>>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>>          return;
>>      }
>> +
>> +    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
>>  }
>>  
>>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index bc79dd04d8..995ce22d8d 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -27,6 +27,11 @@
>>  #include "sysemu/numa.h"
>>  #include "trace.h"
>>  
>> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
>> +                             Error **errp)
>> +{
>> +}
> why introducing shim without anything?

Because you requested for review to split things up :)

I can easily squash this.

> I'd just merge it with following patch and clarify (in commit message)
> why the rest of pre_plug code isn't moved here as well.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[Igor Mammedov]

On Wed, 13 Jun 2018 16:15:48 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 13.06.2018 15:10, Igor Mammedov wrote:
> > On Mon, 11 Jun 2018 14:16:54 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> We'll be factoring out some pc-dimm specific and some memory-device
> >> checks next.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/i386/pc.c             | 2 ++
> >>  hw/mem/pc-dimm.c         | 5 +++++
> >>  hw/ppc/spapr.c           | 1 +
> >>  include/hw/mem/pc-dimm.h | 2 ++
> >>  4 files changed, 10 insertions(+)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 017396fe84..dc8e7b033b 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,  
> > keeping                              ^^^^^
> > similar to newly introduced pc_dimm_memory_pre_plug()
> > and I have no clue what to suggest as alternative  
> 
> Can you elaborate?

It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug
are very similar so it become confusing and with name alone
you can't figure if both do the same or different things.

Looking at 11/11 maybe you could just drop this and the next
patch for now as there isn't obvious (if any) demand for it
within this series at all.
And introduce similar patch when it's actually required.

I might imagine following naming in future:

   pc_dimm_pre_plug()
       memory_device_pre_plug()
       ... some pc-dimm only stuff ...

   virtio_mem_pre_plug()
       memory_device_pre_plug()
       ... some virtio-mem only stuff ...

> In pc.c we now have:
> - static void pc_dimm_plug()
> - static void pc_dimm_unplug_request()
> - static void pc_dimm_unplug()
> 
> And I add
> - static void pc_dimm_pre_plug()
> 
> I am sticking to the existing naming scheme.
> 
> (maybe the problem is that PC_DIMM should never have been named PC_DIMM
> but simply DIMM, then the "pc_" prefix would be unique)
Agreed,
but now type name is part of interface so we are stuck with it.


> >>          error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> >>          return;
> >>      }
> >> +
> >> +    pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
> >>  }
> >>  
> >>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >> index bc79dd04d8..995ce22d8d 100644
> >> --- a/hw/mem/pc-dimm.c
> >> +++ b/hw/mem/pc-dimm.c
> >> @@ -27,6 +27,11 @@
> >>  #include "sysemu/numa.h"
> >>  #include "trace.h"
> >>  
> >> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
> >> +                             Error **errp)
> >> +{
> >> +}  
> > why introducing shim without anything?  
> 
> Because you requested for review to split things up :)
> 
> I can easily squash this.
> 
> > I'd just merge it with following patch and clarify (in commit message)
> > why the rest of pre_plug code isn't moved here as well.  
> 
> 

Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[David Hildenbrand]

On 15.06.2018 11:34, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 16:15:48 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.06.2018 15:10, Igor Mammedov wrote:
>>> On Mon, 11 Jun 2018 14:16:54 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> We'll be factoring out some pc-dimm specific and some memory-device
>>>> checks next.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/i386/pc.c             | 2 ++
>>>>  hw/mem/pc-dimm.c         | 5 +++++
>>>>  hw/ppc/spapr.c           | 1 +
>>>>  include/hw/mem/pc-dimm.h | 2 ++
>>>>  4 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 017396fe84..dc8e7b033b 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,  
>>> keeping                              ^^^^^
>>> similar to newly introduced pc_dimm_memory_pre_plug()
>>> and I have no clue what to suggest as alternative  
>>
>> Can you elaborate?
> 
> It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug
> are very similar so it become confusing and with name alone
> you can't figure if both do the same or different things.
> 
> Looking at 11/11 maybe you could just drop this and the next
> patch for now as there isn't obvious (if any) demand for it
> within this series at all.
> And introduce similar patch when it's actually required.
> 
> I might imagine following naming in future:
> 
>    pc_dimm_pre_plug()
>        memory_device_pre_plug()
>        ... some pc-dimm only stuff ...
> 
>    virtio_mem_pre_plug()
>        memory_device_pre_plug()
>        ... some virtio-mem only stuff ...

We will always have the chain

$machine_XXX_pre_plug()
	... some machine specific stuff
	pc_dimm_memory_pre_plug()
		... some pc-dimm specific stuff
		memory_device_pre_plug()

I can rename

pc_dimm_(plug|unplug ...) to
pc_memory_(plug|unplug ...)

and pc_dimm_memory_(plug|unplug ...) to
pc_dimm_(plug|unplug ...)

That way we match spapr naming scheme:

spapr_memory_plug/spapr_memory_unplug

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()

[Igor Mammedov]

On Fri, 15 Jun 2018 11:48:33 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 15.06.2018 11:34, Igor Mammedov wrote:
> > On Wed, 13 Jun 2018 16:15:48 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 13.06.2018 15:10, Igor Mammedov wrote:  
> >>> On Mon, 11 Jun 2018 14:16:54 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> We'll be factoring out some pc-dimm specific and some memory-device
> >>>> checks next.
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>  hw/i386/pc.c             | 2 ++
> >>>>  hw/mem/pc-dimm.c         | 5 +++++
> >>>>  hw/ppc/spapr.c           | 1 +
> >>>>  include/hw/mem/pc-dimm.h | 2 ++
> >>>>  4 files changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>>> index 017396fe84..dc8e7b033b 100644
> >>>> --- a/hw/i386/pc.c
> >>>> +++ b/hw/i386/pc.c
> >>>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,    
> >>> keeping                              ^^^^^
> >>> similar to newly introduced pc_dimm_memory_pre_plug()
> >>> and I have no clue what to suggest as alternative    
> >>
> >> Can you elaborate?  
> > 
> > It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug
> > are very similar so it become confusing and with name alone
> > you can't figure if both do the same or different things.
> > 
> > Looking at 11/11 maybe you could just drop this and the next
> > patch for now as there isn't obvious (if any) demand for it
> > within this series at all.
> > And introduce similar patch when it's actually required.
> > 
> > I might imagine following naming in future:
> > 
> >    pc_dimm_pre_plug()
> >        memory_device_pre_plug()
> >        ... some pc-dimm only stuff ...
> > 
> >    virtio_mem_pre_plug()
> >        memory_device_pre_plug()
> >        ... some virtio-mem only stuff ...  
> 
> We will always have the chain
> 
> $machine_XXX_pre_plug()
> 	... some machine specific stuff
> 	pc_dimm_memory_pre_plug()
> 		... some pc-dimm specific stuff
> 		memory_device_pre_plug()
> 
> I can rename
> 
> pc_dimm_(plug|unplug ...) to
> pc_memory_(plug|unplug ...)
> 
> and pc_dimm_memory_(plug|unplug ...) to
> pc_dimm_(plug|unplug ...)
> 
> That way we match spapr naming scheme:
> 
> spapr_memory_plug/spapr_memory_unplug

sounds good to me

[Qemu-devel] [PATCH v1 11/11] 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 change, so let's reduce
error handling a bit by using &error_abort.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 995ce22d8d..88423f95a3 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -30,12 +30,25 @@
 void pc_dimm_memory_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_memory_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);
@@ -61,22 +74,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
     }
     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);
 
-- 
2.17.1

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

[David Gibson]

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

On Mon, Jun 11, 2018 at 02:16:55PM +0200, David Hildenbrand wrote:
> We can assign and verify the slot before realizing and trying to plug.
> reading/writing the slot property should never change, so let's reduce
> error handling a bit by using &error_abort.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/mem/pc-dimm.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 995ce22d8d..88423f95a3 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -30,12 +30,25 @@
>  void pc_dimm_memory_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_memory_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);
> @@ -61,22 +74,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>      }
>      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);
>  

-- 
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 00/11] pc-dimm: next bunch of cleanups

[Igor Mammedov]

On Mon, 11 Jun 2018 14:16:44 +0200
David Hildenbrand <david@redhat.com> wrote:

> This is another set of cleanups as the result from
>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
> And is based on the two series
>     [PATCH v1 0/2] memory: fix alignment checks/asserts
>     [PATCH v2 0/6] spapr: machine hotplug handler cleanups
> 
> These cleanup are the last step before factoring out the
> pre_plug, plug and unplug logic of memory devices completely, to make it
> more independent from pc-dimm.
patches 1-4 are fine,
the rest starting from 5/11 is based on wrong assumptions so should
be reworked or dropped.

> But these patches will already try to make an important point: Assigning
> physical addresses of memory devices will not be done in pre_plug but
> during plug. Other properties, like the "slot" property, however can be
> handled during pre_plug and is done in this patch series, because they
> don't realy on the device to be realized.
> 
> Igor proposed to move all property checks + assigmnets to the pre_plug
> stage. Hovewer for determining a physical address of a memory device, we
> need to know the exact size and the alignment of the underlying memory
> region.
> 
> This region might not be available and initialized before the device has
> been realized (e.g. for NVDIMM). So my point is: Accessing derived
> "properties" ("memory region" set up based on "memdev" property and maybe
> others e.g. for NVDIMM) via device class functions should not be done
> before the device has been realized. These functions should not be
> called during pre_plug.
It's impl. issue/bug of NVDIMM where it does initialization late, which
leads to access to uninitialized region in several places and we should fix.

There is nothing fundamental that prevents accessing size/memory of
backend that was linked to dimm device at pre_plug() time,
since all user specified properties are already set and it's time
for machine to check if properties are correct from machine's pov
and set its own values before proceeding to device.realize() and
plug() handler. That includes setting default GPA property. 

Hence I'm not willing to agree going backwards or adding more
code/refactoring based on broken behavior.

> 
> Enforcing this, already leads to sime nice cleanup numbers in pc-dimm
> code.
> 
> David Hildenbrand (11):
>   pc-dimm: remove leftover "struct pc_dimms_capacity"
>   nvdimm: no need to overwrite get_vmstate_memory_region()
>   pc: factor out pc-dimm checks into pc_dimm_pre_plug()
>   hostmem: drop error variable from host_memory_backend_get_memory()
>   spapr: move memory hotplug size check into plug code
>   pc-dimm: don't allow to access "size" before the device was realized
>   pc-dimm: get_memory_region() can never fail
>   pc-dimm: get_memory_region() will never return a NULL pointer
>   pc-dimm: remove pc_dimm_get_vmstate_memory_region()
>   pc-dimm: introduce and use pc_dimm_memory_pre_plug()
>   pc-dimm: assign and verify the "slot" property during pre_plug
> 
>  backends/hostmem.c       |  3 +-
>  hw/i386/pc.c             | 53 ++++++++++++++------------
>  hw/mem/nvdimm.c          | 12 ++----
>  hw/mem/pc-dimm.c         | 82 +++++++++++++++-------------------------
>  hw/misc/ivshmem.c        |  3 +-
>  hw/ppc/spapr.c           | 36 +++++-------------
>  include/hw/mem/pc-dimm.h |  6 ++-
>  include/sysemu/hostmem.h |  3 +-
>  numa.c                   |  3 +-
>  9 files changed, 81 insertions(+), 120 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups

[David Hildenbrand]

On 13.06.2018 15:34, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:44 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> This is another set of cleanups as the result from
>>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
>> And is based on the two series
>>     [PATCH v1 0/2] memory: fix alignment checks/asserts
>>     [PATCH v2 0/6] spapr: machine hotplug handler cleanups
>>
>> These cleanup are the last step before factoring out the
>> pre_plug, plug and unplug logic of memory devices completely, to make it
>> more independent from pc-dimm.
> patches 1-4 are fine,
> the rest starting from 5/11 is based on wrong assumptions so should
> be reworked or dropped.

@Paolo can you pick up patch 1-4, so we have them out of the way (while
I potentially create new and more patches?)

> 
>> But these patches will already try to make an important point: Assigning
>> physical addresses of memory devices will not be done in pre_plug but
>> during plug. Other properties, like the "slot" property, however can be
>> handled during pre_plug and is done in this patch series, because they
>> don't realy on the device to be realized.
>>
>> Igor proposed to move all property checks + assigmnets to the pre_plug
>> stage. Hovewer for determining a physical address of a memory device, we
>> need to know the exact size and the alignment of the underlying memory
>> region.
>>
>> This region might not be available and initialized before the device has
>> been realized (e.g. for NVDIMM). So my point is: Accessing derived
>> "properties" ("memory region" set up based on "memdev" property and maybe
>> others e.g. for NVDIMM) via device class functions should not be done
>> before the device has been realized. These functions should not be
>> called during pre_plug.
> It's impl. issue/bug of NVDIMM where it does initialization late, which
> leads to access to uninitialized region in several places and we should fix.
> 
> There is nothing fundamental that prevents accessing size/memory of
> backend that was linked to dimm device at pre_plug() time,
> since all user specified properties are already set and it's time
> for machine to check if properties are correct from machine's pov
> and set its own values before proceeding to device.realize() and
> plug() handler. That includes setting default GPA property. 
> 
> Hence I'm not willing to agree going backwards or adding more
> code/refactoring based on broken behavior.
> 
>>
>> Enforcing this, already leads to sime nice cleanup numbers in pc-dimm
>> code.
>>
>> David Hildenbrand (11):
>>   pc-dimm: remove leftover "struct pc_dimms_capacity"
>>   nvdimm: no need to overwrite get_vmstate_memory_region()
>>   pc: factor out pc-dimm checks into pc_dimm_pre_plug()
>>   hostmem: drop error variable from host_memory_backend_get_memory()
>>   spapr: move memory hotplug size check into plug code
>>   pc-dimm: don't allow to access "size" before the device was realized
>>   pc-dimm: get_memory_region() can never fail
>>   pc-dimm: get_memory_region() will never return a NULL pointer
>>   pc-dimm: remove pc_dimm_get_vmstate_memory_region()
>>   pc-dimm: introduce and use pc_dimm_memory_pre_plug()
>>   pc-dimm: assign and verify the "slot" property during pre_plug
>>
>>  backends/hostmem.c       |  3 +-
>>  hw/i386/pc.c             | 53 ++++++++++++++------------
>>  hw/mem/nvdimm.c          | 12 ++----
>>  hw/mem/pc-dimm.c         | 82 +++++++++++++++-------------------------
>>  hw/misc/ivshmem.c        |  3 +-
>>  hw/ppc/spapr.c           | 36 +++++-------------
>>  include/hw/mem/pc-dimm.h |  6 ++-
>>  include/sysemu/hostmem.h |  3 +-
>>  numa.c                   |  3 +-
>>  9 files changed, 81 insertions(+), 120 deletions(-)
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups

[David Hildenbrand]

On 13.06.2018 16:11, David Hildenbrand wrote:
> On 13.06.2018 15:34, Igor Mammedov wrote:
>> On Mon, 11 Jun 2018 14:16:44 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> This is another set of cleanups as the result from
>>>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
>>> And is based on the two series
>>>     [PATCH v1 0/2] memory: fix alignment checks/asserts
>>>     [PATCH v2 0/6] spapr: machine hotplug handler cleanups
>>>
>>> These cleanup are the last step before factoring out the
>>> pre_plug, plug and unplug logic of memory devices completely, to make it
>>> more independent from pc-dimm.
>> patches 1-4 are fine,
>> the rest starting from 5/11 is based on wrong assumptions so should
>> be reworked or dropped.
> 
> @Paolo can you pick up patch 1-4, so we have them out of the way (while
> I potentially create new and more patches?)

I'll drag these 4 along and perform some renaming in patch nr 3.


-- 

Thanks,

David / dhildenb