Re: [PATCH v2 2/5] apic: add support for x2APIC mode

[Bui Quang Minh]

On 3/27/23 18:04, David Woodhouse wrote:
> On Sun, 2023-03-26 at 12:20 +0700, Bui Quang Minh wrote:
> > This commit extends the APIC ID to 32-bit long and remove the 255 max APIC
> > ID limit in userspace APIC. The array that manages local APICs is now
> > dynamically allocated based on the max APIC ID of created x86 machine.
> > Also, new x2APIC IPI destination determination scheme, self IPI and x2APIC
> > mode register access are supported.
> >
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> >   hw/i386/x86.c                   |   8 +-
> >   hw/intc/apic.c                  | 229 +++++++++++++++++++++++---------
> >   hw/intc/apic_common.c           |   8 +-
> >   include/hw/i386/apic.h          |   3 +-
> >   include/hw/i386/apic_internal.h |   2 +-
> >   5 files changed, 184 insertions(+), 66 deletions(-)
> >
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index a88a126123..fa9b15190d 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -132,11 +132,11 @@ void x86_cpus_init(X86MachineState *x86ms, int 
> > default_cpu_version)
> >        * Can we support APIC ID 255 or higher?
> >        *
> >        * Under Xen: yes.
> > -     * With userspace emulated lapic: no
> > +     * With userspace emulated lapic: yes.
>
> Are you making this unconditional? It shall not be possible to emulate
> a CPU *without* X2APIC?

You are right, this should report error when APIC ID is higher than 255 
and x2APIC is not supported by the CPU.

> >        * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> >        */
> >       if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> > -        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
> > +        kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> >           error_report("current -smp configuration requires kernel "
> >                        "irqchip and X2APIC API support.");
> >           exit(EXIT_FAILURE);
> ...
> > @@ -276,16 +288,17 @@ static void apic_bus_deliver(const uint32_t 
> > *deliver_bitmask,
> >                    apic_set_irq(apic_iter, vector_num, trigger_mode) );
> >   }
> >   
> > -void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> > +void apic_deliver_irq(uint32_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> >                         uint8_t vector_num, uint8_t trigger_mode)
>
> We can make this 'static' while we're here. It isn't actually called
> from anywhere else, is it?

I'll fix it in the next version.

> >   
> >   static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> > -                                      uint8_t dest, uint8_t dest_mode)
> > +                                      uint32_t dest, uint8_t dest_mode)
> >   {
> >       APICCommonState *apic_iter;
> >       int i;
> >   
> > +    memset(deliver_bitmask, 0x00, max_apic_words * sizeof(uint32_t));
> > +
> > +    /* x2APIC broadcast id for both physical and logical (cluster) mode */
> > +    if (dest == 0xffffffff) {
> > +        apic_get_broadcast_bitmask(deliver_bitmask, true);
> > +        return;
> > +    }
> > +
> >       if (dest_mode == 0) {
>
> Might be nice to have a constant for DEST_MODE_PHYS vs.
> DEST_MODE_LOGICAL to make this clearer?

I'll fix it in the next version.

> > +        apic_find_dest(deliver_bitmask, dest);
> > +        /* Broadcast to xAPIC mode apics */
> >           if (dest == 0xff) {
> > -            memset(deliver_bitmask, 0xff, MAX_APIC_WORDS * sizeof(uint32_t));
> > -        } else {
> > -            int idx = apic_find_dest(dest);
> > -            memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
> > -            if (idx >= 0)
> > -                apic_set_bit(deliver_bitmask, idx);
> > +            apic_get_broadcast_bitmask(deliver_bitmask, false);
>
>
> Hrm... aren't you still interpreting destination 0x000000FF as
> broadcast even for X2APIC mode? Or am I misreading this?

In case the destination is 0xFF, the IPI will be delivered to CPU has 
APIC ID 0xFF if it is in x2APIC mode, and it will be delivered to all 
CPUs that are in xAPIC mode. In case the destination is 0xFFFFFFFF, the 
IPI is delivered to all CPUs that are in x2APIC mode. I've created 
apic_get_broadcast_bitmask function and changed the apic_find_dest to 
implement that logic.

> > @@ -366,7 +370,7 @@ static const VMStateDescription vmstate_apic_common = {
> >           VMSTATE_UINT8(arb_id, APICCommonState),
> >           VMSTATE_UINT8(tpr, APICCommonState),
> >           VMSTATE_UINT32(spurious_vec, APICCommonState),
> > -        VMSTATE_UINT8(log_dest, APICCommonState),
> > +        VMSTATE_UINT32(log_dest, APICCommonState),
> >           VMSTATE_UINT8(dest_mode, APICCommonState),
> >           VMSTATE_UINT32_ARRAY(isr, APICCommonState, 8),
> >           VMSTATE_UINT32_ARRAY(tmr, APICCommonState, 8),
>
>
> Hm, doesn't this need to be added in a separate subsection, much as
> ide_drive/pio_state in the example in docs/devel/migration.rst? Or did
> I *not* need to do that in commit ecb0e98b4 (unrelated to x2apic, but
> similar addition of state)?
>
> Can you confirm that you've tested the behaviour when migrating back
> from this to an older QEMU, both for a guest *with* X2APIC enabled
> (which should fail gracefully), and a guest without X2APIC (which
> should work).

Oh, thank you for pointing out, I actually don't understand the use of 
vmstate before, I'll look at the document more and fix it.

> > diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
> > index 2cebeb4faf..d938bfa8e0 100644
> > --- a/include/hw/i386/apic.h
> > +++ b/include/hw/i386/apic.h
> > @@ -3,7 +3,8 @@
> >   
> >   
> >   /* apic.c */
> > -void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> > +void apic_set_max_apic_id(uint32_t max_apic_id);
> > +void apic_deliver_irq(uint32_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> >                         uint8_t vector_num, uint8_t trigger_mode);
>
> Making it static means this can be removed, of course.

I'll fix it in next version.

Thanks,
Quang Minh.