From nobody Fri Mar 29 14:20:12 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1531239824136237.0268657171124; Tue, 10 Jul 2018 09:23:44 -0700 (PDT) Received: from localhost ([::1]:48783 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fcvQQ-0004Y7-Kn for importer@patchew.org; Tue, 10 Jul 2018 12:23:38 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47528) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fcuzG-00078s-QJ for qemu-devel@nongnu.org; Tue, 10 Jul 2018 11:55:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fcuzD-0007lf-Az for qemu-devel@nongnu.org; Tue, 10 Jul 2018 11:55:34 -0400 Received: from 3.mo177.mail-out.ovh.net ([46.105.36.172]:36310) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fcuzC-0007lB-Uf for qemu-devel@nongnu.org; Tue, 10 Jul 2018 11:55:31 -0400 Received: from player734.ha.ovh.net (unknown [10.109.108.80]) by mo177.mail-out.ovh.net (Postfix) with ESMTP id 304D3B976C for ; Tue, 10 Jul 2018 17:55:29 +0200 (CEST) Received: from bahia.lan (lns-bzn-46-82-253-208-248.adsl.proxad.net [82.253.208.248]) (Authenticated sender: groug@kaod.org) by player734.ha.ovh.net (Postfix) with ESMTPA id 24FA62800AF; Tue, 10 Jul 2018 17:55:20 +0200 (CEST) From: Greg Kurz To: qemu-devel@nongnu.org Date: Tue, 10 Jul 2018 17:55:14 +0200 Message-ID: <153123811483.256409.15360824885082888930.stgit@bahia.lan> User-Agent: StGit/0.17.1-46-g6855-dirty MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Ovh-Tracer-Id: 15487879120265451840 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedtiedrgeeggdeliecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 46.105.36.172 Subject: [Qemu-devel] [PATCH] ppc/xics: split ICP into icp-base and icp class X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Satheesh Rajendran , Bharata B Rao , qemu-ppc@nongnu.org, =?utf-8?q?C=C3=A9dric?= Le Goater , David Gibson Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Recent cleanup in commit a028dd423ee6 causes QEMU to crash during CPU hotplug: (qemu) device_add host-spapr-cpu-core,id=3Dcore1,core-id=3D1 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 =3D ICP_GET_CLASS(dev); icpc->parent_reset(dev); but icpc->parent_reset is NULL. 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=3Doff 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 Fixes: a028dd423ee6dfd091a8c63028240832bf10f671 Signed-off-by: Greg Kurz --- 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 @@ =20 void icp_pic_print_info(ICPState *icp, Monitor *mon) { - ICPStateClass *icpc =3D ICP_GET_CLASS(icp); + ICPStateClass *icpc =3D ICP_BASE_GET_CLASS(icp); int cpu_index =3D icp->cs ? icp->cs->cpu_index : -1; =20 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 =3D opaque; - ICPStateClass *info =3D ICP_GET_CLASS(icp); + ICPStateClass *info =3D ICP_BASE_GET_CLASS(icp); =20 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 =3D opaque; - ICPStateClass *info =3D ICP_GET_CLASS(icp); + ICPStateClass *info =3D ICP_BASE_GET_CLASS(icp); =20 if (info->post_load) { return info->post_load(icp, version_id); @@ -291,9 +291,9 @@ static const VMStateDescription vmstate_icp_server =3D { }, }; =20 -static void icp_reset(void *dev) +static void icp_base_reset(DeviceState *dev) { - ICPState *icp =3D ICP(dev); + ICPState *icp =3D ICP_BASE(dev); =20 icp->xirr =3D 0; icp->pending_priority =3D 0xff; @@ -303,9 +303,9 @@ static void icp_reset(void *dev) qemu_set_irq(icp->output, 0); } =20 -static void icp_realize(DeviceState *dev, Error **errp) +static void icp_base_realize(DeviceState *dev, Error **errp) { - ICPState *icp =3D ICP(dev); + ICPState *icp =3D ICP_BASE(dev); PowerPCCPU *cpu; CPUPPCState *env; Object *obj; @@ -345,31 +345,91 @@ static void icp_realize(DeviceState *dev, Error **err= p) return; } =20 - qemu_register_reset(icp_reset, dev); vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp); } =20 -static void icp_unrealize(DeviceState *dev, Error **errp) +static void icp_base_unrealize(DeviceState *dev, Error **errp) { - ICPState *icp =3D ICP(dev); + ICPState *icp =3D ICP_BASE(dev); =20 vmstate_unregister(NULL, &vmstate_icp_server, icp); - qemu_unregister_reset(icp_reset, dev); } =20 -static void icp_class_init(ObjectClass *klass, void *data) +static void icp_base_class_init(ObjectClass *klass, void *data) { DeviceClass *dc =3D DEVICE_CLASS(klass); =20 - dc->realize =3D icp_realize; - dc->unrealize =3D icp_unrealize; + dc->realize =3D icp_base_realize; + dc->unrealize =3D icp_base_unrealize; + dc->reset =3D icp_base_reset; } =20 -static const TypeInfo icp_info =3D { - .name =3D TYPE_ICP, +static const TypeInfo icp_base_info =3D { + .name =3D TYPE_ICP_BASE, .parent =3D TYPE_DEVICE, .instance_size =3D sizeof(ICPState), - .class_init =3D icp_class_init, + .class_init =3D icp_base_class_init, + .class_size =3D sizeof(ICPStateClass), + .abstract =3D true, +}; + +static void icp_simple_reset(DeviceState *dev) +{ + ICPStateClass *icpc =3D 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 =3D ICP_BASE_GET_CLASS(dev); + Error *local_err =3D 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 =3D ICP_BASE_GET_CLASS(dev); + Error *local_err =3D 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 =3D DEVICE_CLASS(klass); + ICPStateClass *icpc =3D 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_rese= t); +} + +static const TypeInfo icp_simple_info =3D { + .name =3D TYPE_ICP, + .parent =3D TYPE_ICP_BASE, + .instance_size =3D sizeof(ICPState), + .class_init =3D icp_simple_class_init, .class_size =3D sizeof(ICPStateClass), }; =20 @@ -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); } =20 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 versi= on_id) =20 static void icp_kvm_reset(DeviceState *dev) { - ICPStateClass *icpc =3D ICP_GET_CLASS(dev); + ICPStateClass *icpc =3D ICP_BASE_GET_CLASS(dev); =20 icpc->parent_reset(dev); =20 - 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); } =20 static void icp_kvm_realize(DeviceState *dev, Error **errp) { - ICPState *icp =3D ICP(dev); - ICPStateClass *icpc =3D ICP_GET_CLASS(icp); + ICPState *icp =3D ICP_BASE(dev); + ICPStateClass *icpc =3D ICP_BASE_GET_CLASS(icp); Error *local_err =3D NULL; CPUState *cs; KVMEnabledICP *enabled_icp; @@ -166,15 +171,25 @@ static void icp_kvm_realize(DeviceState *dev, Error *= *errp) enabled_icp =3D g_malloc(sizeof(*enabled_icp)); enabled_icp->vcpu_id =3D 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 =3D ICP_BASE(dev); + + qemu_unregister_reset(icp_kvm_reset_handler, icp); } =20 static void icp_kvm_class_init(ObjectClass *klass, void *data) { DeviceClass *dc =3D DEVICE_CLASS(klass); - ICPStateClass *icpc =3D ICP_CLASS(klass); + ICPStateClass *icpc =3D ICP_BASE_CLASS(klass); =20 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); =20 @@ -185,7 +200,7 @@ static void icp_kvm_class_init(ObjectClass *klass, void= *data) =20 static const TypeInfo icp_kvm_info =3D { .name =3D TYPE_KVM_ICP, - .parent =3D TYPE_ICP, + .parent =3D TYPE_ICP_BASE, .instance_size =3D sizeof(ICPState), .class_init =3D icp_kvm_class_init, .class_size =3D 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 @@ =20 static uint64_t pnv_icp_read(void *opaque, hwaddr addr, unsigned width) { - ICPState *icp =3D ICP(opaque); + ICPState *icp =3D ICP_BASE(opaque); PnvICPState *picp =3D PNV_ICP(opaque); bool byte0 =3D (width =3D=3D 1 && (addr & 0x3) =3D=3D 0); uint64_t val =3D 0xffffffff; @@ -96,7 +96,7 @@ bad_access: static void pnv_icp_write(void *opaque, hwaddr addr, uint64_t val, unsigned width) { - ICPState *icp =3D ICP(opaque); + ICPState *icp =3D ICP_BASE(opaque); PnvICPState *picp =3D PNV_ICP(opaque); bool byte0 =3D (width =3D=3D 1 && (addr & 0x3) =3D=3D 0); =20 @@ -145,6 +145,18 @@ bad_access: } } =20 +static void pnv_icp_reset(DeviceState *dev) +{ + ICPStateClass *icpc =3D 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 =3D { .read =3D pnv_icp_read, .write =3D pnv_icp_write, @@ -161,9 +173,9 @@ static const MemoryRegionOps pnv_icp_ops =3D { =20 static void pnv_icp_realize(DeviceState *dev, Error **errp) { - ICPState *icp =3D ICP(dev); + ICPState *icp =3D ICP_BASE(dev); PnvICPState *pnv_icp =3D PNV_ICP(icp); - ICPStateClass *icpc =3D ICP_GET_CLASS(icp); + ICPStateClass *icpc =3D ICP_BASE_GET_CLASS(icp); Error *local_err =3D NULL; =20 icpc->parent_realize(dev, &local_err); @@ -174,21 +186,24 @@ static void pnv_icp_realize(DeviceState *dev, Error *= *errp) =20 memory_region_init_io(&pnv_icp->mmio, OBJECT(icp), &pnv_icp_ops, icp, "icp-thread", 0x1000); + qemu_register_reset(pnv_icp_reset_handler, icp); } =20 static void pnv_icp_class_init(ObjectClass *klass, void *data) { DeviceClass *dc =3D DEVICE_CLASS(klass); - ICPStateClass *icpc =3D ICP_CLASS(klass); + ICPStateClass *icpc =3D ICP_BASE_CLASS(klass); =20 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 =3D "PowerNV ICP"; } =20 static const TypeInfo pnv_icp_info =3D { .name =3D TYPE_PNV_ICP, - .parent =3D TYPE_ICP, + .parent =3D TYPE_ICP_BASE, .instance_size =3D sizeof(PnvICPState), .class_init =3D pnv_icp_class_init, .class_size =3D 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; =20 +#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) =20 @@ -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) =20 -#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) =20 struct ICPStateClass { DeviceClass parent_class; =20 DeviceRealize parent_realize; + DeviceUnrealize parent_unrealize; DeviceReset parent_reset; =20 void (*pre_save)(ICPState *icp);