On Fri, Dec 18, 2020 at 11:33:56AM +0100, Greg Kurz wrote: > Documentation of object_property_iter_init() clearly stipulates that > "it is forbidden to modify the property list while iterating". But this > is exactly what we do when resetting transient DR connectors during CAS. > The call to spapr_drc_reset() can finalize the hot-unplug sequence of a > PHB or a PCI bridge, both of which will then in turn destroy their PCI > DRCs. This could potentially invalidate the iterator. It is pure luck > that this haven't caused any issues so far. > > Change spapr_drc_reset() to return true if it caused a device to be > removed. Restart from scratch in this case. This can potentially > increase the overall DRC reset time, especially with a high maxmem > which generates a lot of LMB DRCs. But this kind of setup is rare, > and so is the use case of rebooting a guest while doing hot-unplug. > > Signed-off-by: Greg Kurz <gr...@kaod.org>
Applied, thanks.
> ---
> include/hw/ppc/spapr_drc.h | 3 ++-
> hw/ppc/spapr_drc.c | 6 +++++-
> hw/ppc/spapr_hcall.c | 8 +++++++-
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index cff5e707d0d9..5d80019f82e2 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -224,7 +224,8 @@ static inline bool spapr_drc_hotplugged(DeviceState *dev)
> return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE);
> }
>
> -void spapr_drc_reset(SpaprDrc *drc);
> +/* Returns true if an unplug request completed */
> +bool spapr_drc_reset(SpaprDrc *drc);
>
> uint32_t spapr_drc_index(SpaprDrc *drc);
> SpaprDrcType spapr_drc_type(SpaprDrc *drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8d62f55066b6..5b5e2ac58a7e 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -417,9 +417,10 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> -void spapr_drc_reset(SpaprDrc *drc)
> +bool spapr_drc_reset(SpaprDrc *drc)
> {
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + bool unplug_completed = false;
>
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> @@ -428,6 +429,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> */
> if (drc->unplug_requested) {
> spapr_drc_release(drc);
> + unplug_completed = true;
> }
>
> if (drc->dev) {
> @@ -444,6 +446,8 @@ void spapr_drc_reset(SpaprDrc *drc)
> drc->ccs_offset = -1;
> drc->ccs_depth = -1;
> }
> +
> + return unplug_completed;
> }
>
> static bool spapr_drc_unplug_requested_needed(void *opaque)
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 4e9d50c254f0..aa22830ac4bd 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1639,6 +1639,7 @@ static void
> spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
> ObjectPropertyIterator iter;
>
> drc_container = container_get(object_get_root(), "/dr-connector");
> +restart:
> object_property_iter_init(&iter, drc_container);
> while ((prop = object_property_iter_next(&iter))) {
> SpaprDrc *drc;
> @@ -1652,8 +1653,13 @@ static void
> spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
>
> /*
> * This will complete any pending plug/unplug requests.
> + * In case of a unplugged PHB or PCI bridge, this will
> + * cause some DRCs to be destroyed and thus potentially
> + * invalidate the iterator.
> */
> - spapr_drc_reset(drc);
> + if (spapr_drc_reset(drc)) {
> + goto restart;
> + }
> }
>
> spapr_clear_pending_hotplug_events(spapr);
--
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
