[PATCH for-6.0 v2 0/4] spapr: Perform hotplug sanity checks at pre-plug

[PATCH for-6.0 v2 0/4] spapr: Perform hotplug sanity checks at pre-plug

[Greg Kurz]

Igor recently suggested that instead of failing in spapr_drc_attach()
at plug time we should rather check that the DRC is attachable at
pre-plug time. This allows to error out before the hot-plugged device
is even realized and to come up with simpler plug callbacks.

sPAPR currently supports hotplug of PCI devices, PHBs, CPU cores,
PC-DIMM/NVDIMM memory and TPM proxy devices. Some of these already
do sanity checks at pre-plug that are sufficient to ensure the DRC
are attachables. Some others don't even have a pre-plug handler.

This series adds the missing pieces so that all failing conditions
are caught at pre-plug time instead of plug time for all devices.

v2: - hopefully less fragile way of setting compat mode for hot-plugged
      CPUs

Greg Kurz (4):
  spapr: Fix pre-2.10 dummy ICP hack
  spapr: Abort if ppc_set_compat() fails for hot-plugged CPUs
  spapr: Simplify error path of spapr_core_plug()
  spapr: spapr_drc_attach() cannot fail

 include/hw/ppc/spapr_drc.h |  8 ++++++-
 hw/ppc/spapr.c             | 49 ++++++++++++++++++--------------------
 hw/ppc/spapr_drc.c         |  8 ++-----
 hw/ppc/spapr_nvdimm.c      |  2 +-
 hw/ppc/spapr_pci.c         |  2 +-
 5 files changed, 34 insertions(+), 35 deletions(-)

-- 
2.26.2

[PATCH for-6.0 v2 1/4] spapr: Fix pre-2.10 dummy ICP hack

[Greg Kurz]

This hack registers dummy VMState entries of ICPs in order to
support migration of old pseries machine types that used to
create all smp.max_cpus possible ICPs at machine init.

Part of the work is to unregister the dummy entries when plugging
an actual vCPU core, and to register them back when unplugging the
core. The code that unregisters the dummy ICPs in spapr_core_plug()
is misplaced: if ppc_set_compat() fails afterwards, the hotplug
operation will be cancelled and the dummy ICPs won't be registered
back since the unplug handler isn't called.

Unregister the dummy ICPs at the end of spapr_core_plug().

Signed-off-by: Greg Kurz <groug@kaod.org>
---

The next patch makes this patch a bit useless. I post it
anyway for the records because it is a programming error.
---
 hw/ppc/spapr.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e8b236a8313b..57c6eecc56d6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3785,13 +3785,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     core_slot->cpu = OBJECT(dev);
 
-    if (smc->pre_2_10_has_unused_icps) {
-        for (i = 0; i < cc->nr_threads; i++) {
-            cs = CPU(core->threads[i]);
-            pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
-        }
-    }
-
     /*
      * Set compatibility mode to match the boot CPU, which was either set
      * by the machine reset code or by CAS.
@@ -3805,6 +3798,13 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
             }
         }
     }
+
+    if (smc->pre_2_10_has_unused_icps) {
+        for (i = 0; i < cc->nr_threads; i++) {
+            cs = CPU(core->threads[i]);
+            pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
+        }
+    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-- 
2.26.2

[PATCH for-6.0 v2 2/4] spapr: Abort if ppc_set_compat() fails for hot-plugged CPUs

[Greg Kurz]

When a CPU is hot-plugged, we set its compat mode to match the boot
CPU, which was either set by machine reset or by CAS. This is currently
handled in the plug handler after the core got realized. Potential errors
of ppc_set_compat() are propagated to the hot-plug logic.

Handling errors this late in the hot-plug sequence is generally frown
upon. Ideally, we should do sanity checks in a pre-plug handler and pass
&error_abort to ppc_set_compat() in the plug handler.

We can filter out some error cases of ppc_set_compat() by calling
ppc_check_compat() at pre-plug. But ppc_set_compat() also sets the
compat register in KVM, and KVM doesn't provide any API that would
allow to check valid compat mode settings beforehand.

However, at this point we know that the compat mode was already
successfully set for the boot CPU. Since this all boils down to
setting a register with the very same value that was valid
for the boot CPU, it should definitely not fail for hot-plugged
CPUS.

Pass &error_abort to ppc_set_compat().

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 57c6eecc56d6..3a32918dd9a9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3787,15 +3787,13 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     /*
      * Set compatibility mode to match the boot CPU, which was either set
-     * by the machine reset code or by CAS.
+     * by the machine reset code or by CAS. This really shouldn't fail at
+     * this point.
      */
     if (hotplugged) {
         for (i = 0; i < cc->nr_threads; i++) {
-            if (ppc_set_compat(core->threads[i],
-                               POWERPC_CPU(first_cpu)->compat_pvr,
-                               errp) < 0) {
-                return;
-            }
+            ppc_set_compat(core->threads[i], POWERPC_CPU(first_cpu)->compat_pvr,
+                           &error_abort);
         }
     }
 
-- 
2.26.2

[PATCH for-6.0 v2 3/4] spapr: Simplify error path of spapr_core_plug()

[Greg Kurz]

spapr_core_pre_plug() already guarantees that the slot for the given core
ID is available. It is thus safe to assume that spapr_find_cpu_slot()
returns a slot during plug. Turn the error path into an assertion.
It is also safe to assume that no device is attached to the corresponding
DRC and that spapr_drc_attach() shouldn't fail.

Pass &error_abort to spapr_drc_attach() and simplify error handling.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3a32918dd9a9..6707fafdd313 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3741,8 +3741,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
-static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                            Error **errp)
+static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(spapr);
@@ -3757,20 +3756,20 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     int i;
 
     core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
