[PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[Greg Kurz]

Hi,

This is a followup to a previous cleanup for the sPAPR code:

https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

The last two patches had to be dropped because they were wrongly assuming
that object_property_get_uint() returning zero meant failure. This led to
a discussion in which arose a consensus that most of the time (not to say
always) object property getters should never fail actually, ie. failure
is very likely the result of a programming error and QEMU should abort.

This series aims at demonstrating a revelant case I've found while auditing
object property getters (this is patch 4 that I've isolated from a huge
50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
from PC DIMMs. They expect to get some properties from the DIMM object,
which happens to be set by default at the PC DIMM class level. It thus
doesn't make sense to pass an error object and propagate it when getting
them since this would lure the user into thinking they did something wrong.

Some preliminary cleanup is done on the way, especially dropping an unused
@errp argument of pc_dimm_plug(). This affects several platforms other than
sPAPR but I guess the patch is trivial enough to go through David's tree
if it gets acks from the relevant maintainers.

---

Greg Kurz (5):
      pc-dimm: Drop @errp argument of pc_dimm_plug()
      spapr: Use appropriate getter for PC_DIMM_ADDR_PROP
      spapr: Use appropriate getter for PC_DIMM_SLOT_PROP
      spapr: Pass &error_abort when getting some PC DIMM properties
      spapr: Simplify error handling in spapr_memory_plug()


 hw/arm/virt.c                 |    9 +-------
 hw/i386/pc.c                  |    8 +------
 hw/mem/pc-dimm.c              |    2 +-
 hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------
 hw/ppc/spapr_nvdimm.c         |    5 +++-
 include/hw/mem/pc-dimm.h      |    2 +-
 include/hw/ppc/spapr_nvdimm.h |    2 +-
 7 files changed, 25 insertions(+), 51 deletions(-)

--
Greg

[PATCH 1/5] pc-dimm: Drop @errp argument of pc_dimm_plug()

[Greg Kurz]

pc_dimm_plug() doesn't use it. It only aborts on error.

Drop @errp and adapt the callers accordingly.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/arm/virt.c            |    9 +--------
 hw/i386/pc.c             |    8 +-------
 hw/mem/pc-dimm.c         |    2 +-
 hw/ppc/spapr.c           |    5 +----
 include/hw/mem/pc-dimm.h |    2 +-
 5 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e465a988d683..27dbeb549ef1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2261,12 +2261,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
     MachineState *ms = MACHINE(hotplug_dev);
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
-    Error *local_err = NULL;
 
-    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
-    if (local_err) {
-        goto out;
-    }
+    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms));
 
     if (is_nvdimm) {
         nvdimm_plug(ms->nvdimms_state);
@@ -2274,9 +2270,6 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 
     hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
                          dev, &error_abort);
-
-out:
-    error_propagate(errp, local_err);
 }
 
 static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e87be5d29a01..38b1be78e707 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1265,24 +1265,18 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 static void pc_memory_plug(HotplugHandler *hotplug_dev,
                            DeviceState *dev, Error **errp)
 {
-    Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
     MachineState *ms = MACHINE(hotplug_dev);
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
-    if (local_err) {
-        goto out;
-    }
+    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms));
 
     if (is_nvdimm) {
         nvdimm_plug(ms->nvdimms_state);
     }
 
     hotplug_handler_plug(x86ms->acpi_dev, dev, &error_abort);
-out:
-    error_propagate(errp, local_err);
 }
 
 static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index c30351070bb8..2ffc986734df 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -64,7 +64,7 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
                            errp);
 }
 
-void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp)
+void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
 {
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ee716a12af73..4edd31b86915 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3438,10 +3438,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
 
-    pc_dimm_plug(dimm, MACHINE(ms), &local_err);
-    if (local_err) {
-        goto out;
-    }
+    pc_dimm_plug(dimm, MACHINE(ms));
 
     if (!is_nvdimm) {
         addr = object_property_get_uint(OBJECT(dimm),
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index aec9527fdd96..3d3db82641f8 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -72,6 +72,6 @@ struct PCDIMMDeviceClass {
 
 void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
                       const uint64_t *legacy_align, Error **errp);
-void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp);
+void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine);
 void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine);
 #endif

Re: [PATCH 1/5] pc-dimm: Drop @errp argument of pc_dimm_plug()

[Vladimir Sementsov-Ogievskiy]

19.10.2020 11:48, Greg Kurz wrote:
> pc_dimm_plug() doesn't use it. It only aborts on error.
> 
> Drop @errp and adapt the callers accordingly.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 1/5] pc-dimm: Drop @errp argument of pc_dimm_plug()

[David Gibson]

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

On Mon, Oct 19, 2020 at 10:48:04AM +0200, Greg Kurz wrote:
> pc_dimm_plug() doesn't use it. It only aborts on error.
> 
> Drop @errp and adapt the callers accordingly.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

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

> ---
>  hw/arm/virt.c            |    9 +--------
>  hw/i386/pc.c             |    8 +-------
>  hw/mem/pc-dimm.c         |    2 +-
>  hw/ppc/spapr.c           |    5 +----
>  include/hw/mem/pc-dimm.h |    2 +-
>  5 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e465a988d683..27dbeb549ef1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2261,12 +2261,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>      MachineState *ms = MACHINE(hotplug_dev);
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> -    Error *local_err = NULL;
>  
> -    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms));
>  
>      if (is_nvdimm) {
>          nvdimm_plug(ms->nvdimms_state);
> @@ -2274,9 +2270,6 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>  
>      hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
>                           dev, &error_abort);
> -
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e87be5d29a01..38b1be78e707 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1265,24 +1265,18 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
>                             DeviceState *dev, Error **errp)
>  {
> -    Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
>      MachineState *ms = MACHINE(hotplug_dev);
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms));
>  
>      if (is_nvdimm) {
>          nvdimm_plug(ms->nvdimms_state);
>      }
>  
>      hotplug_handler_plug(x86ms->acpi_dev, dev, &error_abort);
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index c30351070bb8..2ffc986734df 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -64,7 +64,7 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>                             errp);
>  }
>  
> -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp)
> +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>  {
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ee716a12af73..4edd31b86915 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3438,10 +3438,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>  
> -    pc_dimm_plug(dimm, MACHINE(ms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(dimm, MACHINE(ms));
>  
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index aec9527fdd96..3d3db82641f8 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -72,6 +72,6 @@ struct PCDIMMDeviceClass {
>  
>  void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>                        const uint64_t *legacy_align, Error **errp);
> -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp);
> +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine);
>  void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine);
>  #endif
> 
> 

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

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

Re: [PATCH 1/5] pc-dimm: Drop @errp argument of pc_dimm_plug()

