Re: [PATCH for-6.0] spapr: Allow memory unplug to always succeed

[Greg Kurz]

On Tue, 8 Dec 2020 15:30:04 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Dec 07, 2020 at 02:37:04PM +0100, Greg Kurz wrote:
> > It is currently impossible to hot-unplug a memory device between
> > machine reset and CAS.
> > 
> > (qemu) device_del dimm1
> > Error: Memory hot unplug not supported for this guest
> > 
> > This limitation was introduced in order to provide an explicit
> > error path for older guests that didn't support hot-plug event
> > sources (and thus memory hot-unplug).
> > 
> > The linux kernel has been supporting these since 4.11. All recent
> > enough guests are thus capable of handling the removal of a memory
> > device at all time, including during early boot.
> > 
> > Lift the limitation for the latest machine type. This means that
> > trying to unplug memory from a guest that doesn't support it will
> > likely just do nothing and the memory will only get removed at
> > next reboot. Such older guests can still get the existing behavior
> > by using an older machine type.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Looks like this conflicts with something I've added to for-6.0
> recently.  Can you rebase and resend, please.
> 

I'm not quite sure what for-6.0 you're talking about. Despite
you're recent announcement about moving to gitlab, it seems
that the branch at github is the most up to date.

gitlab:
- HEAD is "xive: Add trace events"
- Date: 26 Nov, 2020

github:
- HEAD is "MAINTAINERS: Add Greg Kurz as co-maintainer for ppc"
- Date: Dec 4, 2020

I've thus based this patch on github. Also, this is based on Connie's
"hw: add compat machines for 6.0" patch...

> > ---
> > Based-on: 20201109173928.1001764-1-cohuck@redhat.com

... maybe I should have made it more clear than just
mentioning the message id ?

I think I'll just wait for Connie's patch to get merged and I'll repost after
you've rebased ppc-for-6.0.

> > ---
> >  include/hw/ppc/spapr.h | 1 +
> >  hw/ppc/spapr.c         | 6 +++++-
> >  hw/ppc/spapr_events.c  | 3 ++-
> >  3 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index b7ced9faebf5..7aa630350326 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -139,6 +139,7 @@ struct SpaprMachineClass {
> >      hwaddr rma_limit;          /* clamp the RMA to this size */
> >      bool pre_5_1_assoc_refpoints;
> >      bool pre_5_2_numa_associativity;
> > +    bool pre_6_0_memory_unplug;
> >  
> >      bool (*phb_placement)(SpaprMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7e954bc84bed..f0b26b2af30d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -4044,7 +4044,8 @@ static void 
> > spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> >      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >  
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > -        if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> > +        if (!smc->pre_6_0_memory_unplug ||
> > +            spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> >              spapr_memory_unplug_request(hotplug_dev, dev, errp);
> >          } else {
> >              /* NOTE: this means there is a window after guest reset, prior 
> > to
> > @@ -4530,8 +4531,11 @@ DEFINE_SPAPR_MACHINE(6_0, "6.0", true);
> >   */
> >  static void spapr_machine_5_2_class_options(MachineClass *mc)
> >  {
> > +    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> >      spapr_machine_6_0_class_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> > +    smc->pre_6_0_memory_unplug = true;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(5_2, "5.2", false);
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 1add53547ec3..c30123177b16 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -659,7 +659,8 @@ static void spapr_hotplug_req_event(uint8_t hp_id, 
> > uint8_t hp_action,
> >          /* we should not be using count_indexed value unless the guest
> >           * supports dedicated hotplug event source
> >           */
> > -        g_assert(spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
> > +        g_assert(!SPAPR_MACHINE_GET_CLASS(spapr)->pre_6_0_memory_unplug ||
> > +                 spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT));
> >          hp->drc_id.count_indexed.count =
> >              cpu_to_be32(drc_id->count_indexed.count);
> >          hp->drc_id.count_indexed.index =
>

pgp64_Tpg65LK.pgp
Description: OpenPGP digital signature