[Qemu-devel] [PATCH v2 00/12] pc-dimm: next bunch of cleanups

[Qemu-devel] [PATCH v2 00/12] 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
    [PATCH v1 0/2] memory: fix alignment checks/asserts

These cleanup are the last step before
- 1. moving pc-dimm address and slot assignment into pc_dimm_pre_plug
- 2. factoring out pre_plug, plug and unplug logic of memory devices
     completely

v1 -> v2:
- deferring "pc-dimm: assign and verify the "slot" property during pre_plug"
- deferring "pc-dimm: introduce and use pc_dimm_memory_pre_plug()"
- dropped "pc-dimm: get_memory_region() will never return a NULL pointer"
- dropped "pc-dimm: don't allow to access "size" before the device was realized"
- dropped "spapr: move memory hotplug size check into plug code"
- dropped "pc-dimm: get_memory_region() can never fail"
-- replaces by "pc-dimm: get_memory_region() will not fail after realize"
- added "pc: rename pc_dimm_(plug|unplug|...)* into ..."
- added "pc-dimm: rename pc_dimm_memory_* to pc_dimm_*"
- added "pc-dimm: remove pc_dimm_get_free_slot() from header"
- added "pc-dimm: merge get_(vmstate_)memory_region()"
- added "nvdimm: convert "unarmed" into a static property"
- added "nvdimm: convert "label-size" into a static property"
- added "nvdimm: make get_memory_region() perform checks and initialization"


David Hildenbrand (12):
  pc-dimm: remove leftover "struct pc_dimms_capacity"
  pc: rename pc_dimm_(plug|unplug|...)* into
    pc_memory_(plug|unplug|...)*
  pc-dimm: rename pc_dimm_memory_* to pc_dimm_*
  pc-dimm: remove pc_dimm_get_free_slot() from header
  pc: factor out pc specific dimm checks into pc_memory_pre_plug()
  hostmem: drop error variable from host_memory_backend_get_memory()
  pc-dimm: merge get_(vmstate_)memory_region()
  nvdimm: no need to overwrite get_vmstate_memory_region()
  nvdimm: convert "unarmed" into a static property
  nvdimm: convert "label-size" into a static property
  nvdimm: make get_memory_region() perform checks and initialization
  pc-dimm: get_memory_region() will not fail after realize

 backends/hostmem.c       |   3 +-
 hw/i386/pc.c             |  73 +++++++++++-----------
 hw/mem/nvdimm.c          | 130 +++++++++++++++------------------------
 hw/mem/pc-dimm.c         |  35 ++++-------
 hw/misc/ivshmem.c        |   3 +-
 hw/ppc/spapr.c           |  18 ++----
 include/hw/mem/pc-dimm.h |  17 ++---
 include/sysemu/hostmem.h |   3 +-
 numa.c                   |   3 +-
 9 files changed, 116 insertions(+), 169 deletions(-)

-- 
2.17.1

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

[David Hildenbrand]

Not needed anymore, let's drop it.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/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

[Qemu-devel] [PATCH v2 02/12] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[David Hildenbrand]

Use a similar naming scheme as spapr. This way, we can go ahead and
rename e.g. pc_dimm_memory_plug to pc_dimm_plug_plug, which avoids
confusion.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 622e49d6bc..f9250ffae7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1674,8 +1674,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
 }
 
-static void pc_dimm_plug(HotplugHandler *hotplug_dev,
-                         DeviceState *dev, Error **errp)
+static void pc_memory_plug(HotplugHandler *hotplug_dev,
+                           DeviceState *dev, Error **errp)
 {
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
@@ -1728,8 +1728,8 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
-                                   DeviceState *dev, Error **errp)
+static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
@@ -1759,8 +1759,8 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
-                           DeviceState *dev, Error **errp)
+static void pc_memory_unplug(HotplugHandler *hotplug_dev,
+                             DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     HotplugHandlerClass *hhc;
@@ -2015,7 +2015,7 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_plug(hotplug_dev, dev, errp);
     }
@@ -2025,7 +2025,7 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+        pc_memory_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
     } else {
@@ -2038,7 +2038,7 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_memory_unplug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_cb(hotplug_dev, dev, errp);
     } else {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 02/12] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:50 +0200
David Hildenbrand <david@redhat.com> wrote:

