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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <aik@ozlabs.ru>
>> ---
>>  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