* [PATCH] spapr: Don't request to unplug the same core twice @ 2019-10-23 19:17 Greg Kurz 2019-10-23 22:38 ` David Gibson 0 siblings, 1 reply; 4+ messages in thread From: Greg Kurz @ 2019-10-23 19:17 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, qemu-devel We must not call spapr_drc_detach() on a detached DRC otherwise bad things can happen, ie. QEMU hangs or crashes. This is easily demonstrated with a CPU hotplug/unplug loop using QMP. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index f9410d390a07..94f9d27096af 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3741,9 +3741,10 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, spapr_vcpu_id(spapr, cc->core_id)); g_assert(drc); - spapr_drc_detach(drc); - - spapr_hotplug_req_remove_by_index(drc); + if (!spapr_drc_unplug_requested(drc)) { + spapr_drc_detach(drc); + spapr_hotplug_req_remove_by_index(drc); + } } int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] spapr: Don't request to unplug the same core twice 2019-10-23 19:17 [PATCH] spapr: Don't request to unplug the same core twice Greg Kurz @ 2019-10-23 22:38 ` David Gibson 2019-10-24 6:28 ` Greg Kurz 0 siblings, 1 reply; 4+ messages in thread From: David Gibson @ 2019-10-23 22:38 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1343 bytes --] On Wed, Oct 23, 2019 at 09:17:40PM +0200, Greg Kurz wrote: > We must not call spapr_drc_detach() on a detached DRC otherwise bad things > can happen, ie. QEMU hangs or crashes. This is easily demonstrated with > a CPU hotplug/unplug loop using QMP. > > Signed-off-by: Greg Kurz <groug@kaod.org> Ouch, good catch. Applied. I wonder if we have the same problem with other DRC types. > --- > hw/ppc/spapr.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f9410d390a07..94f9d27096af 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3741,9 +3741,10 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > spapr_vcpu_id(spapr, cc->core_id)); > g_assert(drc); > > - spapr_drc_detach(drc); > - > - spapr_hotplug_req_remove_by_index(drc); > + if (!spapr_drc_unplug_requested(drc)) { > + spapr_drc_detach(drc); > + spapr_hotplug_req_remove_by_index(drc); > + } > } > > int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] spapr: Don't request to unplug the same core twice 2019-10-23 22:38 ` David Gibson @ 2019-10-24 6:28 ` Greg Kurz 2019-10-28 22:20 ` David Gibson 0 siblings, 1 reply; 4+ messages in thread From: Greg Kurz @ 2019-10-24 6:28 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2124 bytes --] On Thu, 24 Oct 2019 09:38:17 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Oct 23, 2019 at 09:17:40PM +0200, Greg Kurz wrote: > > We must not call spapr_drc_detach() on a detached DRC otherwise bad things > > can happen, ie. QEMU hangs or crashes. This is easily demonstrated with > > a CPU hotplug/unplug loop using QMP. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Ouch, good catch. Applied. > > I wonder if we have the same problem with other DRC types. > We don't have it with PHB and PCI types, through the same use of spapr_drc_unplug_requested(). LMBs see to avoid it by failing device_del early if an unplug request is already in progress: /* * An existing pending dimm state for this DIMM means that there is an * unplug operation in progress, waiting for the spapr_lmb_release * callback to complete the job (BQL can't cover that far). In this case, * bail out to avoid detaching DRCs that were already released. */ if (spapr_pending_dimm_unplugs_find(spapr, dimm)) { error_setg(&local_err, "Memory unplug already in progress for device %s", dev->id); goto out; } Not sure why we error out in this case instead of ignoring the unplug request. > > --- > > hw/ppc/spapr.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index f9410d390a07..94f9d27096af 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3741,9 +3741,10 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > > spapr_vcpu_id(spapr, cc->core_id)); > > g_assert(drc); > > > > - spapr_drc_detach(drc); > > - > > - spapr_hotplug_req_remove_by_index(drc); > > + if (!spapr_drc_unplug_requested(drc)) { > > + spapr_drc_detach(drc); > > + spapr_hotplug_req_remove_by_index(drc); > > + } > > } > > > > int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, > > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] spapr: Don't request to unplug the same core twice 2019-10-24 6:28 ` Greg Kurz @ 2019-10-28 22:20 ` David Gibson 0 siblings, 0 replies; 4+ messages in thread From: David Gibson @ 2019-10-28 22:20 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2811 bytes --] On Thu, Oct 24, 2019 at 08:28:54AM +0200, Greg Kurz wrote: > On Thu, 24 Oct 2019 09:38:17 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Oct 23, 2019 at 09:17:40PM +0200, Greg Kurz wrote: > > > We must not call spapr_drc_detach() on a detached DRC otherwise bad things > > > can happen, ie. QEMU hangs or crashes. This is easily demonstrated with > > > a CPU hotplug/unplug loop using QMP. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Ouch, good catch. Applied. > > > > I wonder if we have the same problem with other DRC types. > > > > We don't have it with PHB and PCI types, through the same use of > spapr_drc_unplug_requested(). > > LMBs see to avoid it by failing device_del early if an unplug > request is already in progress: > > /* > * An existing pending dimm state for this DIMM means that there is an > * unplug operation in progress, waiting for the spapr_lmb_release > * callback to complete the job (BQL can't cover that far). In this case, > * bail out to avoid detaching DRCs that were already released. > */ > if (spapr_pending_dimm_unplugs_find(spapr, dimm)) { > error_setg(&local_err, > "Memory unplug already in progress for device %s", > dev->id); > goto out; > } > > Not sure why we error out in this case instead of ignoring the unplug > request. I suspect no particularly good reason, just history. Everything's a bit different with LMBs because a single device_del will usually remove a whole bunch of LMB DRCs. In general the interfacing between the qemu user side DIMM handling and the PAPR side LMB/DRC handling is... pretty clunky. > > > > --- > > > hw/ppc/spapr.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index f9410d390a07..94f9d27096af 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -3741,9 +3741,10 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > > > spapr_vcpu_id(spapr, cc->core_id)); > > > g_assert(drc); > > > > > > - spapr_drc_detach(drc); > > > - > > > - spapr_hotplug_req_remove_by_index(drc); > > > + if (!spapr_drc_unplug_requested(drc)) { > > > + spapr_drc_detach(drc); > > > + spapr_hotplug_req_remove_by_index(drc); > > > + } > > > } > > > > > > int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-29 8:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-23 19:17 [PATCH] spapr: Don't request to unplug the same core twice Greg Kurz 2019-10-23 22:38 ` David Gibson 2019-10-24 6:28 ` Greg Kurz 2019-10-28 22:20 ` David Gibson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.