> Use a similar naming scheme as spapr. This way, we can go ahead and
> rename e.g. pc_dimm_memory_plug to pc_dimm_plug_plug, which avoids
                                     ^^^^^ typo?

> confusion.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 622e49d6bc..f9250ffae7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1674,8 +1674,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>      }
>  }
>  
> -static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> -                         DeviceState *dev, Error **errp)
> +static void pc_memory_plug(HotplugHandler *hotplug_dev,
> +                           DeviceState *dev, Error **errp)
>  {
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
> @@ -1728,8 +1728,8 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp)
> +static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev, Error **errp)
>  {
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
> @@ -1759,8 +1759,8 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> -                           DeviceState *dev, Error **errp)
> +static void pc_memory_unplug(HotplugHandler *hotplug_dev,
> +                             DeviceState *dev, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      HotplugHandlerClass *hhc;
> @@ -2015,7 +2015,7 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_plug(hotplug_dev, dev, errp);
> +        pc_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_plug(hotplug_dev, dev, errp);
>      }
> @@ -2025,7 +2025,7 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                  DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug_request(hotplug_dev, dev, errp);
> +        pc_memory_unplug_request(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>      } else {
> @@ -2038,7 +2038,7 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug(hotplug_dev, dev, errp);
> +        pc_memory_unplug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>      } else {

Re: [Qemu-devel] [PATCH v2 02/12] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[David Hildenbrand]

On 15.06.2018 13:51, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 13:24:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Use a similar naming scheme as spapr. This way, we can go ahead and
>> rename e.g. pc_dimm_memory_plug to pc_dimm_plug_plug, which avoids
>                                      ^^^^^ typo?

Indeed, luckily only in the patch description.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 02/12] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[Igor Mammedov]

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

> On 15.06.2018 13:51, Igor Mammedov wrote:
> > On Fri, 15 Jun 2018 13:24:50 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Use a similar naming scheme as spapr. This way, we can go ahead and
> >> rename e.g. pc_dimm_memory_plug to pc_dimm_plug_plug, which avoids  
> >                                      ^^^^^ typo?  
> 
> Indeed, luckily only in the patch description.

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

[Qemu-devel] [PATCH v2 03/12] pc-dimm: rename pc_dimm_memory_* to pc_dimm_*

[David Hildenbrand]

Let's rename it to make it look more consistent.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f9250ffae7..f23133facc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1713,7 +1713,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
     if (local_err) {
         goto out;
     }
@@ -1773,7 +1773,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_unplug(dev, MACHINE(pcms));
+    pc_dimm_unplug(dev, MACHINE(pcms));
     object_unparent(OBJECT(dev));
 
  out:
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 62b34a992e..9e0c83e415 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,8 +27,8 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp)
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
+                  Error **errp)
 {
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
@@ -84,7 +84,7 @@ out:
     error_propagate(errp, local_err);
 }
 
-void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
+void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daac..3e5320020f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3153,7 +3153,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
-    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
     if (local_err) {
         goto out;
     }
@@ -3176,7 +3176,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     return;
 
 out_unplug:
-    pc_dimm_memory_unplug(dev, MACHINE(ms));
+    pc_dimm_unplug(dev, MACHINE(ms));
 out:
     error_propagate(errp, local_err);
 }
@@ -3328,7 +3328,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
     sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
-    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
+    pc_dimm_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 627c8601d9..860343d64f 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,7 +78,7 @@ typedef struct PCDIMMDeviceClass {
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, 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);
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
+                  Error **errp);
+void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
 #endif
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 03/12] pc-dimm: rename pc_dimm_memory_* to pc_dimm_*

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's rename it to make it look more consistent.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/i386/pc.c             | 4 ++--
>  hw/mem/pc-dimm.c         | 6 +++---
>  hw/ppc/spapr.c           | 6 +++---
>  include/hw/mem/pc-dimm.h | 6 +++---
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f9250ffae7..f23133facc 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1713,7 +1713,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -1773,7 +1773,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_unplug(dev, MACHINE(pcms));
> +    pc_dimm_unplug(dev, MACHINE(pcms));
>      object_unparent(OBJECT(dev));
>  
>   out:
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 62b34a992e..9e0c83e415 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,8 +27,8 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp)
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> +                  Error **errp)
>  {
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -84,7 +84,7 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
> +void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..3e5320020f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,7 +3153,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> -    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -3176,7 +3176,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      return;
>  
>  out_unplug:
> -    pc_dimm_memory_unplug(dev, MACHINE(ms));
> +    pc_dimm_unplug(dev, MACHINE(ms));
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -3328,7 +3328,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
>  
> -    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
> +    pc_dimm_unplug(dev, MACHINE(hotplug_dev));
>      object_unparent(OBJECT(dev));
>      spapr_pending_dimm_unplugs_remove(spapr, ds);
>  }
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..860343d64f 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -78,7 +78,7 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, 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);
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> +                  Error **errp);
> +void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
>  #endif

