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

[Peter Maydell]

On 23 August 2013 21:10, Christoffer Dall <address@hidden> wrote:
> Save and restore the ARM KVM VGIC state from the kernel.  We rely on
> QEMU to marshal the GICState data structure and therefore simply
> synchronize the kernel state with the QEMU emulated state in both
> directions.
>
> We take some care on the restore path to check the VGIC has been
> configured with enough IRQs and CPU interfaces that we can properly
> restore the state, and for separate set/clear registers we first fully
> clear the registers and then set the required bits.
>
> Signed-off-by: Christoffer Dall <address@hidden>
> ---
>  hw/intc/arm_gic_common.c    |    2 +
>  hw/intc/arm_gic_kvm.c       |  418 
> ++++++++++++++++++++++++++++++++++++++++++-
>  hw/intc/gic_internal.h      |    1 +
>  include/migration/vmstate.h |    6 +
>  4 files changed, 425 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index a50ded2..f39bdc1 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -77,6 +77,8 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
>          VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
>          VMSTATE_UINT8_2DARRAY(binary_point, GICState, 2, NCPU),
> +        VMSTATE_UINT32_2DARRAY(active_prio, GICState, 4, NCPU),
> +        VMSTATE_UINT32(num_irq, GICState),

You don't need to save and restore num_irq, it is constant
for the lifetime of the device (set by a property on the
device which is fixed by the board model). Migration only
works between two identical command lines; if the command
lines are identical at each end then num_irq should be too.

>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 9f0a852..9785281 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -23,6 +23,15 @@
>  #include "kvm_arm.h"
>  #include "gic_internal.h"
>
> +//#define DEBUG_GIC_KVM
> +
> +#ifdef DEBUG_GIC_KVM
> +#define DPRINTF(fmt, ...) \
> +do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while(0)
> +#endif
> +
>  #define TYPE_KVM_ARM_GIC "kvm-arm-gic"
>  #define KVM_ARM_GIC(obj) \
>       OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GIC)
> @@ -73,14 +82,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)
> +{
> +    KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
> +    return kgc->dev_fd >= 0;
> +}
> +
> +static void kvm_arm_gic_reg_access(GICState *s, int group, int offset,
> +                                   int cpu, uint32_t *val, bool write)
> +{
> +    KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
> +    struct kvm_device_attr attr;
> +    int type;
> +
> +    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 = (uint64_t)(long)val;

(uintptr_t) should suffice.

> +
> +    if (write) {
> +        type = KVM_SET_DEVICE_ATTR;
> +    } else {
> +        type = KVM_GET_DEVICE_ATTR;
> +    }
> +
> +    if (kvm_device_ioctl(kgc->dev_fd, type, &attr) < 0) {
> +            fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
> +                    strerror(errno));

Your kvm_device_ioctl returns -errno rather than setting
errno, doesn't it?

> +            abort();
> +    }
> +}
> +

> +/* Read a register group from the kernel VGIC */
> +static void kvm_arm_gic_dist_readr(GICState *s, uint32_t offset, int width,
> +                                   int maxirq, vgic_translate_fn 
> translate_fn)
> +{
> +    uint32_t reg;
> +    int i;
> +    int j;
> +    int irq;
> +    int cpu;
> +    int regsz = 32 / width; /* irqs per kernel register */
> +    uint32_t field;
> +
> +    for_each_irq_reg(i, maxirq, width) {
> +        irq = i * regsz;
> +        cpu = 0;
> +        while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
> +            kvm_arm_gic_dist_reg_access(s, offset, cpu, &reg, false);
> +            for (j = 0; j < regsz; j++) {
> +                field = reg >> (j * width) & ((1 << width) - 1);

    field = extract32(reg, j * width, width);

> +                translate_fn(s, irq + j, cpu, &field, false);
> +            }
> +
> +            cpu++;
> +        }
> +        offset += 4;
> +    }
> +}
> +
> +/* Write a register group to the kernel VGIC */
> +static void kvm_arm_gic_dist_writer(GICState *s, uint32_t offset, int width,
> +                                    int maxirq, vgic_translate_fn 
> translate_fn)
> +{
> +    uint32_t reg;
> +    int i;
> +    int j;
> +    int irq;
> +    int cpu;
> +    int regsz = 32 / width; /* irqs per kernel register */
> +    uint32_t field;
> +
> +    for_each_irq_reg(i, maxirq, width) {
> +        irq = i * regsz;
> +        cpu = 0;
> +        while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
> +            reg = 0;
> +            for (j = 0; j < regsz; j++) {
> +                translate_fn(s, irq + j, cpu, &field, true);
> +                reg |= (field & ((1 << width) - 1)) << (j * width);

   reg = deposit32(reg, j * width, width, field);

> +            }
> +            kvm_arm_gic_dist_reg_access(s, offset, cpu, &reg, true);
> +
> +            cpu++;
> +        }
> +        offset += 4;
> +    }
> +}


>  static void kvm_arm_gic_reset(DeviceState *dev)
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 424a41f..9771163 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -100,6 +100,7 @@ typedef struct GICState {
>
>      /* these registers are mainly used for save/restore of KVM state */
>      uint8_t  binary_point[2][NCPU]; /* [0]: group 0, [1]: group 1 */
> +    uint32_t active_prio[4][NCPU]; /* implementation defined layout */

You can't make this impdef in QEMU's state, that would mean
we couldn't do migration between implementations which
use different layouts. We need to pick a standard ("whatever
the ARM v2 GIC implementation is" seems the obvious choice)
and make the kernel convert if it's on an implementation which
doesn't follow that version.

>      uint32_t num_cpu;
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1c31b5d..e5538c7 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -633,9 +633,15 @@ extern const VMStateInfo vmstate_info_bitmap;
>  #define VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint32, uint32_t)
>
> +#define VMSTATE_UINT32_2DARRAY_V(_f, _s, _n1, _n2, _v)                \
> +    VMSTATE_2DARRAY(_f, _s, _n1, _n2, _v, vmstate_info_uint32, uint32_t)
> +
>  #define VMSTATE_UINT32_ARRAY(_f, _s, _n)                              \
>      VMSTATE_UINT32_ARRAY_V(_f, _s, _n, 0)
>
> +#define VMSTATE_UINT32_2DARRAY(_f, _s, _n1, _n2)                      \
> +    VMSTATE_UINT32_2DARRAY_V(_f, _s, _n1, _n2, 0)
> +
>  #define VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)                        \
>      VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint64, uint64_t)

Can you put the vmstate improvements in their own patch, please?

thanks
-- PMM