Re: [Qemu-devel] [PATCH v2 07/18] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

On Fri, Jun 24, 2016 at 06:05:55PM +0200, Igor Mammedov wrote:
> CPU added with device_add help won't have APIC ID set,
> so set it according to socket/core/thread ids provided
> with device_add command.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> v2:
>  - add validity checks for socket-id/core-id/thread-id values
> ---
>  hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index eaa4b59..0a65caf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,14 +1763,52 @@ static void pc_cpu_pre_plug(HotplugHandler 
> *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> +    CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
>      X86CPU *cpu = X86_CPU(dev);
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
>  
> +    /* if APIC ID is not set, set it based on socket/core/thread properties 
> */
> +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> +        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;

The calculation looks right, but I would love to move this to
common code that takes care of SMP configuration and topology,
instead of an inline calculation. We have been bitten by bugs
related to ad hoc topology calculations before (see commit
7dfddd7).

I think we can do that as a follow-up patch after Drew's series is included.

> +
[...]
> +    cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
>      if (!cpu_slot) {
> -        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
> -                   "), valid range 0:%d", cpu->apic_id,
> +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, 
> &topo);
> +        error_setg(errp, "Invalid CPU[socket: %d, core: %d, thread: %d] with"

topo.{pkg,core,smt}_id are unsigned. Treating them as signed can generate
confusing error messages:

 (qemu) device_add qemu64-x86_64-cpu,apic-id=0xfffffffe
 Invalid CPU[socket: -2, core: 0, thread: 0] with APIC ID (4294967294), valid 
index range 0:3

Also, I wonder if showing APIC ID in hex wouldn't be more useful.


> +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",
> +                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
>                     pcms->possible_cpus->len - 1);
>          return;
>      }
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo