[PATCH 0/3] spapr: Fix device unplug vs CAS or migration

[PATCH 0/3] spapr: Fix device unplug vs CAS or migration

[Greg Kurz]

While working on getting rid of CAS reboot, I realized that we currently
don't handle device hot unplug properly in the following situations:

1) if the device is unplugged between boot and CAS, SLOF doesn't handle
   the even, which is a known limitation. The device hence stays around
   forever (specifically, until some other event is emitted and the guest
   eventually completes the unplug or a reboot). Until we can teach SLOF
   to correctly process the full FDT at CAS, we should trigger a CAS reboot,
   like we already do for hotplug.

2) if the guest is migrated after the even was emitted but before the
   guest could process it, the destination is unaware of the pending
   unplug operation and doesn't remove the device when the guests
   releases it. The 'unplug_requested' field of the DRC is actually state
   that should be migrated.

--
Greg

---

Greg Kurz (3):
      spapr: Don't use spapr_drc_needed() in CAS code
      spapr: Detect hot unplugged devices during CAS
      spapr: Migrate SpaprDrc::unplug_requested


 hw/ppc/spapr_drc.c         |   30 ++++++++++++++++++++++++++----
 hw/ppc/spapr_hcall.c       |   12 +++++++++---
 include/hw/ppc/spapr_drc.h |    8 +++++++-
 3 files changed, 42 insertions(+), 8 deletions(-)

[PATCH 1/3] spapr: Don't use spapr_drc_needed() in CAS code

[Greg Kurz]

We currently don't support hotplug of devices between boot and CAS. If
this happens a CAS reboot is triggered. We detect this during CAS using
the spapr_drc_needed() function which is essentially a VMStateDescription
.needed callback. Even if the condition for CAS reboot happens to be the
same as for DRC migration, it looks wrong to use a migration related helper
for this.

Introduce a helper with more explicit semantics (ie. the device attached
to this DRC is ready or not) and use it in both CAS and DRC migration code.

This doesn't change any behaviour.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_drc.c         |    5 ++---
 hw/ppc/spapr_hcall.c       |   12 +++++++++---
 include/hw/ppc/spapr_drc.h |    8 +++++++-
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 17aeac38016d..d512ac6e1e7f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -455,10 +455,9 @@ void spapr_drc_reset(SpaprDrc *drc)
     }
 }
 