[Qemu-devel] [PATCH v2 04/12] pc-dimm: remove pc_dimm_get_free_slot() from header

[David Hildenbrand]

Not used outside of pc-dimm.c and there shouldn't be other users. If
other devices ever have to also sue slots, this has to be factored out.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9e0c83e415..7387963cf1 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,6 +27,8 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
+static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
+
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp)
 {
@@ -111,7 +113,7 @@ static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
     return 0;
 }
 
-int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
+static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
 {
     unsigned long *bitmap;
     int slot = 0;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 860343d64f..cf71247630 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -76,8 +76,6 @@ typedef struct PCDIMMDeviceClass {
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
-
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp);
 void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 04/12] pc-dimm: remove pc_dimm_get_free_slot() from header

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:52 +0200
David Hildenbrand <david@redhat.com> wrote:

> Not used outside of pc-dimm.c and there shouldn't be other users. If
> other devices ever have to also sue slots, this has to be factored out.
s/sue/use/ or better rephrase the last sentence (it's hard to read)

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

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/pc-dimm.c         | 4 +++-
>  include/hw/mem/pc-dimm.h | 2 --
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9e0c83e415..7387963cf1 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,6 +27,8 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> +static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
> +
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp)
>  {
> @@ -111,7 +113,7 @@ static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
>      return 0;
>  }
>  
> -int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
> +static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
>  {
>      unsigned long *bitmap;
>      int slot = 0;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 860343d64f..cf71247630 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -76,8 +76,6 @@ typedef struct PCDIMMDeviceClass {
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> -int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
> -
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);

[Qemu-devel] [PATCH v2 05/12] pc: factor out pc specific dimm checks into pc_memory_pre_plug()

[David Hildenbrand]

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

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f23133facc..2db032a6df 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_memory_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_memory_plug(HotplugHandler *hotplug_dev,
                            DeviceState *dev, Error **errp)
 {
@@ -1696,23 +1719,6 @@ static void pc_memory_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_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_memory_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

[Qemu-devel] [PATCH v2 06/12] hostmem: drop error variable from host_memory_backend_get_memory()

[David Hildenbrand]

Unused, so let's remove it.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
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 4087aca25e..37e19044d6 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 7387963cf1..73f0eee4c7 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -226,12 +226,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 v2 06/12] hostmem: drop error variable from host_memory_backend_get_memory()

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> Unused, so let's remove it.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
breaks compilation with

 hw/mem/nvdimm.c:171:5: error: too many arguments to function ‘host_memory_backend_get_memory’
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);


> ---
>  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 4087aca25e..37e19044d6 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 7387963cf1..73f0eee4c7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -226,12 +226,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));

Re: [Qemu-devel] [PATCH v2 06/12] hostmem: drop error variable from host_memory_backend_get_memory()

[David Hildenbrand]

On 15.06.2018 14:39, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 13:24:54 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Unused, so let's remove it.
>>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> breaks compilation with
> 
>  hw/mem/nvdimm.c:171:5: error: too many arguments to function ‘host_memory_backend_get_memory’
>      return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
> 

Looks like some reshuffling of patches was not that healthy. Will fix.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v2 07/12] pc-dimm: merge get_(vmstate_)memory_region()

[David Hildenbrand]

Importantly, get_vmstate_memory_region() should also fail with a proper
error if called before the device is realized. For a PCDIMM, both functions
are to return the same thing, so share the implementation.

All current users are called after the device has been realized, so we
can expect the calls to succeed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c         | 13 +++++--------
 include/hw/mem/pc-dimm.h |  3 ++-
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 73f0eee4c7..4ff39b59ef 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -35,7 +35,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
+    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
+                                                              &error_abort);
     Error *local_err = NULL;
     MemoryRegion *mr;
     uint64_t addr;