[Igor Mammedov]

On Mon, 19 Oct 2020 10:48:04 +0200
Greg Kurz <groug@kaod.org> wrote:

> pc_dimm_plug() doesn't use it. It only aborts on error.
> 
> Drop @errp and adapt the callers accordingly.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

arguably the same should be done in spapr.

> ---
>  hw/arm/virt.c            |    9 +--------
>  hw/i386/pc.c             |    8 +-------
>  hw/mem/pc-dimm.c         |    2 +-
>  hw/ppc/spapr.c           |    5 +----
>  include/hw/mem/pc-dimm.h |    2 +-
>  5 files changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e465a988d683..27dbeb549ef1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2261,12 +2261,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>      MachineState *ms = MACHINE(hotplug_dev);
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> -    Error *local_err = NULL;
>  
> -    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms));
>  
>      if (is_nvdimm) {
>          nvdimm_plug(ms->nvdimms_state);
> @@ -2274,9 +2270,6 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>  
>      hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
>                           dev, &error_abort);
> -
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e87be5d29a01..38b1be78e707 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1265,24 +1265,18 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
>                             DeviceState *dev, Error **errp)
>  {
> -    Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
>      MachineState *ms = MACHINE(hotplug_dev);
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms));
>  
>      if (is_nvdimm) {
>          nvdimm_plug(ms->nvdimms_state);
>      }
>  
>      hotplug_handler_plug(x86ms->acpi_dev, dev, &error_abort);
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index c30351070bb8..2ffc986734df 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -64,7 +64,7 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>                             errp);
>  }
>  
> -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp)
> +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>  {
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ee716a12af73..4edd31b86915 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3438,10 +3438,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
>  
> -    pc_dimm_plug(dimm, MACHINE(ms), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    pc_dimm_plug(dimm, MACHINE(ms));
>  
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index aec9527fdd96..3d3db82641f8 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -72,6 +72,6 @@ struct PCDIMMDeviceClass {
>  
>  void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>                        const uint64_t *legacy_align, Error **errp);
> -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp);
> +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine);
>  void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine);
>  #endif
> 
> 
> 

Re: [PATCH 1/5] pc-dimm: Drop @errp argument of pc_dimm_plug()

[Greg Kurz]

On Fri, 23 Oct 2020 21:19:19 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 19 Oct 2020 10:48:04 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > pc_dimm_plug() doesn't use it. It only aborts on error.
> > 
> > Drop @errp and adapt the callers accordingly.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> arguably the same should be done in spapr.
> 

As explained in another mail, we have to keep spapr_drc_attach()
at plug time and this can legitimately fail if an unplug operation
is pending for the device. We certainly prefer to report this as
an error rather than aborting.

> > ---
> >  hw/arm/virt.c            |    9 +--------
> >  hw/i386/pc.c             |    8 +-------
> >  hw/mem/pc-dimm.c         |    2 +-
> >  hw/ppc/spapr.c           |    5 +----
> >  include/hw/mem/pc-dimm.h |    2 +-
> >  5 files changed, 5 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index e465a988d683..27dbeb549ef1 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2261,12 +2261,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >      MachineState *ms = MACHINE(hotplug_dev);
> >      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> > -    Error *local_err = NULL;
> >  
> > -    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > +    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms));
> >  
> >      if (is_nvdimm) {
> >          nvdimm_plug(ms->nvdimms_state);
> > @@ -2274,9 +2270,6 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >  
> >      hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> >                           dev, &error_abort);
> > -
> > -out:
> > -    error_propagate(errp, local_err);
> >  }
> >  
> >  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e87be5d29a01..38b1be78e707 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1265,24 +1265,18 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> >                             DeviceState *dev, Error **errp)
> >  {
> > -    Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >      X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
> >      MachineState *ms = MACHINE(hotplug_dev);
> >      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >  
> > -    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms), &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > +    pc_dimm_plug(PC_DIMM(dev), MACHINE(pcms));
> >  
> >      if (is_nvdimm) {
> >          nvdimm_plug(ms->nvdimms_state);
> >      }
> >  
> >      hotplug_handler_plug(x86ms->acpi_dev, dev, &error_abort);
> > -out:
> > -    error_propagate(errp, local_err);
> >  }
> >  
> >  static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index c30351070bb8..2ffc986734df 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -64,7 +64,7 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
> >                             errp);
> >  }
> >  
> > -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp)
> > +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
> >  {
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ee716a12af73..4edd31b86915 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3438,10 +3438,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  
> >      size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> >  
> > -    pc_dimm_plug(dimm, MACHINE(ms), &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > +    pc_dimm_plug(dimm, MACHINE(ms));
> >  
> >      if (!is_nvdimm) {
> >          addr = object_property_get_uint(OBJECT(dimm),
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index aec9527fdd96..3d3db82641f8 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -72,6 +72,6 @@ struct PCDIMMDeviceClass {
> >  
> >  void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
> >                        const uint64_t *legacy_align, Error **errp);
> > -void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp);
> > +void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine);
> >  void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine);
> >  #endif
> > 
> > 
> > 
> 
> 

[PATCH 2/5] spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

[Greg Kurz]

The PC_DIMM_ADDR_PROP property is defined as:

    DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),

Use object_property_get_uint() instead of object_property_get_int().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4edd31b86915..115fc52e3b06 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3572,8 +3572,8 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
     uint64_t addr_start, addr;
     int i;
 
-    addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
-                                         &error_abort);
+    addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
+                                          &error_abort);
 
     addr = addr_start;
     for (i = 0; i < nr_lmbs; i++) {

Re: [PATCH 2/5] spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

[Philippe Mathieu-Daudé]

On 10/19/20 10:48 AM, Greg Kurz wrote:
> The PC_DIMM_ADDR_PROP property is defined as:
> 
>      DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> 
> Use object_property_get_uint() instead of object_property_get_int().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/ppc/spapr.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4edd31b86915..115fc52e3b06 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3572,8 +3572,8 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
>       uint64_t addr_start, addr;
>       int i;
>   
> -    addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> -                                         &error_abort);
> +    addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> +                                          &error_abort);
>   
>       addr = addr_start;
>       for (i = 0; i < nr_lmbs; i++) {
> 
> 
> 

Re: [PATCH 2/5] spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

[David Gibson]

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

On Mon, Oct 19, 2020 at 10:48:16AM +0200, Greg Kurz wrote:
> The PC_DIMM_ADDR_PROP property is defined as:
> 
>     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> 
> Use object_property_get_uint() instead of object_property_get_int().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
>  hw/ppc/spapr.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4edd31b86915..115fc52e3b06 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3572,8 +3572,8 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
>      uint64_t addr_start, addr;
>      int i;
>  
> -    addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> -                                         &error_abort);
> +    addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> +                                          &error_abort);
>  
>      addr = addr_start;
>      for (i = 0; i < nr_lmbs; i++) {
> 
> 

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

[PATCH 3/5] spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

[Greg Kurz]

The PC_DIMM_SLOT_PROP property is defined as:

    DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
                      PC_DIMM_UNASSIGNED_SLOT),

