[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] ppc/xics: split ICP into icp-base and icp class
|
From: |
Greg Kurz |
|
Subject: |
Re: [Qemu-ppc] [PATCH] ppc/xics: split ICP into icp-base and icp class |
|
Date: |
Thu, 12 Jul 2018 10:54:11 +0200 |
On Thu, 12 Jul 2018 14:40:34 +1000
David Gibson <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > Fixes: a028dd423ee6dfd091a8c63028240832bf10f671
> > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > ---
> > > >
> > > > 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);
> > > >
> > >
> >
>
>
>
pgp7Y8Xzo9bEP.pgp
Description: OpenPGP digital signature