@@ -90,7 +91,8 @@ void pc_dimm_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 *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
+                                                              &error_abort);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
 
     memory_device_unplug_region(machine, mr);
@@ -229,11 +231,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **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);
-}
-
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
 {
     const PCDIMMDevice *dimm = PC_DIMM(md);
@@ -298,7 +295,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 */
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index cf71247630..5679a80465 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -73,7 +73,8 @@ typedef struct PCDIMMDeviceClass {
     /* public */
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
-    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
+    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
+                                               Error **errp);
 } PCDIMMDeviceClass;
 
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 07/12] pc-dimm: merge get_(vmstate_)memory_region()

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:55 +0200
David Hildenbrand <david@redhat.com> wrote:

> Importantly, get_vmstate_memory_region() should also fail with a proper
> error if called before the device is realized. For a PCDIMM, both functions
> are to return the same thing, so share the implementation.
> 
> All current users are called after the device has been realized, so we
> can expect the calls to succeed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/pc-dimm.c         | 13 +++++--------
>  include/hw/mem/pc-dimm.h |  3 ++-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 73f0eee4c7..4ff39b59ef 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -35,7 +35,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> +    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      Error *local_err = NULL;
>      MemoryRegion *mr;
>      uint64_t addr;
> @@ -90,7 +91,8 @@ void pc_dimm_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 *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>  
>      memory_device_unplug_region(machine, mr);
> @@ -229,11 +231,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **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);
> -}
> -
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>  {
>      const PCDIMMDevice *dimm = PC_DIMM(md);
> @@ -298,7 +295,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 */
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index cf71247630..5679a80465 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -73,7 +73,8 @@ typedef struct PCDIMMDeviceClass {
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> +    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
> +                                               Error **errp);
nvdimm also uses this callback, so patch as it is will break bisection.


>  } PCDIMMDeviceClass;
>  
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,

[Qemu-devel] [PATCH v2 08/12] nvdimm: no need to overwrite get_vmstate_memory_region()

[David Hildenbrand]

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

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 37e19044d6..df9716231f 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 v2 08/12] nvdimm: no need to overwrite get_vmstate_memory_region()

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:56 +0200
David Hildenbrand <david@redhat.com> wrote:

> Our parent class (PC_DIMM) provides exactly the same function.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 37e19044d6..df9716231f 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;

maybe put this patch before 7/12 ?

Re: [Qemu-devel] [PATCH v2 08/12] nvdimm: no need to overwrite get_vmstate_memory_region()

[David Hildenbrand]

On 15.06.2018 14:46, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 13:24:56 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Our parent class (PC_DIMM) provides exactly the same function.
>>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/nvdimm.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>> index 37e19044d6..df9716231f 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;
> 
> maybe put this patch before 7/12 ?
> 

Yes, that's how it was before moving all nvdimm patches into one chunk.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v2 09/12] nvdimm: convert "unarmed" into a static property

[David Hildenbrand]

We don't allow to modify it after realization. So we can simply turn
it into a static property.

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

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index df9716231f..7260c9c6b1 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -64,36 +64,11 @@ out:
     error_propagate(errp, local_err);
 }
 
-static bool nvdimm_get_unarmed(Object *obj, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-
-    return nvdimm->unarmed;
-}
-
-static void nvdimm_set_unarmed(Object *obj, bool value, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-    Error *local_err = NULL;
-
-    if (memory_region_size(&nvdimm->nvdimm_mr)) {
-        error_setg(&local_err, "cannot change property value");
-        goto out;
-    }
-
-    nvdimm->unarmed = value;
-
- out:
-    error_propagate(errp, local_err);
-}
-
 static void nvdimm_init(Object *obj)
 {
     object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
                         nvdimm_get_label_size, nvdimm_set_label_size, NULL,
                         NULL, NULL);
-    object_property_add_bool(obj, NVDIMM_UNARMED_PROP,
-                             nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
 }
 
 static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
@@ -166,13 +141,20 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
     memory_region_set_dirty(mr, backend_offset, size);
 }
 