Use object_property_get_int() instead of object_property_get_uint().
Since spapr_memory_plug() only gets called if pc_dimm_pre_plug()
succeeded, we expect to have a valid >= 0 slot number, either because
the user passed a valid slot number or because pc_dimm_get_free_slot()
picked one up for us.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 115fc52e3b06..1b173861152f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     Error *local_err = NULL;
     SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
-    uint64_t size, addr, slot;
+    uint64_t size, addr;
+    int64_t slot;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
     size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
@@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                        spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
                        &local_err);
     } else {
-        slot = object_property_get_uint(OBJECT(dimm),
-                                        PC_DIMM_SLOT_PROP, &local_err);
+        slot = object_property_get_int(OBJECT(dimm),
+                                       PC_DIMM_SLOT_PROP, &local_err);
         if (local_err) {
             goto out_unplug;
         }
+        /* We should have valid slot number at this point */
+        g_assert(slot >= 0);
         spapr_add_nvdimm(dev, slot, &local_err);
     }
 

Re: [PATCH 3/5] spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

[Philippe Mathieu-Daudé]

On 10/19/20 10:48 AM, Greg Kurz wrote:
> The PC_DIMM_SLOT_PROP property is defined as:
> 
>      DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
>                        PC_DIMM_UNASSIGNED_SLOT),

Worth adding:

       #define PC_DIMM_UNASSIGNED_SLOT -1

for better understanding why it is signed.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Use object_property_get_int() instead of object_property_get_uint().
> Since spapr_memory_plug() only gets called if pc_dimm_pre_plug()
> succeeded, we expect to have a valid >= 0 slot number, either because
> the user passed a valid slot number or because pc_dimm_get_free_slot()
> picked one up for us.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/spapr.c |    9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 115fc52e3b06..1b173861152f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       Error *local_err = NULL;
>       SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>       PCDIMMDevice *dimm = PC_DIMM(dev);
> -    uint64_t size, addr, slot;
> +    uint64_t size, addr;
> +    int64_t slot;
>       bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>   
>       size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> @@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                          spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
>                          &local_err);
>       } else {
> -        slot = object_property_get_uint(OBJECT(dimm),
> -                                        PC_DIMM_SLOT_PROP, &local_err);
> +        slot = object_property_get_int(OBJECT(dimm),
> +                                       PC_DIMM_SLOT_PROP, &local_err);
>           if (local_err) {
>               goto out_unplug;
>           }
> +        /* We should have valid slot number at this point */
> +        g_assert(slot >= 0);
>           spapr_add_nvdimm(dev, slot, &local_err);
>       }
>   
> 
> 
> 

Re: [PATCH 3/5] spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

[David Gibson]

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

On Mon, Oct 19, 2020 at 10:48:27AM +0200, Greg Kurz wrote:
> The PC_DIMM_SLOT_PROP property is defined as:
> 
>     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
>                       PC_DIMM_UNASSIGNED_SLOT),
> 
> Use object_property_get_int() instead of object_property_get_uint().
> Since spapr_memory_plug() only gets called if pc_dimm_pre_plug()
> succeeded, we expect to have a valid >= 0 slot number, either because
> the user passed a valid slot number or because pc_dimm_get_free_slot()
> picked one up for us.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
>  hw/ppc/spapr.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 115fc52e3b06..1b173861152f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      Error *local_err = NULL;
>      SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> -    uint64_t size, addr, slot;
> +    uint64_t size, addr;
> +    int64_t slot;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
>      size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
> @@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
>                         &local_err);
>      } else {
> -        slot = object_property_get_uint(OBJECT(dimm),
> -                                        PC_DIMM_SLOT_PROP, &local_err);
> +        slot = object_property_get_int(OBJECT(dimm),
> +                                       PC_DIMM_SLOT_PROP, &local_err);
>          if (local_err) {
>              goto out_unplug;
>          }
> +        /* We should have valid slot number at this point */
> +        g_assert(slot >= 0);
>          spapr_add_nvdimm(dev, slot, &local_err);
>      }
>  
> 
> 

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

[PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Greg Kurz]

Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
default property list of the PC DIMM device class:

    DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),

    DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
                      PC_DIMM_UNASSIGNED_SLOT),

They should thus be always gettable for both PC DIMMs and NVDIMMs.
An error in getting them can only be the result of a programming
error. It doesn't make much sense to propagate the error in this
case. Abort instead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |   17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1b173861152f..62f217a6b914 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     if (!is_nvdimm) {
         addr = object_property_get_uint(OBJECT(dimm),
-                                        PC_DIMM_ADDR_PROP, &local_err);
-        if (local_err) {
-            goto out_unplug;
-        }
+                                        PC_DIMM_ADDR_PROP, &error_abort);
         spapr_add_lmbs(dev, addr, size,
                        spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
                        &local_err);
     } else {
         slot = object_property_get_int(OBJECT(dimm),
-                                       PC_DIMM_SLOT_PROP, &local_err);
-        if (local_err) {
-            goto out_unplug;
-        }
+                                       PC_DIMM_SLOT_PROP, &error_abort);
         /* We should have valid slot number at this point */
         g_assert(slot >= 0);
         spapr_add_nvdimm(dev, slot, &local_err);
@@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
-    Error *local_err = NULL;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
@@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
 
     addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
