[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [FIX PATCH v2] spapr: Fix QEMU abort during memory unplug
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [FIX PATCH v2] spapr: Fix QEMU abort during memory unplug |
Date: |
Fri, 21 Jul 2017 16:57:36 +1000 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Fri, Jul 21, 2017 at 10:21:06AM +0530, Bharata B Rao wrote:
> Commit 0cffce56 (hw/ppc/spapr.c: adding pending_dimm_unplugs to
> sPAPRMachineState) introduced a new way to track pending LMBs of DIMM
> device that is marked for removal. Since this commit we can hit the
> assert in spapr_pending_dimm_unplugs_add() in the following situation:
>
> - DIMM device removal fails as the guest doesn't allow the removal.
> - Subsequent attempt to remove the same DIMM would hit the assert
> as the corresponding sPAPRDIMMState is still part of the
> pending_dimm_unplugs list.
>
> Fix this by removing the assert and conditionally adding the
> sPAPRDIMMState to pending_dimm_unplugs list only when it is not
> already present.
>
> Fixes: 0cffce56ae3501c5783d779f97993ce478acf856
> Signed-off-by: Bharata B Rao <address@hidden>
Applied to ppc-for-2.10.
> ---
> Changes in v2:
> - sPAPRDIMMState is now allocated within spapr_pending_dimm_unplugs_add()
> itself (David Gibson)
> - spapr_recover_pending_dimm_state() should never return a NULL
> sPAPRDIMMState,
> added an assert for the same.
>
> hw/ppc/spapr.c | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1cb09e7..2465b27 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2850,11 +2850,25 @@ static sPAPRDIMMState
> *spapr_pending_dimm_unplugs_find(sPAPRMachineState *s,
> return dimm_state;
> }
>
> -static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
> - sPAPRDIMMState *dimm_state)
> +static sPAPRDIMMState *spapr_pending_dimm_unplugs_add(sPAPRMachineState
> *spapr,
> + uint32_t nr_lmbs,
> + PCDIMMDevice *dimm)
> {
> - g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm));
> - QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> + sPAPRDIMMState *ds = NULL;
> +
> + /*
> + * If this request is for a DIMM whose removal had failed earlier
> + * (due to guest's refusal to remove the LMBs), we would have this
> + * dimm already in the pending_dimm_unplugs list. In that
> + * case don't add again.
> + */
> + if (!spapr_pending_dimm_unplugs_find(spapr, dimm)) {
> + ds = g_malloc0(sizeof(sPAPRDIMMState));
> + ds->nr_lmbs = nr_lmbs;
> + ds->dimm = dimm;
> + QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, ds, next);
> + }
> + return ds;
> }
>
> static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
> @@ -2875,7 +2889,6 @@ static sPAPRDIMMState
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> uint32_t avail_lmbs = 0;
> uint64_t addr_start, addr;
> int i;
> - sPAPRDIMMState *ds;
>
> addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &error_abort);
> @@ -2891,11 +2904,7 @@ static sPAPRDIMMState
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> }
>
> - ds = g_malloc0(sizeof(sPAPRDIMMState));
> - ds->nr_lmbs = avail_lmbs;
> - ds->dimm = dimm;
> - spapr_pending_dimm_unplugs_add(ms, ds);
> - return ds;
> + return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
> }
>
> /* Callback to be called during DRC release. */
> @@ -2911,6 +2920,7 @@ void spapr_lmb_release(DeviceState *dev)
> * during the unplug process. In this case recover it. */
> if (ds == NULL) {
> ds = spapr_recover_pending_dimm_state(spapr, PC_DIMM(dev));
> + g_assert(ds);
> /* The DRC being examined by the caller at least must be counted */
> g_assert(ds->nr_lmbs);
> }
> @@ -2942,18 +2952,13 @@ static void
> spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> uint64_t addr_start, addr;
> int i;
> sPAPRDRConnector *drc;
> - sPAPRDIMMState *ds;
> -
> addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> if (local_err) {
> goto out;
> }
>
> - ds = g_malloc0(sizeof(sPAPRDIMMState));
> - ds->nr_lmbs = nr_lmbs;
> - ds->dimm = dimm;
> - spapr_pending_dimm_unplugs_add(spapr, ds);
> + spapr_pending_dimm_unplugs_add(spapr, nr_lmbs, dimm);
>
> addr = addr_start;
> for (i = 0; i < nr_lmbs; i++) {
--
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