Re: [Qemu-devel] [PATCH 27/27] pc: generate APIC IDs according to CPU topology

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 24 Oct 2012 15:50:01 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This keeps compatibility on machine-types pc-1.2 and older, and prints a
> warning in case the requested configuration won't get the correct
> topology.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  - Move code to cpu.c
>  - keep using cpu_index on *-user
>  - Use SMP.contiguous_apic_ids global property
>  - Prints warning in case the compatibility mode will expose incorrect
>    topology
> 
> Changes v2 -> v3:
>  - Now all code is inside hw/pc.c
>  - Use a real "PC" class and a "contiguous_apic_ids" property
> 
> Changes v3 -> v4:
>  - Instead of using a global property, use a separate machine init
>    function and a PCInitArgs field, to implement compatibility mode
>  - Use error_report() instead of fprintf(stderr) for the warning
>  - Use a field on PCInitArgs instead of a static variable to check
>    if warning was already printed
> ---
>  hw/pc.c      | 19 +++++++++++++++----
>  hw/pc.h      |  6 ++++++
>  hw/pc_piix.c |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 8895087..3d08271 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -51,6 +51,8 @@
>  #include "exec-memory.h"
>  #include "arch_init.h"
>  #include "bitmap.h"
> +#include "topology.h"
> +#include "cpus.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -562,10 +564,19 @@ static void bochs_bios_write(void *opaque, uint32_t
> addr, uint32_t val) */
>  static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
>  {
> -    /* right now APIC ID == CPU index. this will eventually change to use
> -     * the CPU topology configuration properly
> -     */
> -    return cpu_index;
> +    uint32_t correct_id;
> +
> +    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
> +    if (args->compat_contiguous_apic_ids) {
> +        if (cpu_index != correct_id && !args->apic_id_warned) {
> +            error_report("APIC IDs set in compatibility mode, "
> +                         "CPU topology won't match the configuration");
> +            args->apic_id_warned = true;
> +        }
> +        return cpu_index;
> +    } else {
> +        return correct_id;
> +    }
>  }
>  
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> diff --git a/hw/pc.h b/hw/pc.h
> index 53883f5..a14944a 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -81,6 +81,12 @@ typedef struct PCInitArgs {
>      QEMUMachineInitArgs *qemu_args;
>      bool pci_enabled;
>      bool kvmclock_enabled;
> +    /* Enable compatibility (buggy) APIC ID generation, that keep APIC IDs
> +     * contiguous
> +     */
> +    bool compat_contiguous_apic_ids;
> +    /* Warning message about incorrect APIC ID was shown */
> +    bool apic_id_warned;
>  
>      /* Memory regions & sizes: */
>      MemoryRegion *system_memory;
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 57a3228..79a54e6 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -298,6 +298,7 @@ static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
>          .qemu_args = args,
>          .pci_enabled = true,
>          .kvmclock_enabled = true,
> +        .compat_contiguous_apic_ids = true,
I suppose libvirt/whatever would know that it's starting v1_2 machine, so it
would be able to provide valid APIC ID at hotplug time. But it looks fragile.

As Anthony once suggested, perhaps we could model sockets as links.
/machine/cpu[apic_id..]
So machine at level we would have a pre-defined topology with apic_id in link
name serving as initial apic_id selector pins in socket.

Pre-allocate them (sockets) in pc_cpus_init() for all possible CPUs and when
creating CPUs, apic_id property setter could accept link path and get
appropriate apic_id from link name and assign itself to a specified link.

It would be friendly with hot-plug and
-device X86CPU,id='/machine/cpu[apic_id]' cmdline if we ever decide in future
to create CPUs this way.

>      };
>      pc_init1(&pc_args);
>  }