-                                         &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+                                          &error_abort);
 
     /*
      * An existing pending dimm state for this DIMM means that there is an

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Igor Mammedov]

On Mon, 19 Oct 2020 10:48:41 +0200
Greg Kurz <groug@kaod.org> wrote:

> Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> default property list of the PC DIMM device class:
> 
>     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> 
>     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
>                       PC_DIMM_UNASSIGNED_SLOT),
> 
> They should thus be always gettable for both PC DIMMs and NVDIMMs.
> An error in getting them can only be the result of a programming
> error. It doesn't make much sense to propagate the error in this
> case. Abort instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

TODO for future,
get rid of local_err in spapr_memory_plug() altogether, it should not fail.
it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.

that will clear up (a bit) road for dropping errp in spapr_memory_plug()
> ---
>  hw/ppc/spapr.c |   17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b173861152f..62f217a6b914 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
> -                                        PC_DIMM_ADDR_PROP, &local_err);
> -        if (local_err) {
> -            goto out_unplug;
> -        }
> +                                        PC_DIMM_ADDR_PROP, &error_abort);
>          spapr_add_lmbs(dev, addr, size,
>                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
>                         &local_err);
>      } else {
>          slot = object_property_get_int(OBJECT(dimm),
> -                                       PC_DIMM_SLOT_PROP, &local_err);
> -        if (local_err) {
> -            goto out_unplug;
> -        }
> +                                       PC_DIMM_SLOT_PROP, &error_abort);
>          /* We should have valid slot number at this point */
>          g_assert(slot >= 0);
>          spapr_add_nvdimm(dev, slot, &local_err);
> @@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> -    Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
> @@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>  
>      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> -                                         &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +                                          &error_abort);
>  
>      /*
>       * An existing pending dimm state for this DIMM means that there is an
> 
> 
> 

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Greg Kurz]

On Fri, 23 Oct 2020 21:15:09 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 19 Oct 2020 10:48:41 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> > default property list of the PC DIMM device class:
> > 
> >     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> > 
> >     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
> >                       PC_DIMM_UNASSIGNED_SLOT),
> > 
> > They should thus be always gettable for both PC DIMMs and NVDIMMs.
> > An error in getting them can only be the result of a programming
> > error. It doesn't make much sense to propagate the error in this
> > case. Abort instead.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> TODO for future,
> get rid of local_err in spapr_memory_plug() altogether, it should not fail.
> it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.
> 
> that will clear up (a bit) road for dropping errp in spapr_memory_plug()

Igor,

I could find time to look a bit into attaching DRCs at pre-plug and I
think this isn't possible. The problem is that there doesn't seem to be
a reverse operation for pre-plug. If realize fails for the DIMM device,
spapr_drc_detach() wouldn't be called, which would be wrong.

Am I missing something ?

> > ---
> >  hw/ppc/spapr.c |   17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 1b173861152f..62f217a6b914 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  
> >      if (!is_nvdimm) {
> >          addr = object_property_get_uint(OBJECT(dimm),
> > -                                        PC_DIMM_ADDR_PROP, &local_err);
> > -        if (local_err) {
> > -            goto out_unplug;
> > -        }
> > +                                        PC_DIMM_ADDR_PROP, &error_abort);
> >          spapr_add_lmbs(dev, addr, size,
> >                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> >                         &local_err);
> >      } else {
> >          slot = object_property_get_int(OBJECT(dimm),
> > -                                       PC_DIMM_SLOT_PROP, &local_err);
> > -        if (local_err) {
> > -            goto out_unplug;
> > -        }
> > +                                       PC_DIMM_SLOT_PROP, &error_abort);
> >          /* We should have valid slot number at this point */
> >          g_assert(slot >= 0);
> >          spapr_add_nvdimm(dev, slot, &local_err);
> > @@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> >                                          DeviceState *dev, Error **errp)
> >  {
> >      SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > -    Error *local_err = NULL;
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      uint32_t nr_lmbs;
> >      uint64_t size, addr_start, addr;
> > @@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> >      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> >  
> >      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> > -                                         &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > +                                          &error_abort);
> >  
> >      /*
> >       * An existing pending dimm state for this DIMM means that there is an
> > 
> > 
> > 
> 

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Igor Mammedov]

On Sun, 25 Oct 2020 16:24:44 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 23 Oct 2020 21:15:09 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Mon, 19 Oct 2020 10:48:41 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> > > default property list of the PC DIMM device class:
> > > 
> > >     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> > > 
> > >     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
> > >                       PC_DIMM_UNASSIGNED_SLOT),
> > > 
> > > They should thus be always gettable for both PC DIMMs and NVDIMMs.
> > > An error in getting them can only be the result of a programming
> > > error. It doesn't make much sense to propagate the error in this
> > > case. Abort instead.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > TODO for future,
> > get rid of local_err in spapr_memory_plug() altogether, it should not fail.
> > it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.
> > 
> > that will clear up (a bit) road for dropping errp in spapr_memory_plug()  
> 
> Igor,
> 
> I could find time to look a bit into attaching DRCs at pre-plug and I
> think this isn't possible. The problem is that there doesn't seem to be
> a reverse operation for pre-plug. If realize fails for the DIMM device,
> spapr_drc_detach() wouldn't be called, which would be wrong.

probably I was clear enough, I didn't suggest to move spapr_drc_detach()
to pre_plug time but rather do a bit of surgery along the lines:

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2db810f73a..59a229b4fa 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3474,6 +3474,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
+    get drc
+    if (!spapr_drc_attachable(drc)) {
+        error out
+    }
+
     if (is_nvdimm) {
         spapr_nvdimm_validate(hotplug_dev, NVDIMM(dev), size, &local_err);
         if (local_err) {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fe998d8108..ae049bc202 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -371,14 +371,16 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
     } while (fdt_depth != 0);
 }
 
-void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
+
+bool spapr_drc_attachable(SpaprDrc *drc)
+{
+   return !drc->dev;
+}
+
+void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
 {
     trace_spapr_drc_attach(spapr_drc_index(drc));
 
-    if (drc->dev) {
-        error_setg(errp, "an attached device is still awaiting release");
-        return;
-    }
     g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
              || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));

> 
> Am I missing something ?
> 
> > > ---
> > >  hw/ppc/spapr.c |   17 +++--------------
> > >  1 file changed, 3 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 1b173861152f..62f217a6b914 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >  
> > >      if (!is_nvdimm) {
> > >          addr = object_property_get_uint(OBJECT(dimm),
> > > -                                        PC_DIMM_ADDR_PROP, &local_err);
> > > -        if (local_err) {
> > > -            goto out_unplug;
> > > -        }
> > > +                                        PC_DIMM_ADDR_PROP, &error_abort);
> > >          spapr_add_lmbs(dev, addr, size,
> > >                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> > >                         &local_err);
> > >      } else {
> > >          slot = object_property_get_int(OBJECT(dimm),
> > > -                                       PC_DIMM_SLOT_PROP, &local_err);
> > > -        if (local_err) {
> > > -            goto out_unplug;
> > > -        }
> > > +                                       PC_DIMM_SLOT_PROP, &error_abort);
> > >          /* We should have valid slot number at this point */
> > >          g_assert(slot >= 0);
> > >          spapr_add_nvdimm(dev, slot, &local_err);
> > > @@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > >                                          DeviceState *dev, Error **errp)
> > >  {
> > >      SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > -    Error *local_err = NULL;
> > >      PCDIMMDevice *dimm = PC_DIMM(dev);
> > >      uint32_t nr_lmbs;
> > >      uint64_t size, addr_start, addr;
> > > @@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > >      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > >  
> > >      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> > > -                                         &local_err);
> > > -    if (local_err) {
> > > -        error_propagate(errp, local_err);
> > > -        return;
> > > -    }
> > > +                                          &error_abort);
> > >  
> > >      /*
> > >       * An existing pending dimm state for this DIMM means that there is an
> > > 
> > > 
> > >   
> >   
> 

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Greg Kurz]

On Tue, 27 Oct 2020 12:54:24 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Sun, 25 Oct 2020 16:24:44 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Fri, 23 Oct 2020 21:15:09 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > > On Mon, 19 Oct 2020 10:48:41 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > >   
> > > > Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> > > > default property list of the PC DIMM device class:
> > > > 
> > > >     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> > > > 
> > > >     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
> > > >                       PC_DIMM_UNASSIGNED_SLOT),
> > > > 
> > > > They should thus be always gettable for both PC DIMMs and NVDIMMs.
> > > > An error in getting them can only be the result of a programming
> > > > error. It doesn't make much sense to propagate the error in this
> > > > case. Abort instead.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > > 
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > TODO for future,
> > > get rid of local_err in spapr_memory_plug() altogether, it should not fail.
> > > it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.
> > > 
> > > that will clear up (a bit) road for dropping errp in spapr_memory_plug()  
> > 
> > Igor,
> > 
> > I could find time to look a bit into attaching DRCs at pre-plug and I
> > think this isn't possible. The problem is that there doesn't seem to be
> > a reverse operation for pre-plug. If realize fails for the DIMM device,
> > spapr_drc_detach() wouldn't be called, which would be wrong.
> 
> probably I was clear enough, I didn't suggest to move spapr_drc_detach()
> to pre_plug time but rather do a bit of surgery along the lines:
> 

Ok, I get it now. I realize now I actually misread your suggestion. Sorry...

> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2db810f73a..59a229b4fa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3474,6 +3474,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> +    get drc
> +    if (!spapr_drc_attachable(drc)) {
> +        error out
> +    }
> +

It might require some more code refactoring because the way regular
PC-DIMMs are broken down into a set of logical memory blocks (LMBs),
each one having its own DRC but it's certainly doable. Probably for
QEMU 6.0 though since we're entering soft freeze and David already
fired a PR today.

>      if (is_nvdimm) {
>          spapr_nvdimm_validate(hotplug_dev, NVDIMM(dev), size, &local_err);
>          if (local_err) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fe998d8108..ae049bc202 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -371,14 +371,16 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>      } while (fdt_depth != 0);
>  }
>  
> -void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
> +
> +bool spapr_drc_attachable(SpaprDrc *drc)
> +{
> +   return !drc->dev;
> +}
> +
> +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
>  {
>      trace_spapr_drc_attach(spapr_drc_index(drc));
>  
> -    if (drc->dev) {
> -        error_setg(errp, "an attached device is still awaiting release");
> -        return;
> -    }
>      g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
>               || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
> 
> > 
> > Am I missing something ?
> > 
> > > > ---
> > > >  hw/ppc/spapr.c |   17 +++--------------
> > > >  1 file changed, 3 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 1b173861152f..62f217a6b914 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >  
> > > >      if (!is_nvdimm) {
> > > >          addr = object_property_get_uint(OBJECT(dimm),
> > > > -                                        PC_DIMM_ADDR_PROP, &local_err);
> > > > -        if (local_err) {
> > > > -            goto out_unplug;
> > > > -        }
> > > > +                                        PC_DIMM_ADDR_PROP, &error_abort);
> > > >          spapr_add_lmbs(dev, addr, size,
> > > >                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> > > >                         &local_err);
> > > >      } else {
> > > >          slot = object_property_get_int(OBJECT(dimm),
> > > > -                                       PC_DIMM_SLOT_PROP, &local_err);
> > > > -        if (local_err) {
> > > > -            goto out_unplug;
> > > > -        }
> > > > +                                       PC_DIMM_SLOT_PROP, &error_abort);
> > > >          /* We should have valid slot number at this point */
> > > >          g_assert(slot >= 0);
> > > >          spapr_add_nvdimm(dev, slot, &local_err);
> > > > @@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > > >                                          DeviceState *dev, Error **errp)
> > > >  {
> > > >      SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > -    Error *local_err = NULL;
> > > >      PCDIMMDevice *dimm = PC_DIMM(dev);
> > > >      uint32_t nr_lmbs;
> > > >      uint64_t size, addr_start, addr;
> > > > @@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> > > >      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
> > > >  
> > > >      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> > > > -                                         &local_err);
> > > > -    if (local_err) {
> > > > -        error_propagate(errp, local_err);
> > > > -        return;
> > > > -    }
> > > > +                                          &error_abort);
> > > >  
> > > >      /*
> > > >       * An existing pending dimm state for this DIMM means that there is an
> > > > 
> > > > 
> > > >   
> > >   
> > 
> 

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Igor Mammedov]

On Tue, 27 Oct 2020 16:18:58 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Tue, 27 Oct 2020 12:54:24 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Sun, 25 Oct 2020 16:24:44 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > On Fri, 23 Oct 2020 21:15:09 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >   
> > > > On Mon, 19 Oct 2020 10:48:41 +0200
> > > > Greg Kurz <groug@kaod.org> wrote:
> > > >     
> > > > > Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> > > > > default property list of the PC DIMM device class:
> > > > > 
> > > > >     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> > > > > 
> > > > >     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
> > > > >                       PC_DIMM_UNASSIGNED_SLOT),
> > > > > 
> > > > > They should thus be always gettable for both PC DIMMs and NVDIMMs.
> > > > > An error in getting them can only be the result of a programming
> > > > > error. It doesn't make much sense to propagate the error in this
> > > > > case. Abort instead.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > TODO for future,
> > > > get rid of local_err in spapr_memory_plug() altogether, it should not fail.
> > > > it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.
> > > > 
> > > > that will clear up (a bit) road for dropping errp in spapr_memory_plug()    
> > > 
> > > Igor,
> > > 
> > > I could find time to look a bit into attaching DRCs at pre-plug and I
> > > think this isn't possible. The problem is that there doesn't seem to be
> > > a reverse operation for pre-plug. If realize fails for the DIMM device,
> > > spapr_drc_detach() wouldn't be called, which would be wrong.  
> > 
> > probably I was clear enough, I didn't suggest to move spapr_drc_detach()
> > to pre_plug time but rather do a bit of surgery along the lines:
> >   
> 
> Ok, I get it now. I realize now I actually misread your suggestion. Sorry...
> 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2db810f73a..59a229b4fa 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3474,6 +3474,11 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          return;
> >      }
> >  
> > +    get drc
> > +    if (!spapr_drc_attachable(drc)) {
> > +        error out
> > +    }
> > +  
> 
> It might require some more code refactoring because the way regular
> PC-DIMMs are broken down into a set of logical memory blocks (LMBs),
> each one having its own DRC but it's certainly doable. Probably for
> QEMU 6.0 though since we're entering soft freeze and David already
> fired a PR today.

as far as it's not forgotten, it can be done later.

> 
> >      if (is_nvdimm) {
> >          spapr_nvdimm_validate(hotplug_dev, NVDIMM(dev), size, &local_err);
> >          if (local_err) {
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index fe998d8108..ae049bc202 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -371,14 +371,16 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> >      } while (fdt_depth != 0);
> >  }
> >  
> > -void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
> > +
> > +bool spapr_drc_attachable(SpaprDrc *drc)
> > +{
> > +   return !drc->dev;
> > +}
> > +
> > +void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
> >  {
> >      trace_spapr_drc_attach(spapr_drc_index(drc));
> >  
> > -    if (drc->dev) {
> > -        error_setg(errp, "an attached device is still awaiting release");
> > -        return;
> > -    }
> >      g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
> >               || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
> >   
> > > 
> > > Am I missing something ?
[...]

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Greg Kurz]

On Wed, 28 Oct 2020 16:22:16 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 27 Oct 2020 16:18:58 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
[...]
> > 
> > It might require some more code refactoring because the way regular
> > PC-DIMMs are broken down into a set of logical memory blocks (LMBs),
> > each one having its own DRC but it's certainly doable. Probably for
> > QEMU 6.0 though since we're entering soft freeze and David already
> > fired a PR today.
> 
> as far as it's not forgotten, it can be done later.
> 

David,

Can you create a ppc-for-6.0 branch ?

Cheers,

--
Greg

Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[David Gibson]

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

On Fri, Oct 30, 2020 at 02:25:42PM +0100, Greg Kurz wrote:
> On Wed, 28 Oct 2020 16:22:16 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 27 Oct 2020 16:18:58 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> [...]
> > > 
> > > It might require some more code refactoring because the way regular
> > > PC-DIMMs are broken down into a set of logical memory blocks (LMBs),
> > > each one having its own DRC but it's certainly doable. Probably for
> > > QEMU 6.0 though since we're entering soft freeze and David already
> > > fired a PR today.
> > 
> > as far as it's not forgotten, it can be done later.
> > 
> 
> David,
> 
> Can you create a ppc-for-6.0 branch ?

Done.

> 
> Cheers,
> 

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

[PATCH 5/5] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

As recommended in "qapi/error.h", add a bool return value to
spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
of local_err in spapr_memory_plug().

This allows to get rid of the error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c                |   23 ++++++++++-------------
 hw/ppc/spapr_nvdimm.c         |    5 +++--
 include/hw/ppc/spapr_nvdimm.h |    2 +-
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 62f217a6b914..0cc19b5863a4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
-static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
+static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                            bool dedicated_hp_event_source, Error **errp)
 {
     SpaprDrc *drc;
@@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                       addr / SPAPR_MEMORY_BLOCK_SIZE);
                 spapr_drc_detach(drc);
             }
-            return;
+            return false;
         }
         if (!hotplugged) {
             spapr_drc_reset(drc);
@@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                            nr_lmbs);
         }
     }
+    return true;
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
-    Error *local_err = NULL;
     SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint64_t size, addr;
@@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     if (!is_nvdimm) {
         addr = object_property_get_uint(OBJECT(dimm),
                                         PC_DIMM_ADDR_PROP, &error_abort);
-        spapr_add_lmbs(dev, addr, size,
-                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
-                       &local_err);
+        if (!spapr_add_lmbs(dev, addr, size,
+                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
+            goto out_unplug;
+        }
     } else {
         slot = object_property_get_int(OBJECT(dimm),
                                        PC_DIMM_SLOT_PROP, &error_abort);
         /* We should have valid slot number at this point */
         g_assert(slot >= 0);
