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

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Sep 06, 2013 at 04:57:05PM +0200, Paolo Bonzini wrote:
> Il 25/08/2013 17:47, Alexander Graf ha scritto:
> > 
> > On 23.08.2013, at 21:10, Christoffer Dall 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 <christoffer.dall@linaro.org>
> >> ---
> >> 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),
> >>         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)
> > 
> > You really want to make DPRINTF still be format checked by the compiler. Check out hw/intc/openpic.c for an example how to get there.
> > 
> >> +#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?
> > 
> >> +
> >> +    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));
> >> +            abort();
> >> +    }
> >> +}
> >> +
> >> +static void kvm_arm_gic_dist_reg_access(GICState *s, int offset, int cpu,
> >> +                                        uint32_t *val, bool write)
> >> +{
> >> +    kvm_arm_gic_reg_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> >> +                           offset, cpu, val, write);
> >> +}
> >> +
> >> +static void kvm_arm_gic_cpu_reg_access(GICState *s, int offset, int cpu,
> >> +                                       uint32_t *val, bool write)
> >> +{
> >> +    kvm_arm_gic_reg_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();
> >> +    }
> > 
> > I don't understand the to_kernel bits. I thought we're in a device file that only works with KVM?
> 
> The opposite of "to_kernel" is "from_kernel".
> 
> IIUC this is for GIC registers that are modelled as a pair of MMIO
> registers, one with write-1-sets and one with write-1-clears semantics.
>  The patch is first writing all ones to the w1c register, and then
> writing the actual value to the w1s register.
> 
> The way we solved this for x86 is that the set-registers, when called
> from userspace, also clear the zero bits.  See the "host_initiated"
> parts in arch/x86/kvm/x86.c.  However, this would be an ABI change for
> ARM, so Christoffer's solution is also fine.

I prefer sticking to the ARM ABI so we don't have to keep a
documentation of exceptions to that around to the widest extend
possible.  See my comment on Alex's e-mail for a more thorough response
to this as well.

Sorry for the long response time on this.

> 
> One comment on the function names:
> 
> kvm_arm_gic_dist_readr
> kvm_arm_gic_dist_writer
> 
> Why not get_reg/set_reg (I was quite surprised to see readr instead of
> reader :) and it took me a while to understand the convention)?  Or if
> the name is too long, s/readr/get/ and s/writer/set/ would be enough to
> match the ioctls.
> 
r for register, read register, write register...

I thought I'd seen this convention elsewhere, but I may be wrong.  If
you feel really strongly about it, I can rename.

-Christoffer