From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsB8k-000519-Dl for qemu-devel@nongnu.org; Thu, 27 Jun 2013 08:17:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsB8i-0008JI-A6 for qemu-devel@nongnu.org; Thu, 27 Jun 2013 08:17:30 -0400 Received: from mail-pb0-f43.google.com ([209.85.160.43]:32875) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsB8h-0008Ix-Vo for qemu-devel@nongnu.org; Thu, 27 Jun 2013 08:17:28 -0400 Received: by mail-pb0-f43.google.com with SMTP id md12so830850pbc.30 for ; Thu, 27 Jun 2013 05:17:26 -0700 (PDT) Message-ID: <51CC2D4F.9000401@ozlabs.ru> Date: Thu, 27 Jun 2013 22:17:19 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1372315560-5478-1-git-send-email-aik@ozlabs.ru> <1372315560-5478-3-git-send-email-aik@ozlabs.ru> <20130627114719.GG10614@voom.fritz.box> In-Reply-To: <20130627114719.GG10614@voom.fritz.box> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/17] pseries: rework XICS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Paolo Bonzini , Paul Mackerras On 06/27/2013 09:47 PM, David Gibson wrote: > 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. The idea came from Alex Graf, this is already done for openpic/openpic-kvm. The normal practice is to move ioctls to KVM to KVM code and provide empty stubs for non-KVM case. There were too many so having a separate xics-kvm is kind of help. > Won't that > prevent migrating from a system with a kernel xics to one without, or > vice versa? Mmm. Do we care much about that?... At the moment it is not supported that as VMStateDescription have different .name for xics and xics-kvm but easy to fix. And we do not pass a device to vmstate_register so that must be it. > >> >> 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. >> >> 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(-) >> >> 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 >> */ >> >> -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 >> >> @@ -49,12 +42,6 @@ struct icp_server_state { >> >> struct ics_state; >> >> -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 server, 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 >= ics->offset) >> @@ -506,9 +472,8 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr, >> rtas_st(rets, 0, 0); /* Success */ >> } >> >> -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 = (struct icp_state *)opaque; >> struct ics_state *ics = icp->ics; >> int i; >> >> @@ -527,7 +492,12 @@ static void xics_reset(void *opaque) >> } >> } >> >> -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 = CPU(cpu); >> CPUPPCState *env = &cpu->env; >> @@ -551,37 +521,72 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu) >> } >> } >> >> -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 = icp->ics; >> >> - icp = g_malloc0(sizeof(*icp)); >> - icp->nr_servers = nr_servers; >> icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state)); >> >> ics = g_malloc0(sizeof(*ics)); >> - ics->nr_irqs = nr_irqs; >> + ics->nr_irqs = icp->nr_irqs; >> ics->offset = XICS_IRQ_BASE; >> - ics->irqs = g_malloc0(nr_irqs * sizeof(struct ics_irq_state)); >> - ics->islsi = g_malloc0(nr_irqs * sizeof(bool)); >> + ics->irqs = g_malloc0(ics->nr_irqs * sizeof(struct ics_irq_state)); >> + ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); >> >> icp->ics = ics; >> ics->icp = icp; >> >> - ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, nr_irqs); >> + ics->qirqs = qemu_allocate_irqs(handler, ics, ics->nr_irqs); >> +} >> >> - 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 = XICS(dev); >> + >> + xics_common_init(icp, ics_set_irq); >> >> 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); >> >> - qemu_register_reset(xics_reset, icp); >> +} >> + >> +static Property xics_properties[] = { >> + 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 = DEVICE_CLASS(oc); >> + >> + dc->realize = xics_realize; >> + dc->props = xics_properties; >> + dc->reset = xics_reset; >> +} >> + >> +static const TypeInfo xics_info = { >> + .name = TYPE_XICS, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(struct icp_state), >> + .class_init = 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); >> >> - 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) >> } >> } >> >> +static struct icp_state *try_create_xics(const char *type, int nr_servers, >> + int nr_irqs) >> +{ >> + DeviceState *dev; >> + >> + dev = 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 = NULL; >> + >> + icp = 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__ >> >> +#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) >> + */ >> >> -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; >> +}; >> >> qemu_irq xics_get_qirq(struct icp_state *icp, int irq); >> void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi); >> >> -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); >> >> +extern const VMStateDescription vmstate_icp_server; >> +extern const VMStateDescription vmstate_ics; >> + >> #endif /* __XICS_H__ */ > -- Alexey