Alexey Kardashevskiy <a...@ozlabs.ru> writes:
> 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.
The way this should be modelled is:
XICSCommon
-> XICS
-> XICSKVM
With vmstate et al being part of XICSCommon. See how the i8259 and
i8254 are modelled.
Regards,
Anthony Liguori
>
>
>> 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 <a...@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
