On Thu, 12 Jul 2018 14:40:34 +1000
David Gibson <da...@gibson.dropbear.id.au> wrote:

> On Wed, Jul 11, 2018 at 11:26:41AM +0200, Greg Kurz wrote:
> > On Wed, 11 Jul 2018 11:28:02 +1000
> > David Gibson <da...@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Jul 10, 2018 at 05:55:14PM +0200, Greg Kurz wrote:  
> > > > Recent cleanup in commit a028dd423ee6 causes QEMU to crash during CPU
> > > > hotplug:
> > > > 
> > > > (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> > > > Segmentation fault (core dumped)
> > > > 
> > > > When the hotplug path tries to reset the ICP device, we end
> > > > up calling:
> > > > 
> > > > static void icp_kvm_reset(DeviceState *dev)
> > > > {
> > > >     ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > > > 
> > > >     icpc->parent_reset(dev);
> > > > 
> > > > but icpc->parent_reset is NULL.    
> > > 
> > > This seems terribly complex to address this symptom.  Couldn't we just
> > > do a
> > > 
> > >   if (icpc->parent_reset)
> > >           icpc->parent_reset(dev);
> > > 
> > > ?
> > >   
> > 
> > This would certainly avoid the SEGV but this would mean that
> > we skip icp_reset() for hot plugged ICPs... and it doesn't
> > address the fact that icp_kvm_reset() is never called for cold
> > plugged ones.  
> 
> Ah, yeah, duh.
> 
> > Commit a028dd423ee6 inverted the control: icp_reset() no
> > longer calls per-ICP type reset function. It is up to each
> > type to call icp_reset().  
> 
> Hrm.. could we have icp_realize() do a qemu_register_reset() on
> dc->reset, rather than on icp_reset() directly.  And make sure the
> base class sets dc->reset correctly too, of course.
> 
> I think that'll accomplish what we want without rearranging the class
> structure again.
> 

Yeah, that should work.


> > Maybe I can split the patch to ease review. But if you're really
> > seeking simplicity, the best call is probably to simply revert
> > a028dd423ee6 at this point.  
> 
> 
> 
> 
> >   
> > > > Indeed, the TYPE_ICP class doesn't implement DeviceClass::reset, but
> > > > rather directly registers a reset handler with qemu_register_reset().
> > > > This is done this way because ICPs aren't SysBus devices but we want
> > > > machine reset to reset them anyway (commit 7ea6e0671754).
> > > > 
> > > > The crash also proves that ipc_kvm_reset() is obviously not called for
> > > > cold plugged devices, ie, TYPE_ICP_KVM should register its own handler
> > > > with qemu_register_reset().
> > > > 
> > > > The motivation of commit a028dd423ee6 to have cleaner object patterns
> > > > is good, but it means that all specialized ICP types should register
> > > > their own reset handler to be called at machine reset.
> > > > 
> > > > So this patchs converts the current TYPE_ICP class into an abstract
> > > > TYPE_ICP_BASE class that implements DeviceClass::reset instead of
> > > > calling qemu_register_reset().
> > > > 
> > > > The _new_ TYPE_ICP class is derived from TYPE_ICP_BASE to cover the
> > > > the pseries.kernel-irqchip=off case. It merely registers a reset
> > > > handler with qemu_register_reset(). Its device class functions
> > > > are renamed as icp_simple_* to avoid confusion with the base class.
> > > > 
> > > > All other specialized ICP types are also converted to register their
> > > > own reset handler as well.
> > > > 
> > > > This matches what we already have with ICS.
> > > > 
> > > > Since we support CPU hot unplug with spapr, unrealize functions
> > > > are added to TYPE_ICP and TYPE_ICP_KVM so that they can call
> > > > qemu_unregister_reset().
> > > > 
> > > > Reported-by: Satheesh Rajendran <sathn...@linux.vnet.ibm.com>
> > > > Fixes: a028dd423ee6dfd091a8c63028240832bf10f671
> > > > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > > > ---
> > > > 
> > > > I've successfully tested the following:
> > > > - pseries with in-kernel XICS
> > > > - pseries with emulated XICS
> > > > - powernv
> > > > - migration of pseries, including migration to QEMU 2.12 and back
> > > > ---
> > > >  hw/intc/xics.c        |   97 
> > > > ++++++++++++++++++++++++++++++++++++++++---------
> > > >  hw/intc/xics_kvm.c    |   27 +++++++++++---
> > > >  hw/intc/xics_pnv.c    |   27 +++++++++++---
> > > >  include/hw/ppc/xics.h |   12 ++++--
> > > >  4 files changed, 129 insertions(+), 34 deletions(-)
> > > > 
> > > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > > > index b9f1a3c97214..b478b9120062 100644
> > > > --- a/hw/intc/xics.c
> > > > +++ b/hw/intc/xics.c
> > > > @@ -40,7 +40,7 @@
> > > >  
> > > >  void icp_pic_print_info(ICPState *icp, Monitor *mon)
> > > >  {
> > > > -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(icp);
> > > >      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> > > >  
> > > >      if (!icp->output) {
> > > > @@ -255,7 +255,7 @@ static void icp_irq(ICSState *ics, int server, int 
> > > > nr, uint8_t priority)
> > > >  static int icp_dispatch_pre_save(void *opaque)
> > > >  {
> > > >      ICPState *icp = opaque;
> > > > -    ICPStateClass *info = ICP_GET_CLASS(icp);
> > > > +    ICPStateClass *info = ICP_BASE_GET_CLASS(icp);
> > > >  
> > > >      if (info->pre_save) {
> > > >          info->pre_save(icp);
> > > > @@ -267,7 +267,7 @@ static int icp_dispatch_pre_save(void *opaque)
> > > >  static int icp_dispatch_post_load(void *opaque, int version_id)
> > > >  {
> > > >      ICPState *icp = opaque;
> > > > -    ICPStateClass *info = ICP_GET_CLASS(icp);
> > > > +    ICPStateClass *info = ICP_BASE_GET_CLASS(icp);
> > > >  
> > > >      if (info->post_load) {
> > > >          return info->post_load(icp, version_id);
> > > > @@ -291,9 +291,9 @@ static const VMStateDescription vmstate_icp_server 
> > > > = {
> > > >      },
> > > >  };
> > > >  
> > > > -static void icp_reset(void *dev)
> > > > +static void icp_base_reset(DeviceState *dev)
> > > >  {
> > > > -    ICPState *icp = ICP(dev);
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > >  
> > > >      icp->xirr = 0;
> > > >      icp->pending_priority = 0xff;
> > > > @@ -303,9 +303,9 @@ static void icp_reset(void *dev)
> > > >      qemu_set_irq(icp->output, 0);
> > > >  }
> > > >  
> > > > -static void icp_realize(DeviceState *dev, Error **errp)
> > > > +static void icp_base_realize(DeviceState *dev, Error **errp)
> > > >  {
> > > > -    ICPState *icp = ICP(dev);
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > >      PowerPCCPU *cpu;
> > > >      CPUPPCState *env;
> > > >      Object *obj;
> > > > @@ -345,31 +345,91 @@ static void icp_realize(DeviceState *dev, Error 
> > > > **errp)
> > > >          return;
> > > >      }
> > > >  
> > > > -    qemu_register_reset(icp_reset, dev);
> > > >      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, 
> > > > icp);
> > > >  }
> > > >  
> > > > -static void icp_unrealize(DeviceState *dev, Error **errp)
> > > > +static void icp_base_unrealize(DeviceState *dev, Error **errp)
> > > >  {
> > > > -    ICPState *icp = ICP(dev);
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > >  
> > > >      vmstate_unregister(NULL, &vmstate_icp_server, icp);
> > > > -    qemu_unregister_reset(icp_reset, dev);
> > > >  }
> > > >  
> > > > -static void icp_class_init(ObjectClass *klass, void *data)
> > > > +static void icp_base_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > >  
> > > > -    dc->realize = icp_realize;
> > > > -    dc->unrealize = icp_unrealize;
> > > > +    dc->realize = icp_base_realize;
> > > > +    dc->unrealize = icp_base_unrealize;
> > > > +    dc->reset = icp_base_reset;
> > > >  }
> > > >  
> > > > -static const TypeInfo icp_info = {
> > > > -    .name = TYPE_ICP,
> > > > +static const TypeInfo icp_base_info = {
> > > > +    .name = TYPE_ICP_BASE,
> > > >      .parent = TYPE_DEVICE,
> > > >      .instance_size = sizeof(ICPState),
> > > > -    .class_init = icp_class_init,
> > > > +    .class_init = icp_base_class_init,
> > > > +    .class_size = sizeof(ICPStateClass),
> > > > +    .abstract = true,
> > > > +};
> > > > +
> > > > +static void icp_simple_reset(DeviceState *dev)
> > > > +{
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(dev);
> > > > +
> > > > +    icpc->parent_reset(dev);
> > > > +}
> > > > +
> > > > +static void icp_simple_reset_handler(void *dev)
> > > > +{
> > > > +    icp_simple_reset(dev);
> > > > +}
> > > > +
> > > > +static void icp_simple_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(dev);
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    icpc->parent_realize(dev, &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    qemu_register_reset(icp_simple_reset_handler, dev);
> > > > +}
> > > > +
> > > > +static void icp_simple_unrealize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(dev);
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    icpc->parent_unrealize(dev, &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    qemu_unregister_reset(icp_simple_reset_handler, dev);
> > > > +}
> > > > +
> > > > +static void icp_simple_class_init(ObjectClass *klass, void *data)
> > > > +{
> > > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > > +    ICPStateClass *icpc = ICP_BASE_CLASS(klass);
> > > > +
> > > > +    device_class_set_parent_realize(dc, icp_simple_realize,
> > > > +                                    &icpc->parent_realize);
> > > > +    device_class_set_parent_unrealize(dc, icp_simple_unrealize,
> > > > +                                      &icpc->parent_unrealize);
> > > > +    device_class_set_parent_reset(dc, icp_simple_reset, 
> > > > &icpc->parent_reset);
> > > > +}
> > > > +
> > > > +static const TypeInfo icp_simple_info = {
> > > > +    .name = TYPE_ICP,
> > > > +    .parent = TYPE_ICP_BASE,
> > > > +    .instance_size = sizeof(ICPState),
> > > > +    .class_init = icp_simple_class_init,
> > > >      .class_size = sizeof(ICPStateClass),
> > > >  };
> > > >  
> > > > @@ -744,7 +804,8 @@ static void xics_register_types(void)
> > > >  {
> > > >      type_register_static(&ics_simple_info);
> > > >      type_register_static(&ics_base_info);
> > > > -    type_register_static(&icp_info);
> > > > +    type_register_static(&icp_simple_info);
> > > > +    type_register_static(&icp_base_info);
> > > >      type_register_static(&xics_fabric_info);
> > > >  }
> > > >  
> > > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > > > index 30c3769a2084..72e700677059 100644
> > > > --- a/hw/intc/xics_kvm.c
> > > > +++ b/hw/intc/xics_kvm.c
> > > > @@ -116,17 +116,22 @@ static int icp_set_kvm_state(ICPState *icp, int 
> > > > version_id)
> > > >  
> > > >  static void icp_kvm_reset(DeviceState *dev)
> > > >  {
> > > > -    ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(dev);
> > > >  
> > > >      icpc->parent_reset(dev);
> > > >  
> > > > -    icp_set_kvm_state(ICP(dev), 1);
> > > > +    icp_set_kvm_state(ICP_BASE(dev), 1);
> > > > +}
> > > > +
> > > > +static void icp_kvm_reset_handler(void *dev)
> > > > +{
> > > > +    icp_kvm_reset(dev);
> > > >  }
> > > >  
> > > >  static void icp_kvm_realize(DeviceState *dev, Error **errp)
> > > >  {
> > > > -    ICPState *icp = ICP(dev);
> > > > -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(icp);
> > > >      Error *local_err = NULL;
> > > >      CPUState *cs;
> > > >      KVMEnabledICP *enabled_icp;
> > > > @@ -166,15 +171,25 @@ static void icp_kvm_realize(DeviceState *dev, 
> > > > Error **errp)
> > > >      enabled_icp = g_malloc(sizeof(*enabled_icp));
> > > >      enabled_icp->vcpu_id = vcpu_id;
> > > >      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
> > > > +    qemu_register_reset(icp_kvm_reset_handler, icp);
> > > > +}
> > > > +
> > > > +static void icp_kvm_unrealize(DeviceState *dev, Error **errp)
> > > > +{
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > > +
> > > > +    qemu_unregister_reset(icp_kvm_reset_handler, icp);
> > > >  }
> > > >  
> > > >  static void icp_kvm_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > > -    ICPStateClass *icpc = ICP_CLASS(klass);
> > > > +    ICPStateClass *icpc = ICP_BASE_CLASS(klass);
> > > >  
> > > >      device_class_set_parent_realize(dc, icp_kvm_realize,
> > > >                                      &icpc->parent_realize);
> > > > +    device_class_set_parent_unrealize(dc, icp_kvm_unrealize,
> > > > +                                      &icpc->parent_unrealize);
> > > >      device_class_set_parent_reset(dc, icp_kvm_reset,
> > > >                                    &icpc->parent_reset);
> > > >  
> > > > @@ -185,7 +200,7 @@ static void icp_kvm_class_init(ObjectClass *klass, 
> > > > void *data)
> > > >  
> > > >  static const TypeInfo icp_kvm_info = {
> > > >      .name = TYPE_KVM_ICP,
> > > > -    .parent = TYPE_ICP,
> > > > +    .parent = TYPE_ICP_BASE,
> > > >      .instance_size = sizeof(ICPState),
> > > >      .class_init = icp_kvm_class_init,
> > > >      .class_size = sizeof(ICPStateClass),
> > > > diff --git a/hw/intc/xics_pnv.c b/hw/intc/xics_pnv.c
> > > > index fa48505f365e..a29ccb68df73 100644
> > > > --- a/hw/intc/xics_pnv.c
> > > > +++ b/hw/intc/xics_pnv.c
> > > > @@ -33,7 +33,7 @@
> > > >  
> > > >  static uint64_t pnv_icp_read(void *opaque, hwaddr addr, unsigned width)
> > > >  {
> > > > -    ICPState *icp = ICP(opaque);
> > > > +    ICPState *icp = ICP_BASE(opaque);
> > > >      PnvICPState *picp = PNV_ICP(opaque);
> > > >      bool byte0 = (width == 1 && (addr & 0x3) == 0);
> > > >      uint64_t val = 0xffffffff;
> > > > @@ -96,7 +96,7 @@ bad_access:
> > > >  static void pnv_icp_write(void *opaque, hwaddr addr, uint64_t val,
> > > >                                unsigned width)
> > > >  {
> > > > -    ICPState *icp = ICP(opaque);
> > > > +    ICPState *icp = ICP_BASE(opaque);
> > > >      PnvICPState *picp = PNV_ICP(opaque);
> > > >      bool byte0 = (width == 1 && (addr & 0x3) == 0);
> > > >  
> > > > @@ -145,6 +145,18 @@ bad_access:
> > > >      }
> > > >  }
> > > >  
> > > > +static void pnv_icp_reset(DeviceState *dev)
> > > > +{
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(dev);
> > > > +
> > > > +    icpc->parent_reset(dev);
> > > > +}
> > > > +
> > > > +static void pnv_icp_reset_handler(void *dev)
> > > > +{
> > > > +    pnv_icp_reset(dev);
> > > > +}
> > > > +
> > > >  static const MemoryRegionOps pnv_icp_ops = {
> > > >      .read = pnv_icp_read,
> > > >      .write = pnv_icp_write,
> > > > @@ -161,9 +173,9 @@ static const MemoryRegionOps pnv_icp_ops = {
> > > >  
> > > >  static void pnv_icp_realize(DeviceState *dev, Error **errp)
> > > >  {
> > > > -    ICPState *icp = ICP(dev);
> > > > +    ICPState *icp = ICP_BASE(dev);
> > > >      PnvICPState *pnv_icp = PNV_ICP(icp);
> > > > -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > > > +    ICPStateClass *icpc = ICP_BASE_GET_CLASS(icp);
> > > >      Error *local_err = NULL;
> > > >  
> > > >      icpc->parent_realize(dev, &local_err);
> > > > @@ -174,21 +186,24 @@ static void pnv_icp_realize(DeviceState *dev, 
> > > > Error **errp)
> > > >  
> > > >      memory_region_init_io(&pnv_icp->mmio, OBJECT(icp), &pnv_icp_ops,
> > > >                            icp, "icp-thread", 0x1000);
> > > > +    qemu_register_reset(pnv_icp_reset_handler, icp);
> > > >  }
> > > >  
> > > >  static void pnv_icp_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > > -    ICPStateClass *icpc = ICP_CLASS(klass);
> > > > +    ICPStateClass *icpc = ICP_BASE_CLASS(klass);
> > > >  
> > > >      device_class_set_parent_realize(dc, pnv_icp_realize,
> > > >                                      &icpc->parent_realize);
> > > > +    device_class_set_parent_reset(dc, pnv_icp_reset,
> > > > +                                  &icpc->parent_reset);
> > > >      dc->desc = "PowerNV ICP";
> > > >  }
> > > >  
> > > >  static const TypeInfo pnv_icp_info = {
> > > >      .name          = TYPE_PNV_ICP,
> > > > -    .parent        = TYPE_ICP,
> > > > +    .parent        = TYPE_ICP_BASE,
> > > >      .instance_size = sizeof(PnvICPState),
> > > >      .class_init    = pnv_icp_class_init,
> > > >      .class_size    = sizeof(ICPStateClass),
> > > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > > > index 6ac8a9392da6..6a2e45997f8f 100644
> > > > --- a/include/hw/ppc/xics.h
> > > > +++ b/include/hw/ppc/xics.h
> > > > @@ -48,6 +48,9 @@ typedef struct ICSState ICSState;
> > > >  typedef struct ICSIRQState ICSIRQState;
> > > >  typedef struct XICSFabric XICSFabric;
> > > >  
> > > > +#define TYPE_ICP_BASE "icp-base"
> > > > +#define ICP_BASE(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_BASE)
> > > > +
> > > >  #define TYPE_ICP "icp"
> > > >  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
> > > >  
> > > > @@ -57,15 +60,16 @@ typedef struct XICSFabric XICSFabric;
> > > >  #define TYPE_PNV_ICP "pnv-icp"
> > > >  #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
> > > >  
> > > > -#define ICP_CLASS(klass) \
> > > > -     OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
> > > > -#define ICP_GET_CLASS(obj) \
> > > > -     OBJECT_GET_CLASS(ICPStateClass, (obj), TYPE_ICP)
> > > > +#define ICP_BASE_CLASS(klass) \
> > > > +     OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP_BASE)
> > > > +#define ICP_BASE_GET_CLASS(obj) \
> > > > +     OBJECT_GET_CLASS(ICPStateClass, (obj), TYPE_ICP_BASE)
> > > >  
> > > >  struct ICPStateClass {
> > > >      DeviceClass parent_class;
> > > >  
> > > >      DeviceRealize parent_realize;
> > > > +    DeviceUnrealize parent_unrealize;
> > > >      DeviceReset parent_reset;
> > > >  
> > > >      void (*pre_save)(ICPState *icp);
> > > >     
> > >   
> >   
> 
> 
> 

Attachment: pgp7KSs_rpcOf.pgp
Description: OpenPGP digital signature

Reply via email to