-        spapr_add_nvdimm(dev, slot, &local_err);
-    }
-
-    if (local_err) {
-        goto out_unplug;
+        if (!spapr_add_nvdimm(dev, slot, errp)) {
+            goto out_unplug;
+        }
     }
 
     return;
 
 out_unplug:
     pc_dimm_unplug(dimm, MACHINE(ms));
-out:
-    error_propagate(errp, local_err);
 }
 
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 9e3d94071fe1..a833a63b5ed3 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 }
 
 
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
 {
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
@@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
     g_assert(drc);
 
     if (!spapr_drc_attach(drc, dev, errp)) {
-        return;
+        return false;
     }
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
     }
+    return true;
 }
 
 static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index 490b19a009f4..344582d2f5f7 100644
--- a/include/hw/ppc/spapr_nvdimm.h
+++ b/include/hw/ppc/spapr_nvdimm.h
@@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
 void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
 bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
                            uint64_t size, Error **errp);
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
 
 #endif

Re: [PATCH 5/5] spapr: Simplify error handling in spapr_memory_plug()

[Philippe Mathieu-Daudé]

On 10/19/20 10:49 AM, Greg Kurz wrote:
> As recommended in "qapi/error.h", add a bool return value to
> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> of local_err in spapr_memory_plug().
> 
> This allows to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/spapr.c                |   23 ++++++++++-------------
>   hw/ppc/spapr_nvdimm.c         |    5 +++--
>   include/hw/ppc/spapr_nvdimm.h |    2 +-
>   3 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 62f217a6b914..0cc19b5863a4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>       return 0;
>   }
>   
> -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                              bool dedicated_hp_event_source, Error **errp)
>   {
>       SpaprDrc *drc;
> @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                         addr / SPAPR_MEMORY_BLOCK_SIZE);
>                   spapr_drc_detach(drc);
>               }
> -            return;
> +            return false;
>           }
>           if (!hotplugged) {
>               spapr_drc_reset(drc);
> @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                              nr_lmbs);
>           }
>       }
> +    return true;
>   }
>   
>   static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>   {
> -    Error *local_err = NULL;
>       SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>       PCDIMMDevice *dimm = PC_DIMM(dev);
>       uint64_t size, addr;
> @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       if (!is_nvdimm) {
>           addr = object_property_get_uint(OBJECT(dimm),
>                                           PC_DIMM_ADDR_PROP, &error_abort);
> -        spapr_add_lmbs(dev, addr, size,
> -                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> -                       &local_err);
> +        if (!spapr_add_lmbs(dev, addr, size,
> +                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> +            goto out_unplug;
> +        }
>       } else {
>           slot = object_property_get_int(OBJECT(dimm),
>                                          PC_DIMM_SLOT_PROP, &error_abort);
>           /* We should have valid slot number at this point */
>           g_assert(slot >= 0);
> -        spapr_add_nvdimm(dev, slot, &local_err);
> -    }
> -
> -    if (local_err) {
> -        goto out_unplug;
> +        if (!spapr_add_nvdimm(dev, slot, errp)) {
> +            goto out_unplug;
> +        }
>       }
>   
>       return;
>   
>   out_unplug:
>       pc_dimm_unplug(dimm, MACHINE(ms));
> -out:
> -    error_propagate(errp, local_err);
>   }
>   
>   static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 9e3d94071fe1..a833a63b5ed3 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>   }
>   
>   
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>   {
>       SpaprDrc *drc;
>       bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>       g_assert(drc);
>   
>       if (!spapr_drc_attach(drc, dev, errp)) {
> -        return;
> +        return false;
>       }
>   
>       if (hotplugged) {
>           spapr_hotplug_req_add_by_index(drc);
>       }
> +    return true;
>   }
>   
>   static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 490b19a009f4..344582d2f5f7 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>   void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>   bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>                              uint64_t size, Error **errp);
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);