+static Property nvdimm_properties[] = {
+    DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
+    dc->props = nvdimm_properties;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
-- 
2.17.1

[Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a static property

[David Hildenbrand]

We don't allow to modify it after realization. So we can simply turn
it into a static property. The value is now validated during realize().

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

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7260c9c6b1..d3e8a5bcbb 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -27,50 +27,6 @@
 #include "qapi/visitor.h"
 #include "hw/mem/nvdimm.h"
 
-static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-    uint64_t value = nvdimm->label_size;
-
-    visit_type_size(v, name, &value, errp);
-}
-
-static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-    Error *local_err = NULL;
-    uint64_t value;
-
-    if (memory_region_size(&nvdimm->nvdimm_mr)) {
-        error_setg(&local_err, "cannot change property value");
-        goto out;
-    }
-
-    visit_type_size(v, name, &value, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    if (value < MIN_NAMESPACE_LABEL_SIZE) {
-        error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
-                   " at least 0x%lx", object_get_typename(obj),
-                   name, value, MIN_NAMESPACE_LABEL_SIZE);
-        goto out;
-    }
-
-    nvdimm->label_size = value;
-out:
-    error_propagate(errp, local_err);
-}
-
-static void nvdimm_init(Object *obj)
-{
-    object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
-                        nvdimm_get_label_size, nvdimm_set_label_size, NULL,
-                        NULL, NULL);
-}
-
 static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 {
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
@@ -86,6 +42,13 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
 
     align = memory_region_get_alignment(mr);
 
+    if (nvdimm->label_size && nvdimm->label_size < MIN_NAMESPACE_LABEL_SIZE) {
+        error_setg(errp, "the label-size (0x%" PRIx64 ") has to be "
+                   "either 0 or at least 0x%lx",
+                   nvdimm->label_size, MIN_NAMESPACE_LABEL_SIZE);
+        return;
+    }
+
     pmem_size = size - nvdimm->label_size;
     nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
     pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
@@ -143,6 +106,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
 
 static Property nvdimm_properties[] = {
     DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+    DEFINE_PROP_UINT64(NVDIMM_LABEL_SIZE_PROP, NVDIMMDevice, label_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -166,7 +130,6 @@ static TypeInfo nvdimm_info = {
     .class_size    = sizeof(NVDIMMClass),
     .class_init    = nvdimm_class_init,
     .instance_size = sizeof(NVDIMMDevice),
-    .instance_init = nvdimm_init,
 };
 
 static void nvdimm_register_types(void)
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a static property

[Igor Mammedov]

On Fri, 15 Jun 2018 13:24:58 +0200
David Hildenbrand <david@redhat.com> wrote:

> We don't allow to modify it after realization. So we can simply turn
> it into a static property. The value is now validated during realize().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c | 53 ++++++++-----------------------------------------
>  1 file changed, 8 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7260c9c6b1..d3e8a5bcbb 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -27,50 +27,6 @@
>  #include "qapi/visitor.h"
>  #include "hw/mem/nvdimm.h"
>  
> -static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
> -    uint64_t value = nvdimm->label_size;
> -
> -    visit_type_size(v, name, &value, errp);
> -}
> -
> -static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
> -    Error *local_err = NULL;
> -    uint64_t value;
> -
> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
> -        error_setg(&local_err, "cannot change property value");
> -        goto out;
> -    }
> -
> -    visit_type_size(v, name, &value, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
> -        error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
> -                   " at least 0x%lx", object_get_typename(obj),
> -                   name, value, MIN_NAMESPACE_LABEL_SIZE);
> -        goto out;
> -    }
doesn't seem right,
property setter should throw out error at the time it's set if possible.

I'd keep this one check where it is and property dynamic.

and fix only access to uninitialized "if (memory_region_size(&nvdimm->nvdimm_mr)) {"

