Re: [Qemu-ppc] [PATCH 02/17] pseries: rework XICS

[David Gibson]

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?

> 
> 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 <address@hidden>
> ---
>  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__ */

-- 
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

pgpDLnTV2CUaN.pgp
Description: PGP signature