Re: [Qemu-devel] [PATCH v10 4/5] hw/intc: Initial implementation of vGICv3

[Eric Auger <eric.auger@linaro.org>]

Hi Pavel,
On 08/18/2015 03:33 PM, Pavel Fedin wrote:
> This is the initial version of KVM-accelerated GICv3 support.
> State load and save are not yet supported, live migration is
> not possible.
> 
> In order to get correct class name in a simpler way, gicv3_class_name()
> function is implemented, similar to gic_class_name().
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
in v8 you got Peter's R-b. Did you omit it willingly or did you make any
changes since that. A changelog would ease the review in any case.


> ---
>  hw/intc/Makefile.objs   |   1 +
>  hw/intc/arm_gicv3_kvm.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/kvm_arm.h    |  10 ++++
>  target-arm/machine.c    |  18 ++++++
>  4 files changed, 178 insertions(+)
>  create mode 100644 hw/intc/arm_gicv3_kvm.c
> 
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 1317e5a..004b0c2 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-$(call land,$(CONFIG_ARM_GIC_KVM),$(TARGET_AARCH64)) += 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..8070a2a
> --- /dev/null
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -0,0 +1,149 @@
> +/*
> + * ARM Generic Interrupt Controller using KVM in-kernel support
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + * Written by Pavel Fedin
> + * Based on vGICv2 code by Peter Maydell
> + *
> + * 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/intc/arm_gicv3_common.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/kvm.h"
> +#include "kvm_arm.h"
> +#include "vgic_common.h"
> +
> +#ifdef DEBUG_GICV3_KVM
> +#define DPRINTF(fmt, ...) \
> +    do { fprintf(stderr, "kvm_gicv3: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +#define TYPE_KVM_ARM_GICV3 "kvm-arm-gicv3"
> +#define KVM_ARM_GICV3(obj) \
> +     OBJECT_CHECK(GICv3State, (obj), TYPE_KVM_ARM_GICV3)
> +#define KVM_ARM_GICV3_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(KVMARMGICv3Class, (klass), TYPE_KVM_ARM_GICV3)
> +#define KVM_ARM_GICV3_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(KVMARMGICv3Class, (obj), TYPE_KVM_ARM_GICV3)
> +
> +typedef struct KVMARMGICv3Class {
> +    ARMGICv3CommonClass parent_class;
> +    DeviceRealize parent_realize;
> +    void (*parent_reset)(DeviceState *dev);
> +} KVMARMGICv3Class;
> +
> +static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level)
> +{
> +    GICv3State *s = (GICv3State *)opaque;
> +
> +    kvm_arm_gic_set_irq(s->num_irq, irq, level);
> +}
> +
> +static void kvm_arm_gicv3_put(GICv3State *s)
> +{
> +    /* TODO */
> +    DPRINTF("Cannot put kernel gic state, no kernel interface\n");
> +}
> +
> +static void kvm_arm_gicv3_get(GICv3State *s)
> +{
> +    /* TODO */
> +    DPRINTF("Cannot get kernel gic state, no kernel interface\n");
> +}
> +
> +static void kvm_arm_gicv3_reset(DeviceState *dev)
> +{
> +    GICv3State *s = ARM_GICV3_COMMON(dev);
> +    KVMARMGICv3Class *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)
> +{
> +    GICv3State *s = KVM_ARM_GICV3(dev);
> +    KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +    Error *local_err = NULL;
> +
> +    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) {
> +        error_setg(errp, "the in-kernel VGICv3 does not implement the "
> +                   "security extensions");
> +        return;
> +    }
> +
> +    gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL);
> +
> +    /* Try to create the device via the device control API */
> +    s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +    if (s->dev_fd < 0) {
> +        error_setg_errno(errp, -s->dev_fd, "error creating in-kernel VGIC");
might be helful to tell which one we fail in creating
> +        return;
> +    }
> +
> +    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> +                   0, 0, &s->num_irq, 1);
> +
> +    /* Tell the kernel to complete VGIC initialization now */
> +    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                   KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
> +
> +    kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                            KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
> +    kvm_arm_register_device(&s->iomem_redist, -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);
> +    ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_CLASS(klass);
> +    KVMARMGICv3Class *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(GICv3State),
> +    .class_init = kvm_arm_gicv3_class_init,
> +    .class_size = sizeof(KVMARMGICv3Class),
> +};
> +
> +static void kvm_arm_gicv3_register_types(void)
> +{
> +    type_register_static(&kvm_arm_gicv3_info);
> +}
> +
> +type_init(kvm_arm_gicv3_register_types)
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index b3e0ab7..b6f5b69 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -196,4 +196,14 @@ static inline const char *gic_class_name(void)
>      return kvm_irqchip_in_kernel() ? "kvm-arm-gic" : "arm_gic";
>  }
>  
> +/**
> + * gicv3_class_name
> + *
> + * Return name of GICv3 class to use depending on whether KVM acceleration is
> + * in use. May throw an error if the chosen implementation is not available.
> + *
> + * Returns: class name to use
> + */
> +const char *gicv3_class_name(void);
> +
>  #endif
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 32adfe7..c6cdde2 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -1,5 +1,6 @@
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> +#include "qemu/error-report.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "internals.h"
> @@ -328,3 +329,20 @@ const VMStateDescription vmstate_arm_cpu = {
>          NULL
>      }
>  };
> +
> +const char *gicv3_class_name(void)
> +{
> +    if (kvm_irqchip_in_kernel()) {
> +#ifdef TARGET_AARCH64    
checkpatch reports a trailing whitespace error above

Best Regards

Eric
> +        return "kvm-arm-gicv3";
> +#else
> +        error_report("KVM GICv3 acceleration is not supported on this "
> +                     "platform\n");
> +#endif
> +    } else {
> +        /* TODO: Software emulation is not implemented yet */
> +        error_report("KVM is currently required for GICv3 emulation\n");
> +    }
> +
> +    exit(1);
> +}
>