> -
> -    nvdimm->label_size = value;
> -out:
> -    error_propagate(errp, local_err);
> -}
> -
> -static void nvdimm_init(Object *obj)
> -{
> -    object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
> -                        nvdimm_get_label_size, nvdimm_set_label_size, NULL,
> -                        NULL, NULL);
> -}
> -
>  static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
> @@ -86,6 +42,13 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>  
>      align = memory_region_get_alignment(mr);
>  
> +    if (nvdimm->label_size && nvdimm->label_size < MIN_NAMESPACE_LABEL_SIZE) {
> +        error_setg(errp, "the label-size (0x%" PRIx64 ") has to be "
> +                   "either 0 or at least 0x%lx",
> +                   nvdimm->label_size, MIN_NAMESPACE_LABEL_SIZE);
> +        return;
> +    }
> +
>      pmem_size = size - nvdimm->label_size;
>      nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
>      pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
> @@ -143,6 +106,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>  
>  static Property nvdimm_properties[] = {
>      DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> +    DEFINE_PROP_UINT64(NVDIMM_LABEL_SIZE_PROP, NVDIMMDevice, label_size, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -166,7 +130,6 @@ static TypeInfo nvdimm_info = {
>      .class_size    = sizeof(NVDIMMClass),
>      .class_init    = nvdimm_class_init,
>      .instance_size = sizeof(NVDIMMDevice),
> -    .instance_init = nvdimm_init,
>  };
>  
>  static void nvdimm_register_types(void)

Re: [Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a static property

[David Hildenbrand]

On 15.06.2018 14:53, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 13:24:58 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We don't allow to modify it after realization. So we can simply turn
>> it into a static property. The value is now validated during realize().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/nvdimm.c | 53 ++++++++-----------------------------------------
>>  1 file changed, 8 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>> index 7260c9c6b1..d3e8a5bcbb 100644
>> --- a/hw/mem/nvdimm.c
>> +++ b/hw/mem/nvdimm.c
>> @@ -27,50 +27,6 @@
>>  #include "qapi/visitor.h"
>>  #include "hw/mem/nvdimm.h"
>>  
>> -static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name,
>> -                                  void *opaque, Error **errp)
>> -{
>> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
>> -    uint64_t value = nvdimm->label_size;
>> -
>> -    visit_type_size(v, name, &value, errp);
>> -}
>> -
>> -static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>> -                                  void *opaque, Error **errp)
>> -{
>> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
>> -    Error *local_err = NULL;
>> -    uint64_t value;
>> -
>> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
>> -        error_setg(&local_err, "cannot change property value");
>> -        goto out;
>> -    }
>> -
>> -    visit_type_size(v, name, &value, &local_err);
>> -    if (local_err) {
>> -        goto out;
>> -    }
>> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
>> -        error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
>> -                   " at least 0x%lx", object_get_typename(obj),
>> -                   name, value, MIN_NAMESPACE_LABEL_SIZE);
>> -        goto out;
>> -    }
> doesn't seem right,
> property setter should throw out error at the time it's set if possible.
> 
> I'd keep this one check where it is and property dynamic.
> 
> and fix only access to uninitialized "if (memory_region_size(&nvdimm->nvdimm_mr)) {"

Do we really want to simulate a static property with 25+ LOC?

But if you insist, to get this stuff of my list, I will turn the

if (memory_region_size(&nvdimm->nvdimm_mr)) {
into a
if (dev->realized)

And allow setting the property to 0, too (which is also broken right now).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 10/12] nvdimm: convert "label-size" into a static property

[David Hildenbrand]

On 15.06.2018 15:30, David Hildenbrand wrote:
> On 15.06.2018 14:53, Igor Mammedov wrote:
>> On Fri, 15 Jun 2018 13:24:58 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> We don't allow to modify it after realization. So we can simply turn
>>> it into a static property. The value is now validated during realize().
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/mem/nvdimm.c | 53 ++++++++-----------------------------------------
>>>  1 file changed, 8 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>>> index 7260c9c6b1..d3e8a5bcbb 100644
>>> --- a/hw/mem/nvdimm.c
>>> +++ b/hw/mem/nvdimm.c
>>> @@ -27,50 +27,6 @@
>>>  #include "qapi/visitor.h"
>>>  #include "hw/mem/nvdimm.h"
>>>  
>>> -static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name,
>>> -                                  void *opaque, Error **errp)
>>> -{
>>> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
>>> -    uint64_t value = nvdimm->label_size;
>>> -
>>> -    visit_type_size(v, name, &value, errp);
>>> -}
>>> -
>>> -static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>>> -                                  void *opaque, Error **errp)
>>> -{
>>> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
>>> -    Error *local_err = NULL;
>>> -    uint64_t value;
>>> -
>>> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
>>> -        error_setg(&local_err, "cannot change property value");
>>> -        goto out;
>>> -    }
>>> -
>>> -    visit_type_size(v, name, &value, &local_err);
>>> -    if (local_err) {
>>> -        goto out;
>>> -    }
>>> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
>>> -        error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
>>> -                   " at least 0x%lx", object_get_typename(obj),
>>> -                   name, value, MIN_NAMESPACE_LABEL_SIZE);
>>> -        goto out;
>>> -    }
>> doesn't seem right,
>> property setter should throw out error at the time it's set if possible.
>>
>> I'd keep this one check where it is and property dynamic.
>>
>> and fix only access to uninitialized "if (memory_region_size(&nvdimm->nvdimm_mr)) {"
> 
> Do we really want to simulate a static property with 25+ LOC?
> 
> But if you insist, to get this stuff of my list, I will turn the
> 
> if (memory_region_size(&nvdimm->nvdimm_mr)) {
> into a
> if (dev->realized)

or rather a pointer check as you said.

> 
> And allow setting the property to 0, too (which is also broken right now).
> 


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v2 11/12] nvdimm: make get_memory_region() perform checks and initialization

