From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsAwu-0007wR-RH for qemu-devel@nongnu.org; Thu, 27 Jun 2013 08:05:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsAwp-0003ep-Gp for qemu-devel@nongnu.org; Thu, 27 Jun 2013 08:05:16 -0400 Date: Thu, 27 Jun 2013 21:47:19 +1000 From: David Gibson Message-ID: <20130627114719.GG10614@voom.fritz.box> References: <1372315560-5478-1-git-send-email-aik@ozlabs.ru> <1372315560-5478-3-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="XaUbO9McV5wPQijU" Content-Disposition: inline In-Reply-To: <1372315560-5478-3-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH 02/17] pseries: rework XICS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Paolo Bonzini , Paul Mackerras --XaUbO9McV5wPQijU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 27, 2013 at 04:45:45PM +1000, Alexey Kardashevskiy wrote: > Currently XICS interrupt controller is not a QEMU device. As we are going > to support in-kernel emulated XICS which is a part of KVM, it make > sense not to extend the existing XICS and have multiple KVM stub functions > but to create yet another device and share pieces between fully emulated > XICS and in-kernel XICS. Hmm. So, I think changing the xics to the qdev/qom framework is a generally good idea. But I'm not convinced its a good idea to have different devices for the kernel and non-kernel xics. Won't that prevent migrating from a system with a kernel xics to one without, or vice versa? >=20 > The rework includes: > * port to QOM > * made few functions public to use from in-kernel XICS implementation > * made VMStateDescription public to be used for in-kernel XICS migration > * move xics_system_init() to spapr.c, it tries creating fully-emulated > XICS now and will try in-kernel XICS in upcoming patches. >=20 > Signed-off-by: Alexey Kardashevskiy > --- > hw/intc/xics.c | 109 ++++++++++++++++++++++++++-----------------= ------ > hw/ppc/spapr.c | 28 +++++++++++++ > include/hw/ppc/xics.h | 59 ++++++++++++++++++++++++-- > 3 files changed, 141 insertions(+), 55 deletions(-) >=20 > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 091912e..0e374c8 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -34,13 +34,6 @@ > * ICP: Presentation layer > */ > =20 > -struct icp_server_state { > - uint32_t xirr; > - uint8_t pending_priority; > - uint8_t mfrr; > - qemu_irq output; > -}; > - > #define XISR_MASK 0x00ffffff > #define CPPR_MASK 0xff000000 > =20 > @@ -49,12 +42,6 @@ struct icp_server_state { > =20 > struct ics_state; > =20 > -struct icp_state { > - long nr_servers; > - struct icp_server_state *ss; > - struct ics_state *ics; > -}; > - > static void ics_reject(struct ics_state *ics, int nr); > static void ics_resend(struct ics_state *ics); > static void ics_eoi(struct ics_state *ics, int nr); > @@ -171,27 +158,6 @@ static void icp_irq(struct icp_state *icp, int serve= r, int nr, uint8_t priority) > /* > * ICS: Source layer > */ > - > -struct ics_irq_state { > - int server; > - uint8_t priority; > - uint8_t saved_priority; > -#define XICS_STATUS_ASSERTED 0x1 > -#define XICS_STATUS_SENT 0x2 > -#define XICS_STATUS_REJECTED 0x4 > -#define XICS_STATUS_MASKED_PENDING 0x8 > - uint8_t status; > -}; > - > -struct ics_state { > - int nr_irqs; > - int offset; > - qemu_irq *qirqs; > - bool *islsi; > - struct ics_irq_state *irqs; > - struct icp_state *icp; > -}; > - > static int ics_valid_irq(struct ics_state *ics, uint32_t nr) > { > return (nr >=3D ics->offset) > @@ -506,9 +472,8 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnviron= ment *spapr, > rtas_st(rets, 0, 0); /* Success */ > } > =20 > -static void xics_reset(void *opaque) > +void xics_common_reset(struct icp_state *icp) Why do you need to expose this interface? Couldn't the caller use qdev_reset(xics) just as easily? > { > - struct icp_state *icp =3D (struct icp_state *)opaque; > struct ics_state *ics =3D icp->ics; > int i; > =20 > @@ -527,7 +492,12 @@ static void xics_reset(void *opaque) > } > } > =20 > -void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu) > +static void xics_reset(DeviceState *d) > +{ > + xics_common_reset(XICS(d)); > +} > + > +void xics_common_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu) > { > CPUState *cs =3D CPU(cpu); > CPUPPCState *env =3D &cpu->env; > @@ -551,37 +521,72 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCC= PU *cpu) > } > } > =20 > -struct icp_state *xics_system_init(int nr_servers, int nr_irqs) > +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu) > +{ > + xics_common_cpu_setup(icp, cpu); > +} > + > +void xics_common_init(struct icp_state *icp, qemu_irq_handler handler) > { > - struct icp_state *icp; > - struct ics_state *ics; > + struct ics_state *ics =3D icp->ics; > =20 > - icp =3D g_malloc0(sizeof(*icp)); > - icp->nr_servers =3D nr_servers; > icp->ss =3D g_malloc0(icp->nr_servers*sizeof(struct icp_server_state= )); > =20 > ics =3D g_malloc0(sizeof(*ics)); > - ics->nr_irqs =3D nr_irqs; > + ics->nr_irqs =3D icp->nr_irqs; > ics->offset =3D XICS_IRQ_BASE; > - ics->irqs =3D g_malloc0(nr_irqs * sizeof(struct ics_irq_state)); > - ics->islsi =3D g_malloc0(nr_irqs * sizeof(bool)); > + ics->irqs =3D g_malloc0(ics->nr_irqs * sizeof(struct ics_irq_state)); > + ics->islsi =3D g_malloc0(ics->nr_irqs * sizeof(bool)); > =20 > icp->ics =3D ics; > ics->icp =3D icp; > =20 > - ics->qirqs =3D qemu_allocate_irqs(ics_set_irq, ics, nr_irqs); > + ics->qirqs =3D qemu_allocate_irqs(handler, ics, ics->nr_irqs); > +} > =20 > - spapr_register_hypercall(H_CPPR, h_cppr); > - spapr_register_hypercall(H_IPI, h_ipi); > - spapr_register_hypercall(H_XIRR, h_xirr); > - spapr_register_hypercall(H_EOI, h_eoi); > +static void xics_realize(DeviceState *dev, Error **errp) > +{ > + struct icp_state *icp =3D XICS(dev); > + > + xics_common_init(icp, ics_set_irq); > =20 > spapr_rtas_register("ibm,set-xive", rtas_set_xive); > spapr_rtas_register("ibm,get-xive", rtas_get_xive); > spapr_rtas_register("ibm,int-off", rtas_int_off); > spapr_rtas_register("ibm,int-on", rtas_int_on); > =20 > - qemu_register_reset(xics_reset, icp); > +} > + > +static Property xics_properties[] =3D { > + DEFINE_PROP_UINT32("nr_servers", struct icp_state, nr_servers, -1), > + DEFINE_PROP_UINT32("nr_irqs", struct icp_state, nr_irqs, -1), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void xics_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(oc); > + > + dc->realize =3D xics_realize; > + dc->props =3D xics_properties; > + dc->reset =3D xics_reset; > +} > + > +static const TypeInfo xics_info =3D { > + .name =3D TYPE_XICS, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(struct icp_state), > + .class_init =3D xics_class_init, > +}; > + > +static void xics_register_types(void) > +{ > + spapr_register_hypercall(H_CPPR, h_cppr); > + spapr_register_hypercall(H_IPI, h_ipi); > + spapr_register_hypercall(H_XIRR, h_xirr); > + spapr_register_hypercall(H_EOI, h_eoi); > =20 > - return icp; > + type_register_static(&xics_info); > } > + > +type_init(xics_register_types) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 38c29b7..def3505 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -719,6 +719,34 @@ static int spapr_vga_init(PCIBus *pci_bus) > } > } > =20 > +static struct icp_state *try_create_xics(const char *type, int nr_server= s, > + int nr_irqs) > +{ > + DeviceState *dev; > + > + dev =3D qdev_create(NULL, type); > + qdev_prop_set_uint32(dev, "nr_servers", nr_servers); > + qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs); > + if (qdev_init(dev) < 0) { > + return NULL; You could just use qdev_init_nofail() here to avoid the manual handling of failures. > + } > + > + return XICS(dev); > +} > + > +static struct icp_state *xics_system_init(int nr_servers, int nr_irqs) > +{ > + struct icp_state *icp =3D NULL; > + > + icp =3D try_create_xics(TYPE_XICS, nr_servers, nr_irqs); > + if (!icp) { > + perror("Failed to create XICS\n"); > + abort(); > + } > + > + return icp; > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void ppc_spapr_init(QEMUMachineInitArgs *args) > { > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 6bce042..3f72806 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -27,15 +27,68 @@ > #if !defined(__XICS_H__) > #define __XICS_H__ > =20 > +#include "hw/sysbus.h" > + > +#define TYPE_XICS "xics" > +#define XICS(obj) OBJECT_CHECK(struct icp_state, (obj), TYPE_XICS) > + > #define XICS_IPI 0x2 > -#define XICS_IRQ_BASE 0x10 > +#define XICS_BUID 0x1 > +#define XICS_IRQ_BASE (XICS_BUID << 12) > + > +/* > + * We currently only support one BUID which is our interrupt base > + * (the kernel implementation supports more but we don't exploit > + * that yet) > + */ > =20 > -struct icp_state; > +struct icp_state { > + /*< private >*/ > + SysBusDevice parent_obj; > + /*< public >*/ > + uint32_t nr_servers; > + uint32_t nr_irqs; > + struct icp_server_state *ss; > + struct ics_state *ics; > +}; > + > +struct icp_server_state { > + uint32_t xirr; > + uint8_t pending_priority; > + uint8_t mfrr; > + qemu_irq output; > +}; The indivudual server_state and irq_state structures probably shouldn't be exported. > +struct ics_state { > + uint32_t nr_irqs; > + uint32_t offset; > + qemu_irq *qirqs; > + bool *islsi; > + struct ics_irq_state *irqs; > + struct icp_state *icp; > +}; > + > +struct ics_irq_state { > + uint32_t server; > + uint8_t priority; > + uint8_t saved_priority; > +#define XICS_STATUS_ASSERTED 0x1 > +#define XICS_STATUS_SENT 0x2 > +#define XICS_STATUS_REJECTED 0x4 > +#define XICS_STATUS_MASKED_PENDING 0x8 > + uint8_t status; > +}; > =20 > qemu_irq xics_get_qirq(struct icp_state *icp, int irq); > void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi); > =20 > -struct icp_state *xics_system_init(int nr_servers, int nr_irqs); > +void xics_common_init(struct icp_state *icp, qemu_irq_handler handler); > +void xics_common_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu); > +void xics_common_reset(struct icp_state *icp); > + > void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu); > =20 > +extern const VMStateDescription vmstate_icp_server; > +extern const VMStateDescription vmstate_ics; > + > #endif /* __XICS_H__ */ --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --XaUbO9McV5wPQijU Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) iEYEARECAAYFAlHMJkcACgkQaILKxv3ab8ZfswCffKSevMzD2SpvsNxa25uRzb3a /7QAn3784rRVkpiWmh3J50Eqo8y7wzEq =7zM2 -----END PGP SIGNATURE----- --XaUbO9McV5wPQijU--