Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Alex Bennée]

address@hidden writes:

> Save and restore the ARM KVM VGIC state from the kernel.  We rely on
<snip>
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 6,
> -    .minimum_version_id = 6,
> +    .version_id = 7,
> +    .minimum_version_id = 7,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {

Does this mean QEMU and Kernel need to be kept in lock-step for
compatibility?

>  
> +//#define DEBUG_GIC_KVM
> +
> +#ifdef DEBUG_GIC_KVM
> +static const int debug_gic_kvm = 1;
> +#else
> +static const int debug_gic_kvm = 0;
> +#endif
> +
> +#define DPRINTF(fmt, ...) do { \
> +        if (debug_gic_kvm) { \
> +            printf("arm_gic: " fmt , ## __VA_ARGS__); \
> +        } \
> +    } while (0)
> +

Shouldn't we be using QEMU logging framework for this? Also for the
fprintfs later on.

>  #define TYPE_KVM_ARM_GIC "kvm-arm-gic"
>  #define KVM_ARM_GIC(obj) \
>       OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GIC)
> @@ -72,14 +87,419 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, 
> int level)
>      kvm_set_irq(kvm_state, kvm_irq, !!level);
>  }
>  
> +static bool kvm_arm_gic_can_save_restore(GICState *s)
> +{
> +    return s->dev_fd >= 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_gicd_access(GICState *s, int offset, int cpu,
> +                            uint32_t *val, bool write)
> +{
> +    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> +                   offset, cpu, val, write);
> +}
> +
> +static void kvm_gicc_access(GICState *s, int offset, int cpu,
> +                            uint32_t *val, bool write)
> +{
> +    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
> +                   offset, cpu, val, write);
> +}
> +
> +#define for_each_irq_reg(_ctr, _max_irq, _field_width) \
> +    for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
> +
> +/*
> + * Translate from the in-kernel field for an IRQ value to/from the qemu
> + * representation.
> + */
> +typedef void (*vgic_translate_fn)(GICState *s, int irq, int cpu,
> +                                  uint32_t *field, bool to_kernel);
> +
> +/* synthetic translate function used for clear/set registers to completely
> + * clear a setting using a clear-register before setting the remaing bits
> + * using a set-register */
> +static void translate_clear(GICState *s, int irq, int cpu,
> +                            uint32_t *field, bool to_kernel)
> +{
> +    if (to_kernel) {
> +        *field = ~0;
> +    } else {
> +        /* does not make sense: qemu model doesn't use set/clear regs */
> +        abort();
> +    }
> +}
> +
> +static void translate_enabled(GICState *s, int irq, int cpu,
> +                              uint32_t *field, bool to_kernel)
> +{
> +    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +    if (to_kernel) {
> +        *field = GIC_TEST_ENABLED(irq, cm);
> +    } else {
> +        if (*field & 1) {
> +            GIC_SET_ENABLED(irq, cm);
> +        }
> +    }
> +}
> +
> +static void translate_pending(GICState *s, int irq, int cpu,
> +                              uint32_t *field, bool to_kernel)
> +{
> +    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +    if (to_kernel) {
> +        *field = GIC_TEST_PENDING(irq, cm);
> +    } else {
> +        if (*field & 1) {
> +            GIC_SET_PENDING(irq, cm);
> +            /* TODO: Capture is level-line is held high in the kernel */
> +        }
> +    }
> +}
> +
> +static void translate_active(GICState *s, int irq, int cpu,
> +                             uint32_t *field, bool to_kernel)
> +{
> +    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +    if (to_kernel) {
> +        *field = GIC_TEST_ACTIVE(irq, cm);
> +    } else {
> +        if (*field & 1) {

Should 1, 0x2 etc be #define'd constants?

<snip>
>  static void kvm_arm_gic_put(GICState *s)
>  {
> -    /* TODO: there isn't currently a kernel interface to set the GIC state */
> +    uint32_t reg;
> +    int i;
> +    int cpu;
> +    int num_cpu;
> +    int num_irq;
> +
> +    if (!kvm_arm_gic_can_save_restore(s)) {
> +            DPRINTF("Cannot put kernel gic state, no kernel interface");
> +            return;
> +    }
> +
> +    /* Note: We do the restore in a slightly different order than the save
> +     * (where the order doesn't matter and is simply ordered according to the
> +     * register offset values */
> +
> +    /*****************************************************************
> +     * Distributor State
> +     */
> +
> +    /* s->enabled -> GICD_CTLR */
> +    reg = s->enabled;
> +    kvm_gicd_access(s, 0x0, 0, &reg, true);
> +
> +    /* Sanity checking on GICD_TYPER and s->num_irq, s->num_cpu */
> +    kvm_gicd_access(s, 0x4, 0, &reg, false);
> +    num_irq = ((reg & 0x1f) + 1) * 32;
> +    num_cpu = ((reg & 0xe0) >> 5) + 1;
> +
> +    if (num_irq < s->num_irq) {
> +            fprintf(stderr, "Restoring %u IRQs, but kernel supports max 
> %d\n",
> +                    s->num_irq, num_irq);
> +            abort();
> +    } else if (num_cpu != s->num_cpu ) {
> +            fprintf(stderr, "Restoring %u CPU interfaces, kernel only has 
> %d\n",
> +                    s->num_cpu, num_cpu);
> +            /* Did we not create the VCPUs in the kernel yet? */
> +            abort();
> +    }
> +
> +    /* TODO: Consider checking compatibility with the IIDR ? */
> +
> +    /* irq_state[n].enabled -> GICD_ISENABLERn */
> +    kvm_dist_put(s, 0x180, 1, s->num_irq, translate_clear);
> +    kvm_dist_put(s, 0x100, 1, s->num_irq, translate_enabled);
> +
> +    /* s->irq_target[irq] -> GICD_ITARGETSRn
> +     * (restore targets before pending to ensure the pending state is set on
> +     * the appropriate CPU interfaces in the kernel) */
> +    kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets);
> +
> +    /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */
> +    kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear);
> +    kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending);
> +
> +    /* irq_state[n].active -> GICD_ISACTIVERn */
> +    kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear);
> +    kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active);
> +
> +    /* irq_state[n].trigger -> GICD_ICFRn */
> +    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
> +
> +    /* s->priorityX[irq] -> ICD_IPRIORITYRn */
> +    kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority);
> +
> +    /* s->sgi_source -> ICD_CPENDSGIRn */
> +    kvm_dist_put(s, 0xf10, 8, GIC_NR_SGIS, translate_clear);
> +    kvm_dist_put(s, 0xf20, 8, GIC_NR_SGIS, translate_sgisource);

Again what are these magic numbers? I assume the comments refer to the
actual reg names in the manual? Perhaps putting a reference to the
manual if these values aren't to be used else where?

<snip>

-- 
Alex Bennée