[David Hildenbrand]

We might get a call to get_memory_region() before the device has been
realized. We should return a consistent value, as the return value
will e.g. later on be used in the pre_plug handler.

To avoid duplicating too much code, factor the initialization and checks
out into a helper function.

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

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index d3e8a5bcbb..9048ce0b84 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -27,20 +27,19 @@
 #include "qapi/visitor.h"
 #include "hw/mem/nvdimm.h"
 
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
 {
-    NVDIMMDevice *nvdimm = NVDIMM(dimm);
-
-    return &nvdimm->nvdimm_mr;
-}
+    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
+    uint64_t align, pmem_size, size;
+    MemoryRegion *mr;
 
-static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
-{
-    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
-    NVDIMMDevice *nvdimm = NVDIMM(dimm);
-    uint64_t align, pmem_size, size = memory_region_size(mr);
+    if (!dimm->hostmem) {
+        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+        return;
+    }
 
-    align = memory_region_get_alignment(mr);
+    g_assert(!memory_region_size(&nvdimm->nvdimm_mr));
+    mr = host_memory_backend_get_memory(dimm->hostmem);
 
     if (nvdimm->label_size && nvdimm->label_size < MIN_NAMESPACE_LABEL_SIZE) {
         error_setg(errp, "the label-size (0x%" PRIx64 ") has to be "
@@ -49,6 +48,9 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
         return;
     }
 
+    align = memory_region_get_alignment(mr);
+    size = memory_region_size(mr);
+
     pmem_size = size - nvdimm->label_size;
     nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
     pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
@@ -70,6 +72,31 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
     nvdimm->nvdimm_mr.align = align;
 }
 
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+    Error *local_err = NULL;
+
+    if (!memory_region_size(&nvdimm->nvdimm_mr)) {
+        nvdimm_prepare_memory_region(nvdimm, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+    return &nvdimm->nvdimm_mr;
+}
+
+static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+
+    if (!memory_region_size(&nvdimm->nvdimm_mr)) {
+        nvdimm_prepare_memory_region(nvdimm, errp);
+    }
+}
+
 /*
  * the caller should check the input parameters before calling
  * label read/write functions.
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 11/12] nvdimm: make get_memory_region() perform checks and initialization

[Igor Mammedov]

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

> We might get a call to get_memory_region() before the device has been
> realized. We should return a consistent value, as the return value
> will e.g. later on be used in the pre_plug handler.
> 
> To avoid duplicating too much code, factor the initialization and checks
> out into a helper function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index d3e8a5bcbb..9048ce0b84 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -27,20 +27,19 @@
>  #include "qapi/visitor.h"
>  #include "hw/mem/nvdimm.h"
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
>  {
> -    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> -
> -    return &nvdimm->nvdimm_mr;
> -}
> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
> +    uint64_t align, pmem_size, size;
> +    MemoryRegion *mr;
>  
> -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> -{
> -    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
> -    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> -    uint64_t align, pmem_size, size = memory_region_size(mr);
> +    if (!dimm->hostmem) {
> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return;
> +    }
>  
> -    align = memory_region_get_alignment(mr);
> +    g_assert(!memory_region_size(&nvdimm->nvdimm_mr));
> +    mr = host_memory_backend_get_memory(dimm->hostmem);
>  
>      if (nvdimm->label_size && nvdimm->label_size < MIN_NAMESPACE_LABEL_SIZE) {
>          error_setg(errp, "the label-size (0x%" PRIx64 ") has to be "
> @@ -49,6 +48,9 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>          return;
>      }
>  
> +    align = memory_region_get_alignment(mr);
> +    size = memory_region_size(mr);
> +
>      pmem_size = size - nvdimm->label_size;
>      nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
>      pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
> @@ -70,6 +72,31 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>      nvdimm->nvdimm_mr.align = align;
>  }
>  
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> +    Error *local_err = NULL;
> +
> +    if (!memory_region_size(&nvdimm->nvdimm_mr)) {
ditto as in 10/12, this check works by accident (because memory is zero initialized,
hence memory_region_size() returns 0) and not by design.

one way to fix it, could be to make nvdimm->nvdimm_mr a pointer and dynamically
allocate it and use NULL check to see if it's initialized.
one would need to add finalize to free it on nvdimm destruction though.

> +        nvdimm_prepare_memory_region(nvdimm, &local_err);
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +    return &nvdimm->nvdimm_mr;
> +}
> +
> +static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> +
> +    if (!memory_region_size(&nvdimm->nvdimm_mr)) {
ditto

> +        nvdimm_prepare_memory_region(nvdimm, errp);
> +    }
> +}
> +
>  /*
>   * the caller should check the input parameters before calling
>   * label read/write functions.

[Qemu-devel] [PATCH v2 12/12] pc-dimm: get_memory_region() will not fail after realize

[David Hildenbrand]

Let's try to reduce error handling a bit. In the plug/unplug case, the
device was realized and therefore we can assume that getting access to
the memory region will not fail.

For get_vmstate_memory_region() this is already handled that way.
Document both cases.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2db032a6df..f310040351 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,15 +1706,10 @@ static void pc_memory_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, &error_abort);
     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/pc-dimm.c b/hw/mem/pc-dimm.c
index 4ff39b59ef..65843bc52a 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -37,15 +37,10 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
                                                               &error_abort);
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     Error *local_err = NULL;
-    MemoryRegion *mr;
     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) {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3e5320020f..6934abc21e 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, &error_abort);
     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);
 
@@ -3340,16 +3336,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, &error_abort);
     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 5679a80465..26ebb7d5e9 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -62,9 +62,11 @@ typedef struct PCDIMMDevice {
  * @realize: called after common dimm is realized so that the dimm based
  * devices get the chance to do specified operations.
  * @get_memory_region: returns #MemoryRegion associated with @dimm which
- * is directly mapped into the physical address space of guest.
+ * is directly mapped into the physical address space of guest. Will not
+ * fail after the device was realized.
  * @get_vmstate_memory_region: returns #MemoryRegion which indicates the
- * memory of @dimm should be kept during live migration.
+ * memory of @dimm should be kept during live migration. Will not fail
+ * after the device was realized.
  */
 typedef struct PCDIMMDeviceClass {
     /* private */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 12/12] pc-dimm: get_memory_region() will not fail after realize

[Igor Mammedov]

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

> Let's try to reduce error handling a bit. In the plug/unplug case, the
> device was realized and therefore we can assume that getting access to
> the memory region will not fail.
> 
> For get_vmstate_memory_region() this is already handled that way.
> Document both cases.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  hw/i386/pc.c             |  7 +------
>  hw/mem/pc-dimm.c         |  7 +------
>  hw/ppc/spapr.c           | 12 ++----------
>  include/hw/mem/pc-dimm.h |  6 ++++--
>  4 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2db032a6df..f310040351 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1706,15 +1706,10 @@ static void pc_memory_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, &error_abort);
>      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/pc-dimm.c b/hw/mem/pc-dimm.c
> index 4ff39b59ef..65843bc52a 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -37,15 +37,10 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
>                                                                &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
>      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) {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3e5320020f..6934abc21e 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, &error_abort);
>      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);
>  
> @@ -3340,16 +3336,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, &error_abort);
>      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 5679a80465..26ebb7d5e9 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -62,9 +62,11 @@ typedef struct PCDIMMDevice {
>   * @realize: called after common dimm is realized so that the dimm based
>   * devices get the chance to do specified operations.
>   * @get_memory_region: returns #MemoryRegion associated with @dimm which
> - * is directly mapped into the physical address space of guest.
> + * is directly mapped into the physical address space of guest. Will not
> + * fail after the device was realized.
>   * @get_vmstate_memory_region: returns #MemoryRegion which indicates the
> - * memory of @dimm should be kept during live migration.
> + * memory of @dimm should be kept during live migration. Will not fail
> + * after the device was realized.
>   */
>  typedef struct PCDIMMDeviceClass {
>      /* private */