-    if (!core_slot) {
-        error_setg(errp, "Unable to find CPU core with core-id: %d",
-                   cc->core_id);
-        return;
-    }
+    g_assert(core_slot); /* Already checked in spapr_core_pre_plug() */
+
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
                           spapr_vcpu_id(spapr, cc->core_id));
 
     g_assert(drc || !mc->has_hotpluggable_cpus);
 
     if (drc) {
-        if (!spapr_drc_attach(drc, dev, errp)) {
-            return;
-        }
+        /*
+         * spapr_core_pre_plug() already buys us this is a brand new
+         * core being plugged into a free slot. Nothing should already
+         * be attached to the corresponding DRC.
+         */
+        spapr_drc_attach(drc, dev, &error_abort);
 
         if (hotplugged) {
             /*
@@ -4012,7 +4011,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_plug(hotplug_dev, dev);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
         spapr_phb_plug(hotplug_dev, dev);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_TPM_PROXY)) {
-- 
2.26.2

[PATCH for-6.0 v2 4/4] spapr: spapr_drc_attach() cannot fail

[Greg Kurz]

All users are passing &error_abort already. Document the fact
that spapr_drc_attach() should only be passed a free DRC, which
is supposedly the case if appropriate checking is done earlier.

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

diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 165b281496bb..def3593adc8b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -235,7 +235,13 @@ SpaprDrc *spapr_drc_by_index(uint32_t index);
 SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
 int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
 
-bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
+/*
+ * These functions respectively abort if called with a device already
+ * attached or no device attached. In the case of spapr_drc_attach(),
+ * this means that the attachability of the DRC *must* be checked
+ * beforehand (eg. check drc->dev at pre-plug).
+ */
+void spapr_drc_attach(SpaprDrc *drc, DeviceState *d);
 void spapr_drc_detach(SpaprDrc *drc);
 
 /* Returns true if a hot plug/unplug request is pending */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6707fafdd313..d1dcf3ab2c94 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3401,7 +3401,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
          * 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);
+        spapr_drc_attach(drc, dev);
         if (!hotplugged) {
             spapr_drc_reset(drc);
         }
@@ -3769,7 +3769,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
          * core being plugged into a free slot. Nothing should already
          * be attached to the corresponding DRC.
          */
-        spapr_drc_attach(drc, dev, &error_abort);
+        spapr_drc_attach(drc, dev);
 
         if (hotplugged) {
             /*
@@ -3934,7 +3934,7 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
     assert(drc);
 
     /* spapr_phb_pre_plug() already checked the DRC is attachable */
-    spapr_drc_attach(drc, dev, &error_abort);
+    spapr_drc_attach(drc, dev);
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 77718cde1ff2..f991cf89a08a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -369,14 +369,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
     } while (fdt_depth != 0);
 }
 
-bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
+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 false;
-    }
+    g_assert(!drc->dev);
     g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
              || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
 
@@ -386,7 +383,6 @@ bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
                              object_get_typename(OBJECT(drc->dev)),
                              (Object **)(&drc->dev),
                              NULL, 0);
-    return true;
 }
 
 static void spapr_drc_release(SpaprDrc *drc)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 2f1c196e1b76..73ee006541a6 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -101,7 +101,7 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
      * 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);
+    spapr_drc_attach(drc, dev);
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2829f298d9c1..e946bd5055cc 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1601,7 +1601,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
     }
 
     /* spapr_pci_pre_plug() already checked the DRC is attachable */
-    spapr_drc_attach(drc, DEVICE(pdev), &error_abort);
+    spapr_drc_attach(drc, DEVICE(pdev));
 
     /* If this is function 0, signal hotplug for all the device functions.
      * Otherwise defer sending the hotplug event.
-- 
2.26.2

Re: [PATCH for-6.0 v2 0/4] spapr: Perform hotplug sanity checks at pre-plug

[David Gibson]

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

On Tue, Dec 01, 2020 at 12:37:24PM +0100, Greg Kurz wrote:
> Igor recently suggested that instead of failing in spapr_drc_attach()
> at plug time we should rather check that the DRC is attachable at
> pre-plug time. This allows to error out before the hot-plugged device
> is even realized and to come up with simpler plug callbacks.
> 
> sPAPR currently supports hotplug of PCI devices, PHBs, CPU cores,
> PC-DIMM/NVDIMM memory and TPM proxy devices. Some of these already
> do sanity checks at pre-plug that are sufficient to ensure the DRC
> are attachables. Some others don't even have a pre-plug handler.
> 
> This series adds the missing pieces so that all failing conditions
> are caught at pre-plug time instead of plug time for all devices.
> 
> v2: - hopefully less fragile way of setting compat mode for hot-plugged
>       CPUs

Applied to ppc-for-6.0, thanks.

> 
> Greg Kurz (4):
>   spapr: Fix pre-2.10 dummy ICP hack
>   spapr: Abort if ppc_set_compat() fails for hot-plugged CPUs
>   spapr: Simplify error path of spapr_core_plug()
>   spapr: spapr_drc_attach() cannot fail
> 
>  include/hw/ppc/spapr_drc.h |  8 ++++++-
>  hw/ppc/spapr.c             | 49 ++++++++++++++++++--------------------
>  hw/ppc/spapr_drc.c         |  8 ++-----
>  hw/ppc/spapr_nvdimm.c      |  2 +-
>  hw/ppc/spapr_pci.c         |  2 +-
>  5 files changed, 34 insertions(+), 35 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 --]