[RFC,4/4] Initial implementation of vGICv3.

Get/put routines are missing. Live migration is not possible.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/Makefile.objs   |   1 +
 hw/intc/arm_gicv3_kvm.c | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 284 insertions(+)
 create mode 100644 hw/intc/arm_gicv3_kvm.c

On 05/22/2015 12:58 PM, Pavel Fedin wrote:
> Get/put routines are missing. Live migration is not possible.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/intc/Makefile.objs   |   1 +
>  hw/intc/arm_gicv3_kvm.c | 283 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 284 insertions(+)
>  create mode 100644 hw/intc/arm_gicv3_kvm.c
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 41fe9ec..776f517 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
>  
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> +obj-$(CONFIG_ARM_GIC_KVM) += arm_gicv3_kvm.o
>  obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
>  obj-$(CONFIG_GRLIB) += grlib_irqmp.o
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> new file mode 100644
> index 0000000..6f892ea
> --- /dev/null
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -0,0 +1,283 @@
> +/*
> + * ARM Generic Interrupt Controller using KVM in-kernel support
> + *
> + * Copyright (c) 2012 Linaro Limited
> + * Written by Peter Maydell
> + * Save/Restore logic added by Christoffer Dall.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/kvm.h"
> +#include "kvm_arm.h"
> +#include "gicv3_internal.h"
> +
> +//#define DEBUG_GICV3_KVM
> +
> +#ifdef DEBUG_GICV3_KVM
> +static const int debug_gicv3_kvm = 1;
> +#else
> +static const int debug_gicv3_kvm = 0;
> +#endif
> +
> +#define DPRINTF(fmt, ...) do { \
> +        if (debug_gicv3_kvm) { \
> +            printf("kvm_gicv3: " fmt , ## __VA_ARGS__); \
> +        } \
> +    } while (0)
> +
> +#define TYPE_KVM_ARM_GICV3 "kvm-arm-gicv3"
> +#define KVM_ARM_GICV3(obj) \
> +     OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GICV3)
> +#define KVM_ARM_GICV3_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(KVMARMGICClass, (klass), TYPE_KVM_ARM_GICV3)
> +#define KVM_ARM_GICV3_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(KVMARMGICClass, (obj), TYPE_KVM_ARM_GICV3)
> +
> +typedef struct KVMARMGICClass {
> +    ARMGICCommonClass parent_class;
> +    DeviceRealize parent_realize;
> +    void (*parent_reset)(DeviceState *dev);
> +} KVMARMGICClass;
> +
> +static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level)
> +{
> +    /* Meaning of the 'irq' parameter:
> +     *  [0..N-1] : external interrupts
> +     *  [N..N+31] : PPI (internal) interrupts for CPU 0
> +     *  [N+32..N+63] : PPI (internal interrupts for CPU 1
> +     *  ...
> +     * Convert this to the kernel's desired encoding, which
> +     * has separate fields in the irq number for type,
> +     * CPU number and interrupt number.
> +     */
> +    GICState *s = (GICState *)opaque;
> +    int kvm_irq, irqtype, cpu;
> +
> +    if (irq < (s->num_irq - GICV3_INTERNAL)) {
> +        /* External interrupt. The kernel numbers these like the GIC
> +         * hardware, with external interrupt IDs starting after the
> +         * internal ones.
> +         */
> +        irqtype = KVM_ARM_IRQ_TYPE_SPI;
> +        cpu = 0;
> +        irq += GICV3_INTERNAL;
> +    } else {
> +        /* Internal interrupt: decode into (cpu, interrupt id) */
> +        irqtype = KVM_ARM_IRQ_TYPE_PPI;
> +        irq -= (s->num_irq - GICV3_INTERNAL);
> +        cpu = irq / GICV3_INTERNAL;
> +        irq %= GICV3_INTERNAL;
> +    }
> +    kvm_irq = (irqtype << KVM_ARM_IRQ_TYPE_SHIFT)
> +        | (cpu << KVM_ARM_IRQ_VCPU_SHIFT) | irq;
> +
> +    kvm_set_irq(kvm_state, kvm_irq, !!level);
> +}
> +
> +static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = group,
> +        .attr = attrnum,
> +        .flags = 0,
> +    };
> +
> +    if (s->dev_fd == -1) {
> +        return false;
> +    }
> +
> +    return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
> +}
> +
> +static void kvm_gic_access(GICState *s, int group, int offset,
> +                                   int cpu, uint32_t *val, bool write)
> +{
> +    struct kvm_device_attr attr;
> +    int type;
> +    int err;
> +
> +    cpu = cpu & 0xff;
> +
> +    attr.flags = 0;
> +    attr.group = group;
> +    attr.attr = (((uint64_t)cpu << KVM_DEV_ARM_VGIC_CPUID_SHIFT) &
> +                 KVM_DEV_ARM_VGIC_CPUID_MASK) |
> +                (((uint64_t)offset << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) &
> +                 KVM_DEV_ARM_VGIC_OFFSET_MASK);
> +    attr.addr = (uintptr_t)val;
> +
> +    if (write) {
> +        type = KVM_SET_DEVICE_ATTR;
> +    } else {
> +        type = KVM_GET_DEVICE_ATTR;
> +    }
> +
> +    err = kvm_device_ioctl(s->dev_fd, type, &attr);
> +    if (err < 0) {
> +        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
> +                strerror(-err));
> +        abort();
> +    }
> +}
> +
> +static void kvm_arm_gicv3_put(GICState *s)
> +{
> +    /* TODO */
> +    DPRINTF("Cannot put kernel gic state, no kernel interface\n");
> +}
> +
> +static void kvm_arm_gicv3_get(GICState *s)
> +{
> +    /* TODO */
> +    DPRINTF("Cannot get kernel gic state, no kernel interface\n");
> +}
> +
> +static void kvm_arm_gicv3_reset(DeviceState *dev)
> +{
> +    GICState *s = ARM_GIC_COMMON(dev);
> +    KVMARMGICClass *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +
> +    DPRINTF("Reset\n");
> +
> +    kgc->parent_reset(dev);
> +    kvm_arm_gicv3_put(s);
> +}
> +
> +static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> +{
> +    int i;
> +    GICState *s = KVM_ARM_GICV3(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    KVMARMGICClass *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    DPRINTF("kvm_arm_gicv3_realize\n");
> +
> +    kgc->parent_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (s->security_extn)

Hi Pavel, Shlomo,

Looks GICv3 common class currently miss this security_extn field +
parent_fiq so it does not compile without changes. Or did I miss something?

Best Regards

Eric
> +        error_setg(errp, "the in-kernel VGIC does not implement the "
> +                   "security extensions");
> +        return;
> +    }
> +
> +    i = s->num_irq - GICV3_INTERNAL;
> +    /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
> +     * GPIO array layout is thus:
> +     *  [0..N-1] SPIs
> +     *  [N..N+31] PPIs for CPU 0
> +     *  [N+32..N+63] PPIs for CPU 1
> +     *   ...
> +     */
> +    i += (GICV3_INTERNAL * s->num_cpu);
> +    qdev_init_gpio_in(dev, kvm_arm_gicv3_set_irq, i);
> +    /* We never use our outbound IRQ lines but provide them so that
> +     * we maintain the same interface as the non-KVM GIC.
> +     */
> +    for (i = 0; i < s->num_cpu; i++) {
> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
> +    }
> +    for (i = 0; i < s->num_cpu; i++) {
> +        sysbus_init_irq(sbd, &s->parent_fiq[i]);
> +    }
> +
> +    /* Try to create the device via the device control API */
> +    s->dev_fd = -1;
> +    ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +    if (ret >= 0) {
> +        s->dev_fd = ret;
> +    } else if (ret != -ENODEV && ret != -ENOTSUP) {
> +        error_setg_errno(errp, -ret, "error creating in-kernel VGIC");
> +        return;
> +    }
> +
> +    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
> +        uint32_t numirqs = s->num_irq;
> +        DPRINTF("KVM_DEV_ARM_VGIC_GRP_NR_IRQS = %u\n", numirqs);
> +        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
> +    }
> +
> +    /* Tell the kernel to complete VGIC initialization now */
> +    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                              KVM_DEV_ARM_VGIC_CTRL_INIT)) {
> +	DPRINTF("KVM_DEV_ARM_VGIC_CTRL_INIT\n");
> +        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                          KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
> +    }
> +
> +    /* Distributor */
> +    memory_region_init_reservation(&s->iomem_dist, OBJECT(s),
> +                                   "kvm-gicv3_dist", 0x10000);
> +    sysbus_init_mmio(sbd, &s->iomem_dist);
> +    kvm_arm_register_device(&s->iomem_dist, -1,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                            KVM_VGIC_V3_ADDR_TYPE_DIST,
> +                            s->dev_fd);
> +
> +    /* Nothing much to do with SPI and ITS */
> +    memory_region_init_reservation(&s->iomem_spi, OBJECT(s),
> +                                   "kvm-gicv3_spi", 0x10000);
> +    sysbus_init_mmio(sbd, &s->iomem_spi);
> +    memory_region_init_reservation(&s->iomem_its_cntrl, OBJECT(s),
> +                                   "kvm-gicv3_its_control", 0x10000);
> +    sysbus_init_mmio(sbd, &s->iomem_its_cntrl);
> +    memory_region_init_reservation(&s->iomem_its, OBJECT(s),
> +                                   "kvm-gicv3_its_translation", 0x10000);
> +    sysbus_init_mmio(sbd, &s->iomem_its);
> +
> +    /* Redistributor */
> +    memory_region_init_reservation(&s->iomem_lpi, OBJECT(s),
> +                                   "kvm-gicv3_lpi", 0x800000);
> +    sysbus_init_mmio(sbd, &s->iomem_lpi);
> +    kvm_arm_register_device(&s->iomem_lpi, -1,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                            KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +                            s->dev_fd);
> +}
> +
> +static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ARMGICCommonClass *agcc = ARM_GIC_COMMON_CLASS(klass);
> +    KVMARMGICClass *kgc = KVM_ARM_GICV3_CLASS(klass);
> +
> +    agcc->pre_save = kvm_arm_gicv3_get;
> +    agcc->post_load = kvm_arm_gicv3_put;
> +    kgc->parent_realize = dc->realize;
> +    kgc->parent_reset = dc->reset;
> +    dc->realize = kvm_arm_gicv3_realize;
> +    dc->reset = kvm_arm_gicv3_reset;
> +}
> +
> +static const TypeInfo kvm_arm_gicv3_info = {
> +    .name = TYPE_KVM_ARM_GICV3,
> +    .parent = TYPE_ARM_GICV3_COMMON,
> +    .instance_size = sizeof(GICState),
> +    .class_init = kvm_arm_gicv3_class_init,
> +    .class_size = sizeof(KVMARMGICClass),
> +};
> +
> +static void kvm_arm_gicv3_register_types(void)
> +{
> +    type_register_static(&kvm_arm_gicv3_info);
> +}
> +
> +type_init(kvm_arm_gicv3_register_types)
>

