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. 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(). 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); > > >
pgpMyv1IGskac.pgp
Description: OpenPGP digital signature