Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller

["Andreas Färber" <afaerber@suse.de>]

Hi,

Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [aik: moved to a separate device, reworked QOM]
> 
> ---
> Changes:
> 2013/07/17:
> * QOM rework
> 
> 2013/07/01
> * fixed VMState names in order to support xics-kvm migration to xics and vice versa
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Something went wrong here. ;)

[...]
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..04ce1be
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
[...]
> +typedef struct XICSStateKVM {
> +    struct XICSState parent;

parent_obj to align with most new devices?

> +
> +    uint32_t set_xive_token;
> +    uint32_t get_xive_token;
> +    uint32_t int_off_token;
> +    uint32_t int_on_token;
> +    int kernel_xics_fd;
> +} XICSStateKVM;
[...]
> +static int ics_kvm_realize(DeviceState *dev)
> +{
> +    ICSState *ics = ICS(dev);
> +
> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> +    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +
> +    return 0;
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *k = ICS_CLASS(klass);
> +
> +    dc->init = ics_kvm_realize;

This is fishy - probably you want to assign to dc->realize and fix the
signature?

Andreas

> +    dc->reset = ics_kvm_reset;
> +    k->pre_save = ics_get_kvm_state;
> +    k->post_load = ics_set_kvm_state;
> +}
> +
> +static TypeInfo ics_kvm_info = {

static const for all TypeInfos.

> +    .name = TYPE_ICS_KVM,
> +    .parent = TYPE_ICS,
> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs;
> +    ICPState *ss;
> +    XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
> +            OBJECT(icp), TYPE_XICS_KVM);
> +    XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));

Are you intentionally accessing that class by name rather than using
XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?

> +
> +    if (!icpkvm) {
> +        return;
> +    }
> +
> +    cs = CPU(cpu);
> +    ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +    if (icpkvm->kernel_xics_fd == -1) {
> +        abort();
> +    }
> +
> +    if (icpkvm->kernel_xics_fd != -1) {
> +        int ret;
> +        struct kvm_enable_cap xics_enable_cap = {
> +            .cap = KVM_CAP_IRQ_XICS,
> +            .flags = 0,
> +            .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
> +        };
> +
> +        ss->cs = cs;
> +
> +        ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
> +        if (ret < 0) {
> +            fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
> +                    cs->cpu_index, strerror(errno));

error_report() in place of all fprintf(stderr, ...)? (without \n then)

> +            exit(1);
> +        }
> +    }
> +
> +    /* Call emulated XICS implementation for consistency */
> +    assert(xics_info);

If you want to go safe, you could add assert(xics_info->cpu_setup).

> +    xics_info->cpu_setup(icp, cpu);
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                       uint32_t token,
> +                       uint32_t nargs, target_ulong args,
> +                       uint32_t nret, target_ulong rets)
> +{
> +    fprintf(stderr, "pseries: %s must never be called for in-kernel XICS\n",
> +            __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    XICSStateKVM *icpkvm = XICS_KVM(dev);
> +    XICSState *icp = XICS(dev);
> +    ICSState *ics;
> +    QemuOptsList *list = qemu_find_opts("machine");
> +    int i, rc;
> +    struct kvm_create_device xics_create_device = {
> +        .type = KVM_DEV_TYPE_XICS,
> +        .flags = 0,
> +    };
> +
> +    if (!kvm_enabled()) {
> +        error_setg(errp, "KVM must be enabled for in-kernel XICS");
> +        goto fail;
> +    }
> +
> +    if (QTAILQ_EMPTY(&list->head) ||
> +        !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                           "kernel_irqchip", true) ||

Is it safe to take a value from the first element in the list? Shouldn't
merging -machine spapr,accel=kvm -machine kernel_irqchip=on be taken
into account?

> +        !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> +        error_setg(errp, "KVM must be enabled for in-kernel XICS");
> +        return;
> +    }
> +
> +    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> +    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> +    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> +    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
> +        goto fail;
> +    }
> +
> +    /* Create the kernel ICP */
> +    rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
> +    if (rc < 0) {
> +        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> +    icp->ics = ICS(object_new(TYPE_ICS_KVM));
> +    ics = icp->ics;
> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +
> +    ics->nr_irqs = icp->nr_irqs;
> +    ics->offset = XICS_IRQ_BASE;
> +    ics->icp = icp;
> +    qdev_init_nofail(DEVICE(ics));

Since this is in a realize function, please use
object_property_set_bool(), which gives you access to the Error - to
catch it you can use a local Error *err variable.

> +
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        char buffer[32];
> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> +        qdev_init_nofail(DEVICE(&icp->ss[i]));

object_property_set_bool()

Where does icp->nr_servers come from? Is there no way to split this into
instance_init and realize?

> +    }
> +    return;
> +
> +fail:
> +    kvmppc_define_rtas_token(0, "ibm,set-xive");
> +    kvmppc_define_rtas_token(0, "ibm,get-xive");
> +    kvmppc_define_rtas_token(0, "ibm,int-on");
> +    kvmppc_define_rtas_token(0, "ibm,int-off");
> +    return;
> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *k = XICS_CLASS(oc);

xsc?

> +
> +    dc->realize = xics_kvm_realize;
> +    k->cpu_setup = xics_kvm_cpu_setup;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> +    .name          = TYPE_XICS_KVM,
> +    .parent        = TYPE_XICS,
> +    .instance_size = sizeof(XICSStateKVM),
> +    .class_init    = xics_kvm_class_init,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> +    type_register_static(&xics_kvm_info);
> +    type_register_static(&ics_kvm_info);
> +    type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..84433ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>  {
>      XICSState *icp = NULL;
>  
> -    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    if (kvm_enabled()) {
> +        bool irqchip_allowed = true, irqchip_required = false;
> +        QemuOptsList *list = qemu_find_opts("machine");
> +
> +        if (!QTAILQ_EMPTY(&list->head)) {
> +            irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                                                "kernel_irqchip", true);
> +            irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                                                 "kernel_irqchip", false);
> +        }

Again, don't we have APIs to help with opts access?

> +
> +        if (irqchip_allowed) {
> +            icp = try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs);
> +        }
> +
> +        if (irqchip_required && !icp) {
> +            perror("iFailed to create in-kernel XICS\n");

"Failed to ..."?

> +            abort();
> +        }
> +    }
> +
> +    if (!icp) {
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    }
> +
>      if (!icp) {
>          perror("Failed to create XICS\n");
>          abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 90ecaf8..835a3d6 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -32,6 +32,9 @@
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define TYPE_XICS_KVM "xics-kvm"
> +#define XICS_KVM(obj) OBJECT_CHECK(XICSStateKVM, (obj), TYPE_XICS_KVM)

TYPE_KVM_XICS, KVM_XICS() please - from the specific to base.

> +
>  #define XICS_CLASS(klass) \
>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
>  #define XICS_GET_CLASS(obj) \
> @@ -73,6 +76,9 @@ struct XICSState {
>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> +#define TYPE_ICP_KVM "icp-kvm"
> +#define ICP_KVM(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_KVM)
> +
>  #define ICP_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
>  #define ICP_GET_CLASS(obj) \
> @@ -89,6 +95,7 @@ struct ICPState {
>      /*< private >*/
>      DeviceState parent_obj;
>      /*< public >*/
> +    CPUState *cs;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -98,6 +105,9 @@ struct ICPState {
>  #define TYPE_ICS "ics"
>  #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>  
> +#define TYPE_ICS_KVM "icskvm"
> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
> +
>  #define ICS_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>  #define ICS_GET_CLASS(obj) \

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg