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

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

On Fri, Sep 27, 2013 at 09:11:18AM +0100, Alex Bennée wrote:
> 
> christoffer.dall@linaro.org writes:
> 

[...]

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

It would be a little tedious.  The whole bunch of emulation files for
the GIC here are sort of predicated on the fact that you know how to
find the GIC manual and can see the register encodings and determine the
offsets.

There's also very little reuse for these, as this is the only place
where we translate the qemu-specific data structure format to the KVM
API format.

That being said, I wouldn't mind cleaning up a lot of this code and
adding defines for the register offsets would at least be very helpful,
but for now I'm sticking with the current style, trying to get this code
finished, and then I'll follow up with a cleanup patch set, which takes
care of the whole lot - not just the KVM interface specific bits.

> >  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?
> 
These are offsets (as indicated by the parameter name in kvm_dist_put)
for the corresponding registers, and yes, the comments are the reg names
used in the manual.  As I said above, you're lost here if you're not
looking at the manual while looking at the code, but I really do want to
clean up this part and share the offsets with arm_gic.c later, add nice
references to the manual and a whole bunch of other stuff that could be
made much nicer, but I'd really like to get this functional piece of
code in first.

Thanks,
-Christoffer