diff mbox series

[for-6.0,2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only

Message ID 20201120234208.683521-3-groug@kaod.org (mailing list archive)
State New, archived
Headers show
Series spapr: Perform hotplug sanity checks at pre-plug | expand

Commit Message

Greg Kurz Nov. 20, 2020, 11:42 p.m. UTC 
Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
that the memory slot is available and that addresses don't overlap
with existing memory regions. The corresponding DRCs in the LMB
and PMEM namespaces are thus necessarily attachable at plug time.

Pass &error_abort to spapr_drc_attach() in spapr_add_lmbs() and
spapr_add_nvdimm(). This allows to greatly simplify error handling
on the plug path.

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

Comments

David Gibson Nov. 23, 2020, 4:55 a.m. UTC | #1 
On Sat, Nov 21, 2020 at 12:42:01AM +0100, Greg Kurz wrote:
> Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
> that the memory slot is available and that addresses don't overlap
> with existing memory regions. The corresponding DRCs in the LMB
> and PMEM namespaces are thus necessarily attachable at plug time.
> 
> Pass &error_abort to spapr_drc_attach() in spapr_add_lmbs() and
> spapr_add_nvdimm(). This allows to greatly simplify error handling
> on the plug path.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-6.0, thanks.

> ---
>  include/hw/ppc/spapr_nvdimm.h |  2 +-
>  hw/ppc/spapr.c                | 40 ++++++++++++-----------------------
>  hw/ppc/spapr_nvdimm.c         | 11 +++++-----
>  3 files changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 344582d2f5f7..73be250e2ac9 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);
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
>  
>  #endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12a012d9dd09..394d28d9e081 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,8 +3382,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      return 0;
>  }
>  
> -static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> -                           bool dedicated_hp_event_source, Error **errp)
> +static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> +                           bool dedicated_hp_event_source)
>  {
>      SpaprDrc *drc;
>      uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> @@ -3396,15 +3396,12 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                addr / SPAPR_MEMORY_BLOCK_SIZE);
>          g_assert(drc);
>  
> -        if (!spapr_drc_attach(drc, dev, errp)) {
> -            while (addr > addr_start) {
> -                addr -= SPAPR_MEMORY_BLOCK_SIZE;
> -                drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> -                                      addr / SPAPR_MEMORY_BLOCK_SIZE);
> -                spapr_drc_detach(drc);
> -            }
> -            return false;
> -        }
> +        /*
> +         * memory_device_get_free_addr() provided a range of free addresses
> +         * that doesn't overlap with any existing mapping at pre-plug. The
> +         * corresponding LMB DRCs are thus assumed to be all attachable.
> +         */
> +        spapr_drc_attach(drc, dev, &error_abort);
>          if (!hotplugged) {
>              spapr_drc_reset(drc);
>          }
> @@ -3425,11 +3422,9 @@ static bool 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)
> +static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>      SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -3444,24 +3439,15 @@ 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);
> -        if (!spapr_add_lmbs(dev, addr, size,
> -                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> -            goto out_unplug;
> -        }
> +        spapr_add_lmbs(dev, addr, size,
> +                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
>      } 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);
> -        if (!spapr_add_nvdimm(dev, slot, errp)) {
> -            goto out_unplug;
> -        }
> +        spapr_add_nvdimm(dev, slot);
>      }
> -
> -    return;
> -
> -out_unplug:
> -    pc_dimm_unplug(dimm, MACHINE(ms));
>  }
>  
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -4009,7 +3995,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        spapr_memory_plug(hotplug_dev, dev, errp);
> +        spapr_memory_plug(hotplug_dev, dev);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          spapr_core_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5ed3..2f1c196e1b76 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,
>  }
>  
>  
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
>  {
>      SpaprDrc *drc;
>      bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -97,14 +97,15 @@ bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
>      drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
>      g_assert(drc);
>  
> -    if (!spapr_drc_attach(drc, dev, errp)) {
> -        return false;
> -    }
> +    /*
> +     * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
> +     * corresponding DRC is thus assumed to be attachable.
> +     */
> +    spapr_drc_attach(drc, dev, &error_abort);
>  
>      if (hotplugged) {
>          spapr_hotplug_req_add_by_index(drc);
>      }
> -    return true;
>  }
>  
>  static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
diff mbox series

Patch

diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index 344582d2f5f7..73be250e2ac9 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);
-bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
+void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
 
 #endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 12a012d9dd09..394d28d9e081 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3382,8 +3382,8 @@  int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
-static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
-                           bool dedicated_hp_event_source, Error **errp)
+static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
+                           bool dedicated_hp_event_source)
 {
     SpaprDrc *drc;
     uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
@@ -3396,15 +3396,12 @@  static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                               addr / SPAPR_MEMORY_BLOCK_SIZE);
         g_assert(drc);
 
-        if (!spapr_drc_attach(drc, dev, errp)) {
-            while (addr > addr_start) {
-                addr -= SPAPR_MEMORY_BLOCK_SIZE;
-                drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
-                                      addr / SPAPR_MEMORY_BLOCK_SIZE);
-                spapr_drc_detach(drc);
-            }
-            return false;
-        }
+        /*
+         * memory_device_get_free_addr() provided a range of free addresses
+         * that doesn't overlap with any existing mapping at pre-plug. The
+         * corresponding LMB DRCs are thus assumed to be all attachable.
+         */
+        spapr_drc_attach(drc, dev, &error_abort);
         if (!hotplugged) {
             spapr_drc_reset(drc);
         }
@@ -3425,11 +3422,9 @@  static bool 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)
+static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
 {
     SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
@@ -3444,24 +3439,15 @@  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);
-        if (!spapr_add_lmbs(dev, addr, size,
-                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
-            goto out_unplug;
-        }
+        spapr_add_lmbs(dev, addr, size,
+                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
     } 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);
-        if (!spapr_add_nvdimm(dev, slot, errp)) {
-            goto out_unplug;
-        }
+        spapr_add_nvdimm(dev, slot);
     }
-
-    return;
-
-out_unplug:
-    pc_dimm_unplug(dimm, MACHINE(ms));
 }
 
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -4009,7 +3995,7 @@  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_plug(hotplug_dev, dev, errp);
+        spapr_memory_plug(hotplug_dev, dev);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5ed3..2f1c196e1b76 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,
 }
 
 
-bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
+void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
 {
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
@@ -97,14 +97,15 @@  bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
     g_assert(drc);
 
-    if (!spapr_drc_attach(drc, dev, errp)) {
-        return false;
-    }
+    /*
+     * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
+     * corresponding DRC is thus assumed to be attachable.
+     */
+    spapr_drc_attach(drc, dev, &error_abort);
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
     }
-    return true;
 }
 
 static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,