-bool spapr_drc_needed(void *opaque)
+static bool spapr_drc_needed(void *opaque)
 {
     SpaprDrc *drc = (SpaprDrc *)opaque;
-    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
     /* If no dev is plugged in there is no need to migrate the DRC state */
     if (!drc->dev) {
@@ -469,7 +468,7 @@ bool spapr_drc_needed(void *opaque)
      * We need to migrate the state if it's not equal to the expected
      * long-term state, which is the same as the coldplugged initial
      * state */
-    return (drc->state != drck->ready_state);
+    return !spapr_drc_device_ready(drc);
 }
 
 static const VMStateDescription vmstate_spapr_drc = {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index b8bb66b5c0d4..7a33d79bbae9 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1642,18 +1642,24 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
 
 static bool spapr_hotplugged_dev_before_cas(void)
 {
-    Object *drc_container, *obj;
+    Object *drc_container;
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
 
     drc_container = container_get(object_get_root(), "/dr-connector");
     object_property_iter_init(&iter, drc_container);
     while ((prop = object_property_iter_next(&iter))) {
+        SpaprDrc *drc;
+
         if (!strstart(prop->type, "link<", NULL)) {
             continue;
         }
-        obj = object_property_get_link(drc_container, prop->name, NULL);
-        if (spapr_drc_needed(obj)) {
+        drc = SPAPR_DR_CONNECTOR(object_property_get_link(drc_container,
+                                                          prop->name, NULL));
+        if (!drc->dev) {
+            continue;
+        }
+        if (!spapr_drc_device_ready(drc)) {
             return true;
         }
     }
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 83f03cc5773c..8e8bbedb21b7 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -269,7 +269,13 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
 
 void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
 void spapr_drc_detach(SpaprDrc *drc);
-bool spapr_drc_needed(void *opaque);
+
+static inline bool spapr_drc_device_ready(SpaprDrc *drc)
+{
+    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+    return drc->state == drck->ready_state;
+}
 
 static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
 {

[PATCH 2/3] spapr: Detect hot unplugged devices during CAS

[Greg Kurz]

We can't properly handle hotplug of a device as long the guest kernel isn't
fully booted. We detect this at CAS and potentially trigger a CAS reboot to
complete the hotplug sequence.

The same goes actually with hot unplug but we currently don't detect it.
Doing device_del before CAS hence seems to be ignored: when the guest
is booted, it still sees the _unplugged_ device in the DT and configures
it. But if some other hotplug event happens later, then the unplug request
is finally processed by the guest and the device goes away.

This doesn't seem to cause any crash but it is still very confusing. Detect
device unplug at CAS and request a CAS reboot.

Hopefully, when SLOF will know how to handle device addition and deletion
in its DT according to the FDT provided by QEMU at CAS, we'll be able to
address this differently (ie, coldplugging the new devices and removing the
ones with a pending unplug request).

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_hcall.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7a33d79bbae9..84690cc2c1ce 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1659,7 +1659,7 @@ static bool spapr_hotplugged_dev_before_cas(void)
         if (!drc->dev) {
             continue;
         }
-        if (!spapr_drc_device_ready(drc)) {
+        if (spapr_drc_unplug_requested(drc) || !spapr_drc_device_ready(drc)) {
             return true;
         }
     }

[PATCH 3/3] spapr: Migrate SpaprDrc::unplug_requested

[Greg Kurz]

Hot unplugging a device is an asynchronous operation. If the guest is
migrated after the event was sent but before it could release the
device with RTAS, the destination QEMU doesn't know about the pending
unplug operation and doesn't actually remove the device when the guest
finally releases it. The device

Migrate SpaprDrc::unplug_requested to fix the inconsistency. This is
done with a subsection that is only sent if an unplug request is
pending. This allows to preserve migration with older guests in the
case of a pending hotplug request. This will cause migration to fail
if the destination can't handle the subsection, but this is better
than ending with an inconsistency.

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

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index d512ac6e1e7f..6f5cab70fc6b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -455,6 +455,22 @@ void spapr_drc_reset(SpaprDrc *drc)
     }
 }
 
+static bool spapr_drc_unplug_requested_needed(void *opaque)
+{
+    return spapr_drc_unplug_requested(opaque);
+}
+
+static const VMStateDescription vmstate_spapr_drc_unplug_requested = {
+    .name = "spapr_drc/unplug_requested",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_drc_unplug_requested_needed,
+    .fields  = (VMStateField []) {
+        VMSTATE_BOOL(unplug_requested, SpaprDrc),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool spapr_drc_needed(void *opaque)
 {
     SpaprDrc *drc = (SpaprDrc *)opaque;
@@ -467,8 +483,11 @@ static bool spapr_drc_needed(void *opaque)
     /*
      * We need to migrate the state if it's not equal to the expected
      * long-term state, which is the same as the coldplugged initial
-     * state */
-    return !spapr_drc_device_ready(drc);
+     * state, or if an unplug request is pending.
+     */
+    return
+        spapr_drc_unplug_requested_needed(drc) ||
+        !spapr_drc_device_ready(drc);
 }
 
 static const VMStateDescription vmstate_spapr_drc = {
@@ -479,6 +498,10 @@ static const VMStateDescription vmstate_spapr_drc = {
     .fields  = (VMStateField []) {
         VMSTATE_UINT32(state, SpaprDrc),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_spapr_drc_unplug_requested,
+        NULL
     }
 };
 

Re: [PATCH 0/3] spapr: Fix device unplug vs CAS or migration

[Greg Kurz]

Ping ?

This series fixes actual bugs. Also, I have another patch on top of
that to cold plug (or remove) devices pending hot plug (or unplug)
before CAS, hence removing the need for CAS reboot in these cases.
This requires SLOF to correctly parse the FDT it gets at CAS. Patches
have been sent for that too:

https://git.qemu.org/?p=SLOF.git;a=commitdiff;h=689ff6f6554d94fdab854bf4fc4ec85e2675e43d
https://git.qemu.org/?p=SLOF.git;a=commitdiff;h=a093be1ebe7a48321646601d94be6cf735c81e12
https://patchwork.ozlabs.org/patch/1235817/

On Mon, 03 Feb 2020 23:36:04 +0100
Greg Kurz <groug@kaod.org> wrote:

> While working on getting rid of CAS reboot, I realized that we currently
> don't handle device hot unplug properly in the following situations:
> 
> 1) if the device is unplugged between boot and CAS, SLOF doesn't handle
>    the even, which is a known limitation. The device hence stays around
>    forever (specifically, until some other event is emitted and the guest
>    eventually completes the unplug or a reboot). Until we can teach SLOF
>    to correctly process the full FDT at CAS, we should trigger a CAS reboot,
>    like we already do for hotplug.
> 
> 2) if the guest is migrated after the even was emitted but before the
>    guest could process it, the destination is unaware of the pending
>    unplug operation and doesn't remove the device when the guests
>    releases it. The 'unplug_requested' field of the DRC is actually state
>    that should be migrated.
> 
> --
> Greg
> 
> ---
> 
> Greg Kurz (3):
>       spapr: Don't use spapr_drc_needed() in CAS code
>       spapr: Detect hot unplugged devices during CAS
>       spapr: Migrate SpaprDrc::unplug_requested
> 
> 
>  hw/ppc/spapr_drc.c         |   30 ++++++++++++++++++++++++++----
>  hw/ppc/spapr_hcall.c       |   12 +++++++++---
>  include/hw/ppc/spapr_drc.h |    8 +++++++-
>  3 files changed, 42 insertions(+), 8 deletions(-)
> 
> 

Re: [PATCH 3/3] spapr: Migrate SpaprDrc::unplug_requested

[David Gibson]

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

On Mon, Feb 03, 2020 at 11:36:22PM +0100, Greg Kurz wrote:
> Hot unplugging a device is an asynchronous operation. If the guest is
> migrated after the event was sent but before it could release the
> device with RTAS, the destination QEMU doesn't know about the pending
> unplug operation and doesn't actually remove the device when the guest
> finally releases it. The device
> 
> Migrate SpaprDrc::unplug_requested to fix the inconsistency. This is
> done with a subsection that is only sent if an unplug request is
> pending. This allows to preserve migration with older guests in the
> case of a pending hotplug request. This will cause migration to fail
> if the destination can't handle the subsection, but this is better
> than ending with an inconsistency.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_drc.c |   27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index d512ac6e1e7f..6f5cab70fc6b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -455,6 +455,22 @@ void spapr_drc_reset(SpaprDrc *drc)
>      }
>  }
>  
> +static bool spapr_drc_unplug_requested_needed(void *opaque)
> +{
> +    return spapr_drc_unplug_requested(opaque);
> +}
> +
> +static const VMStateDescription vmstate_spapr_drc_unplug_requested = {
> +    .name = "spapr_drc/unplug_requested",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_drc_unplug_requested_needed,
> +    .fields  = (VMStateField []) {
> +        VMSTATE_BOOL(unplug_requested, SpaprDrc),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static bool spapr_drc_needed(void *opaque)
>  {
>      SpaprDrc *drc = (SpaprDrc *)opaque;
> @@ -467,8 +483,11 @@ static bool spapr_drc_needed(void *opaque)
>      /*
>       * We need to migrate the state if it's not equal to the expected
>       * long-term state, which is the same as the coldplugged initial
> -     * state */
> -    return !spapr_drc_device_ready(drc);
> +     * state, or if an unplug request is pending.
> +     */
> +    return
> +        spapr_drc_unplug_requested_needed(drc) ||
> +        !spapr_drc_device_ready(drc);

Hrm.  You start the series by splitting spapr_drc_device_ready() from
spapr_drc_needed().  But at this point, I'm pretty sure you've now got
all the callers of spapr_drc_device_ready() doing equivalent logic
about them, so they might as well be one function again.  That seems
pretty roundabout.

I don't think the rationale for not using the drc_ready function from
the CAS path really makes sense.  It's not just an accident that those
use the same logic - in both cases what we're testing is "Is the DRC
in a state other than that of a default cold-plugged device?".

Changing the name might be sensible, but I still think we want a
common function for the two cases.

>  }
>  
>  static const VMStateDescription vmstate_spapr_drc = {
> @@ -479,6 +498,10 @@ static const VMStateDescription vmstate_spapr_drc = {
>      .fields  = (VMStateField []) {
>          VMSTATE_UINT32(state, SpaprDrc),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_spapr_drc_unplug_requested,
> +        NULL
>      }
>  };
>  
> 

-- 
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/3] spapr: Fix device unplug vs CAS or migration

[David Gibson]

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

On Thu, Feb 13, 2020 at 04:10:55PM +0100, Greg Kurz wrote:
> Ping ?
> 
> This series fixes actual bugs. Also, I have another patch on top of
> that to cold plug (or remove) devices pending hot plug (or unplug)
> before CAS, hence removing the need for CAS reboot in these cases.
> This requires SLOF to correctly parse the FDT it gets at CAS. Patches
> have been sent for that too:
> 
> https://git.qemu.org/?p=SLOF.git;a=commitdiff;h=689ff6f6554d94fdab854bf4fc4ec85e2675e43d
> https://git.qemu.org/?p=SLOF.git;a=commitdiff;h=a093be1ebe7a48321646601d94be6cf735c81e12
> https://patchwork.ozlabs.org/patch/1235817/

Yeah, sorry, I've been having a bit of trouble getting my head around
the cases here.  I've sent a comment now.

> 
> On Mon, 03 Feb 2020 23:36:04 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > While working on getting rid of CAS reboot, I realized that we currently
> > don't handle device hot unplug properly in the following situations:
> > 
> > 1) if the device is unplugged between boot and CAS, SLOF doesn't handle
> >    the even, which is a known limitation. The device hence stays around
> >    forever (specifically, until some other event is emitted and the guest
> >    eventually completes the unplug or a reboot). Until we can teach SLOF
> >    to correctly process the full FDT at CAS, we should trigger a CAS reboot,
> >    like we already do for hotplug.
> > 
> > 2) if the guest is migrated after the even was emitted but before the
> >    guest could process it, the destination is unaware of the pending
> >    unplug operation and doesn't remove the device when the guests
> >    releases it. The 'unplug_requested' field of the DRC is actually state
> >    that should be migrated.
> > 
> 

-- 
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 3/3] spapr: Migrate SpaprDrc::unplug_requested

[Greg Kurz]

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

On Fri, 14 Feb 2020 13:29:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 03, 2020 at 11:36:22PM +0100, Greg Kurz wrote:
> > Hot unplugging a device is an asynchronous operation. If the guest is
> > migrated after the event was sent but before it could release the
> > device with RTAS, the destination QEMU doesn't know about the pending
> > unplug operation and doesn't actually remove the device when the guest
> > finally releases it. The device
> > 
> > Migrate SpaprDrc::unplug_requested to fix the inconsistency. This is
> > done with a subsection that is only sent if an unplug request is
> > pending. This allows to preserve migration with older guests in the
> > case of a pending hotplug request. This will cause migration to fail
> > if the destination can't handle the subsection, but this is better
> > than ending with an inconsistency.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr_drc.c |   27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index d512ac6e1e7f..6f5cab70fc6b 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -455,6 +455,22 @@ void spapr_drc_reset(SpaprDrc *drc)
> >      }
> >  }
> >  
> > +static bool spapr_drc_unplug_requested_needed(void *opaque)
> > +{
> > +    return spapr_drc_unplug_requested(opaque);
> > +}
> > +
> > +static const VMStateDescription vmstate_spapr_drc_unplug_requested = {
> > +    .name = "spapr_drc/unplug_requested",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_drc_unplug_requested_needed,
> > +    .fields  = (VMStateField []) {
> > +        VMSTATE_BOOL(unplug_requested, SpaprDrc),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static bool spapr_drc_needed(void *opaque)
> >  {
> >      SpaprDrc *drc = (SpaprDrc *)opaque;
> > @@ -467,8 +483,11 @@ static bool spapr_drc_needed(void *opaque)
> >      /*
> >       * We need to migrate the state if it's not equal to the expected
> >       * long-term state, which is the same as the coldplugged initial
> > -     * state */
> > -    return !spapr_drc_device_ready(drc);
> > +     * state, or if an unplug request is pending.
> > +     */
> > +    return
> > +        spapr_drc_unplug_requested_needed(drc) ||
> > +        !spapr_drc_device_ready(drc);
> 
> Hrm.  You start the series by splitting spapr_drc_device_ready() from
> spapr_drc_needed().  But at this point, I'm pretty sure you've now got
> all the callers of spapr_drc_device_ready() doing equivalent logic
> about them, so they might as well be one function again.  That seems
> pretty roundabout.
> 

Yeah... I did the split because an earlier draft of this series had
a separate path at some point for the plug and unplug cases... but
I agree these should be reunited.

> I don't think the rationale for not using the drc_ready function from
> the CAS path really makes sense.  It's not just an accident that those
> use the same logic - in both cases what we're testing is "Is the DRC
> in a state other than that of a default cold-plugged device?".
>

"Is the DRC in a state other than that of a default cold-plugged device
or is an unplug request pending ?" since the DRC of the device to be
unplugged only transitions away from the "ready state" when the guest
asks to isolate the device with the "set-indicator" RTAS call.

> Changing the name might be sensible, but I still think we want a
> common function for the two cases.
> 

I'll go for that. Maybe reverse the semantics, like if "the DRC has
no attached device or it has an attached device without pending unplug
request" then it is in a steady state that doesn't require anything
special at CAS or migration time, eg. spapr_drc_steady() ?

> >  }
> >  
> >  static const VMStateDescription vmstate_spapr_drc = {
> > @@ -479,6 +498,10 @@ static const VMStateDescription vmstate_spapr_drc = {
> >      .fields  = (VMStateField []) {
> >          VMSTATE_UINT32(state, SpaprDrc),
> >          VMSTATE_END_OF_LIST()
> > +    },
> > +    .subsections = (const VMStateDescription * []) {
> > +        &vmstate_spapr_drc_unplug_requested,
> > +        NULL
> >      }
> >  };
> >  
> > 
> 


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