Maybe document the returned value?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>   #endif
> 
> 
> 

Re: [PATCH 5/5] spapr: Simplify error handling in spapr_memory_plug()

[Igor Mammedov]

On Mon, 19 Oct 2020 10:49:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> As recommended in "qapi/error.h", add a bool return value to
> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> of local_err in spapr_memory_plug().
> 
> This allows to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
you won't need this patch if check from spapr_drc_attach() were
moved to _pre_plug() time.

> ---
>  hw/ppc/spapr.c                |   23 ++++++++++-------------
>  hw/ppc/spapr_nvdimm.c         |    5 +++--
>  include/hw/ppc/spapr_nvdimm.h |    2 +-
>  3 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 62f217a6b914..0cc19b5863a4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      return 0;
>  }
>  
> -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                             bool dedicated_hp_event_source, Error **errp)
>  {
>      SpaprDrc *drc;
> @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                        addr / SPAPR_MEMORY_BLOCK_SIZE);
>                  spapr_drc_detach(drc);
>              }
> -            return;
> +            return false;
>          }
>          if (!hotplugged) {
>              spapr_drc_reset(drc);
> @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                             nr_lmbs);
>          }
>      }
> +    return true;
>  }
>  
>  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp)
>  {
> -    Error *local_err = NULL;
>      SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      uint64_t size, addr;
> @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
>                                          PC_DIMM_ADDR_PROP, &error_abort);
> -        spapr_add_lmbs(dev, addr, size,
> -                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
> -                       &local_err);
> +        if (!spapr_add_lmbs(dev, addr, size,
> +                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> +            goto out_unplug;
> +        }
>      } else {
>          slot = object_property_get_int(OBJECT(dimm),
>                                         PC_DIMM_SLOT_PROP, &error_abort);
>          /* We should have valid slot number at this point */
>          g_assert(slot >= 0);
> -        spapr_add_nvdimm(dev, slot, &local_err);
> -    }
> -
> -    if (local_err) {
> -        goto out_unplug;
> +        if (!spapr_add_nvdimm(dev, slot, errp)) {
> +            goto out_unplug;
> +        }
>      }
>  
>      return;
>  
>  out_unplug:
>      pc_dimm_unplug(dimm, MACHINE(ms));
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 9e3d94071fe1..a833a63b5ed3 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>  }
>  
>  
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>  {
>      SpaprDrc *drc;
>      bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>      g_assert(drc);
>  
>      if (!spapr_drc_attach(drc, dev, errp)) {
> -        return;
> +        return false;
>      }
>  
>      if (hotplugged) {
>          spapr_hotplug_req_add_by_index(drc);
>      }
> +    return true;
>  }
>  
>  static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 490b19a009f4..344582d2f5f7 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>  void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
>  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
>                             uint64_t size, Error **errp);
> -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
>  
>  #endif
> 
> 

