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

[David Woodhouse]

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?


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

>  {
> -    uint32_t deliver_bitmask[MAX_APIC_WORDS];
> +    uint32_t *deliver_bitmask = g_malloc(max_apic_words * sizeof(uint32_t));
>  
>      trace_apic_deliver_irq(dest, dest_mode, delivery_mode, vector_num,
>                             trigger_mode);
>  
>      apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
>      apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, 
> trigger_mode);
> +    g_free(deliver_bitmask);
>  }
>  
>  bool is_x2apic_mode(DeviceState *dev)
...
>  
>  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? 

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


>          }
>      } else {
>          /* XXX: cluster mode */
> 
...

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

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

smime.p7s
Description: S/MIME cryptographic signature