[Qemu-devel] Re: [PATCH 1/6] provide in-kernel ioapic

[Glauber Costa <glommer@redhat.com>]

On Mon, Sep 28, 2009 at 11:50:46PM +0200, Juan Quintela wrote:
> Glauber Costa <glommer@redhat.com> wrote:
> > This patch provides kvm with an in-kernel ioapic. We are currently not enabling it.
> > The code is heavily based on what's in qemu-kvm.git.
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> 
> 
> base_address IOAPICState field missing.
> 
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
> > +{
> > +    struct kvm_irqchip chip;
> > +    struct kvm_ioapic_state *kioapic;
> > +    int i;
> > +
> 
> if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
>    return;
No need. if this function is ever called and this is not true, this is a bug.

> 
> > +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> > +    kioapic = &chip.chip.ioapic;
> > +
> > +    kioapic->id = s->id;
> > +    kioapic->ioregsel = s->ioregsel;
> > +    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
> > +    kioapic->irr = s->irr;
> > +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> > +        kioapic->redirtbl[i].bits = s->ioredtbl[i];
> > +    }
> > +
> > +    kvm_set_irqchip(&chip);
> > +}
> > +
> > +static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
> > +{
> > +    struct kvm_irqchip chip;
> > +    struct kvm_ioapic_state *kioapic;
> > +    int i;
> 
> if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
>    return;
ditto.

> 
> > +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> > +    kvm_get_irqchip(&chip);
> > +    kioapic = &chip.chip.ioapic;
> > +
> > +    s->id = kioapic->id;
> > +    s->ioregsel = kioapic->ioregsel;
> > +    s->irr = kioapic->irr;
> > +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> > +        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
> > +    }
> > +}
> > +#endif
> > +
> > +static void ioapic_pre_save(const void *opaque)
> > +{
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +    IOAPICState *s = (void *)opaque;
> > +
> > +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> 
> Can we put this test inside the functions?  Yes, I know that qemu-kvm
> uses always this syntax, but it just makes the API simpler to use.

I am not sure if we are aiming for a simpler api here. Those functions should never
be called if we're not using in-kernel irqchip, and it is probably better to have
callers to enforce it.

> 
> > +        kvm_kernel_ioapic_save_to_user(s);
> > +    }
> > +#endif
> > +}
> > +
> > +static int ioapic_pre_load(void *opaque)
> > +{
> > +    IOAPICState *s = opaque;
> > +
> > +    /* in case we are doing version 1, we just set these to sane
> > values */
> 
> You forgot to update vmstate_ioapic to version 2.
ok.

> 
> > +    s->irr = 0;
> > +    return 0;
> > +}
> > +
> > +static int ioapic_post_load(void *opaque)
> > +{
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +    IOAPICState *s = opaque;
> > +
> > +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> > +        kvm_kernel_ioapic_load_from_user(s);
> > +    }
> > +#endif
> > +    return 0;
> > +}
> > +
> > +
> >  static const VMStateDescription vmstate_ioapic = {
> >      .name = "ioapic",
> >      .version_id = 1,
> > @@ -201,7 +276,10 @@ static const VMStateDescription vmstate_ioapic = {
> >          VMSTATE_UINT8(ioregsel, IOAPICState),
> >          VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
> >          VMSTATE_END_OF_LIST()
> > -    }
> > +    },
> > +    .pre_load = ioapic_pre_load,
> > +    .post_load = ioapic_post_load,
> > +    .pre_save = ioapic_pre_save,
> >  };
>