Re: [PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[David Gibson]

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

On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:
> Hi,
> 
> This is a followup to a previous cleanup for the sPAPR code:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html
> 
> The last two patches had to be dropped because they were wrongly assuming
> that object_property_get_uint() returning zero meant failure. This led to
> a discussion in which arose a consensus that most of the time (not to say
> always) object property getters should never fail actually, ie. failure
> is very likely the result of a programming error and QEMU should abort.
> 
> This series aims at demonstrating a revelant case I've found while auditing
> object property getters (this is patch 4 that I've isolated from a huge
> 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
> is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
> from PC DIMMs. They expect to get some properties from the DIMM object,
> which happens to be set by default at the PC DIMM class level. It thus
> doesn't make sense to pass an error object and propagate it when getting
> them since this would lure the user into thinking they did something wrong.
> 
> Some preliminary cleanup is done on the way, especially dropping an unused
> @errp argument of pc_dimm_plug(). This affects several platforms other than
> sPAPR but I guess the patch is trivial enough to go through David's tree
> if it gets acks from the relevant maintainers.

Since this series mostly affects ppc, I've applied it to ppc-for-5.2.

It would be nice to have an acked-by from Igor or Michael for the
first patch, though.

> 
> ---
> 
> Greg Kurz (5):
>       pc-dimm: Drop @errp argument of pc_dimm_plug()
>       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP
>       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP
>       spapr: Pass &error_abort when getting some PC DIMM properties
>       spapr: Simplify error handling in spapr_memory_plug()
> 
> 
>  hw/arm/virt.c                 |    9 +-------
>  hw/i386/pc.c                  |    8 +------
>  hw/mem/pc-dimm.c              |    2 +-
>  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------
>  hw/ppc/spapr_nvdimm.c         |    5 +++-
>  include/hw/mem/pc-dimm.h      |    2 +-
>  include/hw/ppc/spapr_nvdimm.h |    2 +-
>  7 files changed, 25 insertions(+), 51 deletions(-)
> 

-- 
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: [PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[Greg Kurz]

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

On Thu, 22 Oct 2020 15:11:42 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:
> > Hi,
> > 
> > This is a followup to a previous cleanup for the sPAPR code:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html
> > 
> > The last two patches had to be dropped because they were wrongly assuming
> > that object_property_get_uint() returning zero meant failure. This led to
> > a discussion in which arose a consensus that most of the time (not to say
> > always) object property getters should never fail actually, ie. failure
> > is very likely the result of a programming error and QEMU should abort.
> > 
> > This series aims at demonstrating a revelant case I've found while auditing
> > object property getters (this is patch 4 that I've isolated from a huge
> > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
> > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
> > from PC DIMMs. They expect to get some properties from the DIMM object,
> > which happens to be set by default at the PC DIMM class level. It thus
> > doesn't make sense to pass an error object and propagate it when getting
> > them since this would lure the user into thinking they did something wrong.
> > 
> > Some preliminary cleanup is done on the way, especially dropping an unused
> > @errp argument of pc_dimm_plug(). This affects several platforms other than
> > sPAPR but I guess the patch is trivial enough to go through David's tree
> > if it gets acks from the relevant maintainers.
> 
> Since this series mostly affects ppc, I've applied it to ppc-for-5.2.
> 
> It would be nice to have an acked-by from Igor or Michael for the
> first patch, though.
> 

David,

Igor sent a R-b for patches 1 and 4. He also suggested to call
spapr_drc_attach() at pre-plug time. I'll look into this, so maybe
you can drop patch 5 from ppc-for-5.2 (or the entire series at
your convenience).

Cheers,

--
Greg

> > 
> > ---
> > 
> > Greg Kurz (5):
> >       pc-dimm: Drop @errp argument of pc_dimm_plug()
> >       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP
> >       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP
> >       spapr: Pass &error_abort when getting some PC DIMM properties
> >       spapr: Simplify error handling in spapr_memory_plug()
> > 
> > 
> >  hw/arm/virt.c                 |    9 +-------
> >  hw/i386/pc.c                  |    8 +------
> >  hw/mem/pc-dimm.c              |    2 +-
> >  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------
> >  hw/ppc/spapr_nvdimm.c         |    5 +++-
> >  include/hw/mem/pc-dimm.h      |    2 +-
> >  include/hw/ppc/spapr_nvdimm.h |    2 +-
> >  7 files changed, 25 insertions(+), 51 deletions(-)
> > 
> 


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

Re: [PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[Greg Kurz]

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

On Sun, 25 Oct 2020 11:13:40 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 22 Oct 2020 15:11:42 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:
> > > Hi,
> > > 
> > > This is a followup to a previous cleanup for the sPAPR code:
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html
> > > 
> > > The last two patches had to be dropped because they were wrongly assuming
> > > that object_property_get_uint() returning zero meant failure. This led to
> > > a discussion in which arose a consensus that most of the time (not to say
> > > always) object property getters should never fail actually, ie. failure
> > > is very likely the result of a programming error and QEMU should abort.
> > > 
> > > This series aims at demonstrating a revelant case I've found while auditing
> > > object property getters (this is patch 4 that I've isolated from a huge
> > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
> > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
> > > from PC DIMMs. They expect to get some properties from the DIMM object,
> > > which happens to be set by default at the PC DIMM class level. It thus
> > > doesn't make sense to pass an error object and propagate it when getting
> > > them since this would lure the user into thinking they did something wrong.
> > > 
> > > Some preliminary cleanup is done on the way, especially dropping an unused
> > > @errp argument of pc_dimm_plug(). This affects several platforms other than
> > > sPAPR but I guess the patch is trivial enough to go through David's tree
> > > if it gets acks from the relevant maintainers.
> > 
> > Since this series mostly affects ppc, I've applied it to ppc-for-5.2.
> > 
> > It would be nice to have an acked-by from Igor or Michael for the
> > first patch, though.
> > 
> 
> David,
> 
> Igor sent a R-b for patches 1 and 4. He also suggested to call
> spapr_drc_attach() at pre-plug time. I'll look into this, so maybe
> you can drop patch 5 from ppc-for-5.2 (or the entire series at
> your convenience).
> 

It seems that spapr_drc_attach() cannot be called at pre-plug time
actually because there is no way to call spapr_drc_detach() if
the device fails to realize. I think you there's nothing else to do
for this series than adding Igor's r-b to patches 1 and 4.

> Cheers,
> 
> --
> Greg
> 
> > > 
> > > ---
> > > 
> > > Greg Kurz (5):
> > >       pc-dimm: Drop @errp argument of pc_dimm_plug()
> > >       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP
> > >       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP
> > >       spapr: Pass &error_abort when getting some PC DIMM properties
> > >       spapr: Simplify error handling in spapr_memory_plug()
> > > 
> > > 
> > >  hw/arm/virt.c                 |    9 +-------
> > >  hw/i386/pc.c                  |    8 +------
> > >  hw/mem/pc-dimm.c              |    2 +-
> > >  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------
> > >  hw/ppc/spapr_nvdimm.c         |    5 +++-
> > >  include/hw/mem/pc-dimm.h      |    2 +-
> > >  include/hw/ppc/spapr_nvdimm.h |    2 +-
> > >  7 files changed, 25 insertions(+), 51 deletions(-)
> > > 
> > 
> 


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

Re: [PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)

[David Gibson]

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

On Sun, Oct 25, 2020 at 10:33:06PM +0100, Greg Kurz wrote:
> On Sun, 25 Oct 2020 11:13:40 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Thu, 22 Oct 2020 15:11:42 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:
> > > > Hi,
> > > > 
> > > > This is a followup to a previous cleanup for the sPAPR code:
> > > > 
> > > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html
> > > > 
> > > > The last two patches had to be dropped because they were wrongly assuming
> > > > that object_property_get_uint() returning zero meant failure. This led to
> > > > a discussion in which arose a consensus that most of the time (not to say
> > > > always) object property getters should never fail actually, ie. failure
> > > > is very likely the result of a programming error and QEMU should abort.
> > > > 
> > > > This series aims at demonstrating a revelant case I've found while auditing
> > > > object property getters (this is patch 4 that I've isolated from a huge
> > > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
> > > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
> > > > from PC DIMMs. They expect to get some properties from the DIMM object,
> > > > which happens to be set by default at the PC DIMM class level. It thus
> > > > doesn't make sense to pass an error object and propagate it when getting
> > > > them since this would lure the user into thinking they did something wrong.
> > > > 
> > > > Some preliminary cleanup is done on the way, especially dropping an unused
> > > > @errp argument of pc_dimm_plug(). This affects several platforms other than
> > > > sPAPR but I guess the patch is trivial enough to go through David's tree
> > > > if it gets acks from the relevant maintainers.
> > > 
> > > Since this series mostly affects ppc, I've applied it to ppc-for-5.2.
> > > 
> > > It would be nice to have an acked-by from Igor or Michael for the
> > > first patch, though.
> > > 
> > 
> > David,
> > 
> > Igor sent a R-b for patches 1 and 4. He also suggested to call
> > spapr_drc_attach() at pre-plug time. I'll look into this, so maybe
> > you can drop patch 5 from ppc-for-5.2 (or the entire series at
> > your convenience).
> > 
> 
> It seems that spapr_drc_attach() cannot be called at pre-plug time
> actually because there is no way to call spapr_drc_detach() if
> the device fails to realize. I think you there's nothing else to do
> for this series than adding Igor's r-b to patches 1 and 4.

Ok.  In fact I'd already added Igor's r-b, just hadn't pushed out my
latest tree.

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