From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4d3k-0005aX-01 for qemu-devel@nongnu.org; Wed, 31 Jul 2013 16:31:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4cvi-00069l-A3 for qemu-devel@nongnu.org; Wed, 31 Jul 2013 16:23:36 -0400 Message-ID: <51F96B13.1060201@suse.de> Date: Wed, 31 Jul 2013 21:52:51 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1374043057-27208-1-git-send-email-aik@ozlabs.ru> <1374043057-27208-5-git-send-email-aik@ozlabs.ru> In-Reply-To: <1374043057-27208-5-git-send-email-aik@ozlabs.ru> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf , Paul Mackerras , qemu-ppc@nongnu.org, David Gibson Hi, Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy: > From: David Gibson >=20 > Recent (host) kernels support emulating the PAPR defined "XICS" interru= pt > controller system within KVM. This patch allows qemu to initialize and > configure the in-kernel XICS, and keep its state in sync with qemu's XI= CS > state as necessary. >=20 > This should give considerable performance improvements. e.g. on a simp= le > IPI ping-pong test between hardware threads, using qemu XICS gives us > around 5,000 irqs/second, whereas the in-kernel XICS gives us around > 70,000 irqs/s on the same hardware configuration. >=20 > [Mike Qiu : fixed mistype which caused ics_= set_kvm_state() to fail] > Signed-off-by: David Gibson > [aik: moved to a separate device, reworked QOM] >=20 > --- > Changes: > 2013/07/17: > * QOM rework >=20 > 2013/07/01 > * fixed VMState names in order to support xics-kvm migration to xics an= d vice versa >=20 > Signed-off-by: Alexey Kardashevskiy Something went wrong here. ;) [...] > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > new file mode 100644 > index 0000000..04ce1be > --- /dev/null > +++ b/hw/intc/xics_kvm.c [...] > +typedef struct XICSStateKVM { > + struct XICSState parent; parent_obj to align with most new devices? > + > + uint32_t set_xive_token; > + uint32_t get_xive_token; > + uint32_t int_off_token; > + uint32_t int_on_token; > + int kernel_xics_fd; > +} XICSStateKVM; [...] > +static int ics_kvm_realize(DeviceState *dev) > +{ > + ICSState *ics =3D ICS(dev); > + > + ics->irqs =3D g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); > + ics->islsi =3D g_malloc0(ics->nr_irqs * sizeof(bool)); > + ics->qirqs =3D qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_ir= qs); > + > + return 0; > +} > + > +static void ics_kvm_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + ICSStateClass *k =3D ICS_CLASS(klass); > + > + dc->init =3D ics_kvm_realize; This is fishy - probably you want to assign to dc->realize and fix the signature? Andreas > + dc->reset =3D ics_kvm_reset; > + k->pre_save =3D ics_get_kvm_state; > + k->post_load =3D ics_set_kvm_state; > +} > + > +static TypeInfo ics_kvm_info =3D { static const for all TypeInfos. > + .name =3D TYPE_ICS_KVM, > + .parent =3D TYPE_ICS, > + .instance_size =3D sizeof(ICSState), > + .class_init =3D ics_kvm_class_init, > +}; > + > +/* > + * XICS-KVM > + */ > +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > +{ > + CPUState *cs; > + ICPState *ss; > + XICSStateKVM *icpkvm =3D (XICSStateKVM *) object_dynamic_cast( > + OBJECT(icp), TYPE_XICS_KVM); > + XICSStateClass *xics_info =3D XICS_CLASS(object_class_by_name(TYPE= _XICS)); Are you intentionally accessing that class by name rather than using XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things? > + > + if (!icpkvm) { > + return; > + } > + > + cs =3D CPU(cpu); > + ss =3D &icp->ss[cs->cpu_index]; > + > + assert(cs->cpu_index < icp->nr_servers); > + if (icpkvm->kernel_xics_fd =3D=3D -1) { > + abort(); > + } > + > + if (icpkvm->kernel_xics_fd !=3D -1) { > + int ret; > + struct kvm_enable_cap xics_enable_cap =3D { > + .cap =3D KVM_CAP_IRQ_XICS, > + .flags =3D 0, > + .args =3D {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0}, > + }; > + > + ss->cs =3D cs; > + > + ret =3D kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_ca= p); > + if (ret < 0) { > + fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %= s\n", > + cs->cpu_index, strerror(errno)); error_report() in place of all fprintf(stderr, ...)? (without \n then) > + exit(1); > + } > + } > + > + /* Call emulated XICS implementation for consistency */ > + assert(xics_info); If you want to go safe, you could add assert(xics_info->cpu_setup). > + xics_info->cpu_setup(icp, cpu); > +} > + > +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr, > + uint32_t token, > + uint32_t nargs, target_ulong args, > + uint32_t nret, target_ulong rets) > +{ > + fprintf(stderr, "pseries: %s must never be called for in-kernel XI= CS\n", > + __func__); > +} > + > +static void xics_kvm_realize(DeviceState *dev, Error **errp) > +{ > + XICSStateKVM *icpkvm =3D XICS_KVM(dev); > + XICSState *icp =3D XICS(dev); > + ICSState *ics; > + QemuOptsList *list =3D qemu_find_opts("machine"); > + int i, rc; > + struct kvm_create_device xics_create_device =3D { > + .type =3D KVM_DEV_TYPE_XICS, > + .flags =3D 0, > + }; > + > + if (!kvm_enabled()) { > + error_setg(errp, "KVM must be enabled for in-kernel XICS"); > + goto fail; > + } > + > + if (QTAILQ_EMPTY(&list->head) || > + !qemu_opt_get_bool(QTAILQ_FIRST(&list->head), > + "kernel_irqchip", true) || Is it safe to take a value from the first element in the list? Shouldn't merging -machine spapr,accel=3Dkvm -machine kernel_irqchip=3Don be taken into account? > + !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) { > + error_setg(errp, "KVM must be enabled for in-kernel XICS"); > + return; > + } > + > + icpkvm->set_xive_token =3D spapr_rtas_register("ibm,set-xive", rta= s_dummy); > + icpkvm->get_xive_token =3D spapr_rtas_register("ibm,get-xive", rta= s_dummy); > + icpkvm->int_off_token =3D spapr_rtas_register("ibm,int-off", rtas_= dummy); > + icpkvm->int_on_token =3D spapr_rtas_register("ibm,int-on", rtas_du= mmy); > + > + rc =3D kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-x= ive"); > + if (rc < 0) { > + error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive"); > + goto fail; > + } > + > + rc =3D kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-x= ive"); > + if (rc < 0) { > + error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive"); > + goto fail; > + } > + > + rc =3D kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on"= ); > + if (rc < 0) { > + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on"); > + goto fail; > + } > + > + rc =3D kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-of= f"); > + if (rc < 0) { > + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off"); > + goto fail; > + } > + > + /* Create the kernel ICP */ > + rc =3D kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_dev= ice); > + if (rc < 0) { > + error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XI= CS"); > + goto fail; > + } > + > + icpkvm->kernel_xics_fd =3D xics_create_device.fd; > + > + icp->ics =3D ICS(object_new(TYPE_ICS_KVM)); > + ics =3D icp->ics; > + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NU= LL); > + > + ics->nr_irqs =3D icp->nr_irqs; > + ics->offset =3D XICS_IRQ_BASE; > + ics->icp =3D icp; > + qdev_init_nofail(DEVICE(ics)); Since this is in a realize function, please use object_property_set_bool(), which gives you access to the Error - to catch it you can use a local Error *err variable. > + > + icp->ss =3D g_malloc0(icp->nr_servers*sizeof(ICPState)); > + for (i =3D 0; i < icp->nr_servers; i++) { > + char buffer[32]; > + object_initialize(&icp->ss[i], TYPE_ICP_KVM); > + snprintf(buffer, sizeof(buffer), "icp[%d]", i); > + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss= [i]), NULL); > + qdev_init_nofail(DEVICE(&icp->ss[i])); object_property_set_bool() Where does icp->nr_servers come from? Is there no way to split this into instance_init and realize? > + } > + return; > + > +fail: > + kvmppc_define_rtas_token(0, "ibm,set-xive"); > + kvmppc_define_rtas_token(0, "ibm,get-xive"); > + kvmppc_define_rtas_token(0, "ibm,int-on"); > + kvmppc_define_rtas_token(0, "ibm,int-off"); > + return; > +} > + > +static void xics_kvm_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(oc); > + XICSStateClass *k =3D XICS_CLASS(oc); xsc? > + > + dc->realize =3D xics_kvm_realize; > + k->cpu_setup =3D xics_kvm_cpu_setup; > +} > + > +static const TypeInfo xics_kvm_info =3D { > + .name =3D TYPE_XICS_KVM, > + .parent =3D TYPE_XICS, > + .instance_size =3D sizeof(XICSStateKVM), > + .class_init =3D xics_kvm_class_init, > +}; > + > +static void xics_kvm_register_types(void) > +{ > + type_register_static(&xics_kvm_info); > + type_register_static(&ics_kvm_info); > + type_register_static(&icp_kvm_info); > +} > + > +type_init(xics_kvm_register_types) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 432f0d2..84433ee 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers,= int nr_irqs) > { > XICSState *icp =3D NULL; > =20 > - icp =3D try_create_xics(TYPE_XICS, nr_servers, nr_irqs); > + if (kvm_enabled()) { > + bool irqchip_allowed =3D true, irqchip_required =3D false; > + QemuOptsList *list =3D qemu_find_opts("machine"); > + > + if (!QTAILQ_EMPTY(&list->head)) { > + irqchip_allowed =3D qemu_opt_get_bool(QTAILQ_FIRST(&list->= head), > + "kernel_irqchip", true= ); > + irqchip_required =3D qemu_opt_get_bool(QTAILQ_FIRST(&list-= >head), > + "kernel_irqchip", fal= se); > + } Again, don't we have APIs to help with opts access? > + > + if (irqchip_allowed) { > + icp =3D try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs= ); > + } > + > + if (irqchip_required && !icp) { > + perror("iFailed to create in-kernel XICS\n"); "Failed to ..."? > + abort(); > + } > + } > + > + if (!icp) { > + icp =3D try_create_xics(TYPE_XICS, nr_servers, nr_irqs); > + } > + > if (!icp) { > perror("Failed to create XICS\n"); > abort(); > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 90ecaf8..835a3d6 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -32,6 +32,9 @@ > #define TYPE_XICS "xics" > #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS) > =20 > +#define TYPE_XICS_KVM "xics-kvm" > +#define XICS_KVM(obj) OBJECT_CHECK(XICSStateKVM, (obj), TYPE_XICS_KVM) TYPE_KVM_XICS, KVM_XICS() please - from the specific to base. > + > #define XICS_CLASS(klass) \ > OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS) > #define XICS_GET_CLASS(obj) \ > @@ -73,6 +76,9 @@ struct XICSState { > #define TYPE_ICP "icp" > #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP) > =20 > +#define TYPE_ICP_KVM "icp-kvm" > +#define ICP_KVM(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_KVM) > + > #define ICP_CLASS(klass) \ > OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP) > #define ICP_GET_CLASS(obj) \ > @@ -89,6 +95,7 @@ struct ICPState { > /*< private >*/ > DeviceState parent_obj; > /*< public >*/ > + CPUState *cs; > uint32_t xirr; > uint8_t pending_priority; > uint8_t mfrr; > @@ -98,6 +105,9 @@ struct ICPState { > #define TYPE_ICS "ics" > #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS) > =20 > +#define TYPE_ICS_KVM "icskvm" > +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM) > + > #define ICS_CLASS(klass) \ > OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS) > #define ICS_GET_CLASS(obj) \ Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg