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

[David Gibson]

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,

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

signature.asc
Description: PGP signature