Re: [Qemu-devel] [PATCH for-2.8] spapr: fix default DRC state for coldplugged LMBs

[David Gibson <david@gibson.dropbear.id.au>]

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

On Wed, Nov 30, 2016 at 05:05:34PM -0600, Michael Roth wrote:
> Currently we set the initial isolation/allocation state for DRCs
> associated with coldplugged LMBs to ISOLATED/UNUSABLE,
> respectively, under the assumption that the guest will move this
> state to UNISOLATED/USABLE.
> 
> In fact, this is only the case for LMBs added via hotplug. For
> coldplugged LMBs, the guest actually assumes the initial state to
> be UNISOLATED/USABLE.
> 
> In practice, this only becomes an issue when we attempt to unplug
> one of these LMBs, where the guest kernel will issue an
> rtas-get-sensor-state call to check that the corresponding DRC is
> in an USABLE state before it will release the LMB back to
> QEMU. If the returned state is otherwise, the guest will assume no
> further action is needed, which bypasses the QEMU-side cleanup that
> occurs during the USABLE->UNUSABLE transition. This results in
> LMBs and their corresponding pc-dimm devices to stick around
> indefinitely.
> 
> This patch fixes the issue by manually setting DRCs associated with
> cold-plugged LMBs to UNISOLATED/ALLOCATED, but leaving the hotplug
> state untouched. As it turns out, this is analogous to the handling
> for cold-plugged CPUs in spapr_core_plug().
> 
> Cc: qemu-ppc@nongnu.org
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> This patch is a revision to the previously posted series:
> 
>   [PATCH for-2.8 0/3] spapr: fix breakage of memory unplug after migration
> 
> That patchset introduced DRC migration as a means to address DRC state
> synchronization between source/target to allow memory unplug after
> migration. It turns out that unplug was also not working for LMBs
> that were defined on the source via '-device pc-dimm'. The real issue
> was that the default coldplug state on *both* the source and target was
> wrong, and by fixing that default state, we no longer need DRC migration
> for the scenario mentioned in the previous patchset since, incidentally,
> coldplug state on the target now matches post-hotplug DRC state on the
> source.
> 
> This patch is very late for 2.8, but I'm hoping it can still make it
> in for rc3/2.8.
> 
> I've tested forward/backward migration for all possible combinations
> (other than known case unsupported case 2.8->2.7) of
> v2.6/v2.7/v2.8, pseries-2.6/pseries-2.7/pseries-2.8, and
> modern-hotplug-events=true/false, with various combinations of
> CPU / LMB hotplug/unplug before/after migration.

I've merged this to ppc-for-2.8.  I'll begin my test cycle and see if
we can send a pull request in time.

> ---
>  hw/ppc/spapr.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c3269c7..208ef7b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2290,6 +2290,11 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
> +        if (!dev->hotplugged) {
> +            /* guests expect coldplugged LMBs to be pre-allocated */
> +            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> +            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> +        }
>      }
>      /* send hotplug notification to the
>       * guest only in case of hotplugged memory

-- 
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: 819 bytes --]