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

[Christoffer Dall]

On Fri, Sep 06, 2013 at 04:13:32PM +0100, Peter Maydell wrote:
> 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.
> 

ok

> >          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.
> 

yes

> > +
> > +    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?
> 

yes, it does

> > +            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);
> 

ok

> > +                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);
> 

ok

> > +            }
> > +            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.
> 

Implementation defined as in implementation defined in the
architecture.  I didn't think it would make sense to choose a format for
an a15 implementation, for example, and then translate to that format
for other cores using the ARM gic.  Wouldn't migration only be support
for same qemu model to same qemu model, in which case the format of this
register would always be the same, and the kernel must return a format
corresponding to the target cpu.  Am I missing something here?

> >      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?
> 

yes,

-Christoffer