Hi!

> Looks GICv3 common class currently miss this security_extn field +
> parent_fiq so it does not compile without changes. Or did I miss something?

 Just throw this if(...) away. It's my fault. Actually i have rebased Shlomo's patches on
yesterday's master, and during this i added parent_fiq[] to GICv3, because without filling
these in the thing stopped working due to recent GICv2 code changes. I needed to fix the
problem quickly, so i copied initializing parent_fiq together with security_extn field
from v2 code. But, this makes no sense because security_extn is never initialized, it is
not even assigned property name. So just remove this small fragment, it's not needed now.
 I have fixed this in my working tree 2 hours ago. parent_fiq is declared as:
--- cut ---
qemu_irq parent_fiq[GICV3_NCPU];
--- cut ---

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Hi Pavel,

On Fri, May 22, 2015 at 07:57:13PM +0300, Pavel Fedin wrote:
>  Hi!
> 
> > Looks GICv3 common class currently miss this security_extn field +
> > parent_fiq so it does not compile without changes. Or did I miss something?
> 
>  Just throw this if(...) away. It's my fault. Actually i have rebased Shlomo's patches on
> yesterday's master, and during this i added parent_fiq[] to GICv3, because without filling
> these in the thing stopped working due to recent GICv2 code changes. I needed to fix the
> problem quickly, so i copied initializing parent_fiq together with security_extn field
> from v2 code. But, this makes no sense because security_extn is never initialized, it is
> not even assigned property name. So just remove this small fragment, it's not needed now.
>  I have fixed this in my working tree 2 hours ago. parent_fiq is declared as:
> --- cut ---
> qemu_irq parent_fiq[GICV3_NCPU];
> --- cut ---
> 

I think based on this comment that I can safely suggest a bit of advise
regarding sending patches to the QEMU and Linux lists:

Be as careful as you can, properly test your code, review your own
commit messages, run spell check on them, and try to look at what you
send out through the eyes of someone who has never seen this code
before.

Review resources are really scarse in these projects and almost all
maintainers are overloaded, so we all have to work together and make
sure we don't flood the lists unwarranted.

It is good to re-spin quickly so people don't loose context, but if you
are fixing things up in a matter of hours and sending out new revisions,
you are moving too fast.

Turn-around time is days/weeks in the best cases, so better to take a
few extra days to thoroughly test a patch series.  The exception is to
illustrate a general idea or design, in which case clearly marking the
series as an RFC and noting what people should look at and what people
should ignore is key.

Hope this makes sense,
-Christoffer