Re: [Qemu-devel] [PATCH v5 03/10] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 03/08/2016 06:29 PM, Igor Mammedov wrote:
> On Thu,  3 Mar 2016 15:28:56 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
>> on x86 currently range 0..max_cpus is used to generate
>> architecture-dependent CPU ID (APIC Id) for each present
>> and possible CPUs. However architecture-dependent CPU IDs
>> list could be sparse and code that needs to enumerate
>> all IDs (ACPI) ended up doing guess work enumerating all
>> possible and impossible IDs up to
>>    apic_id_limit = x86_cpu_apic_id_from_index(max_cpus).
>>
>> That leads to creation of MADT entries and Processor
>> objects in ACPI tables for not possible CPUs.
>> Fix it by allowing board specify a concrete list of
>> CPU IDs accourding its own rules (which for x86 depends
>> on topology). So that code that needs this list could
>> request it from board instead of trying to guess
>> what IDs are correct on its own.
>>
>> This interface will also allow to help making AML
>> part of CPU hotplug target independent so it could
>> be reused for ARM target.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>> v5:
>>    s/CPUArchId cpus[1]/CPUArchId cpus[0]/ to make length
>>    calculation simpler. Marcel Apfelbaum <marcel@redhat.com>
> Marcel, does v5 looks good to you?
>
> /me, sorry for replying/asking out of thread the first time./
>

No problem, I think I already sent this, but to be sure

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel


>> ---
>>   hw/i386/pc.c         | 44 ++++++++++++++++++++++++++++++++++++++++----
>>   include/hw/boards.h  | 26 ++++++++++++++++++++++++++
>>   include/hw/i386/pc.h |  1 +
>>   3 files changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 151a64c..1af95a1 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1132,10 +1132,17 @@ void pc_cpus_init(PCMachineState *pcms)
>>           exit(1);
>>       }
>>
>> -    for (i = 0; i < smp_cpus; i++) {
>> -        cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
>> -                         &error_fatal);
>> -        object_unref(OBJECT(cpu));
>> +    pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
>> +                                    sizeof(CPUArchId) * max_cpus);
>> +    for (i = 0; i < max_cpus; i++) {
>> +        pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
>> +        pcms->possible_cpus->len++;
>> +        if (i < smp_cpus) {
>> +            cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
>> +                             &error_fatal);
>> +            pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
>> +            object_unref(OBJECT(cpu));
>> +        }
>>       }
>>
>>       /* tell smbios about cpuid version and features */
>> @@ -1658,9 +1665,19 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
>>       error_propagate(errp, local_err);
>>   }
>>
>> +static int pc_apic_cmp(const void *a, const void *b)
>> +{
>> +   CPUArchId *apic_a = (CPUArchId *)a;
>> +   CPUArchId *apic_b = (CPUArchId *)b;
>> +
>> +   return apic_a->arch_id - apic_b->arch_id;
>> +}
>> +
>>   static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>>                           DeviceState *dev, Error **errp)
>>   {
>> +    CPUClass *cc = CPU_GET_CLASS(dev);
>> +    CPUArchId apic_id, *found_cpu;
>>       HotplugHandlerClass *hhc;
>>       Error *local_err = NULL;
>>       PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> @@ -1683,6 +1700,13 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>>
>>       /* increment the number of CPUs */
>>       rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
>> +
>> +    apic_id.arch_id = cc->get_arch_id(CPU(dev));
>> +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
>> +        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
>> +        pc_apic_cmp);
>> +    assert(found_cpu);
>> +    found_cpu->cpu = CPU(dev);
>>   out:
>>       error_propagate(errp, local_err);
>>   }
>> @@ -1925,6 +1949,17 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>>       return topo.pkg_id;
>>   }
>>
>> +static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
>> +{
>> +    PCMachineState *pcms = PC_MACHINE(machine);
>> +    int len = sizeof(CPUArchIdList) +
>> +              sizeof(CPUArchId) * (pcms->possible_cpus->len);
>> +    CPUArchIdList *list = g_malloc(len);
>> +
>> +    memcpy(list, pcms->possible_cpus, len);
>> +    return list;
>> +}
>> +
>>   static void pc_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -1947,6 +1982,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>>       pcmc->save_tsc_khz = true;
>>       mc->get_hotplug_handler = pc_get_hotpug_handler;
>>       mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
>> +    mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
>>       mc->default_boot_order = "cad";
>>       mc->hot_add_cpu = pc_hot_add_cpu;
>>       mc->max_cpus = 255;
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index b5d7eae..4b3fdbe 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -8,6 +8,7 @@
>>   #include "sysemu/accel.h"
>>   #include "hw/qdev.h"
>>   #include "qom/object.h"
>> +#include "qom/cpu.h"
>>
>>   void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>>                                             const char *name,
>> @@ -42,6 +43,26 @@ bool machine_dump_guest_core(MachineState *machine);
>>   bool machine_mem_merge(MachineState *machine);
>>
>>   /**
>> + * CPUArchId:
>> + * @arch_id - architecture-dependent CPU ID of present or possible CPU
>> + * @cpu - pointer to corresponding CPU object if it's present on NULL otherwise
>> + */
>> +typedef struct {
>> +    uint64_t arch_id;
>> +    struct CPUState *cpu;
>> +} CPUArchId;
>> +
>> +/**
>> + * CPUArchIdList:
>> + * @len - number of @CPUArchId items in @cpus array
>> + * @cpus - array of present or possible CPUs for current machine configuration
>> + */
>> +typedef struct {
>> +    int len;
>> +    CPUArchId cpus[0];
>> +} CPUArchIdList;
>> +
>> +/**
>>    * MachineClass:
>>    * @get_hotplug_handler: this function is called during bus-less
>>    *    device hotplug. If defined it returns pointer to an instance
>> @@ -57,6 +78,10 @@ bool machine_mem_merge(MachineState *machine);
>>    *    Set only by old machines because they need to keep
>>    *    compatibility on code that exposed QEMU_VERSION to guests in
>>    *    the past (and now use qemu_hw_version()).
>> + * @possible_cpu_arch_ids:
>> + *    Returns an array of @CPUArchId architecture-dependent CPU IDs
>> + *    which includes CPU IDs for present and possible to hotplug CPUs.
>> + *    Caller is responsible for freeing returned list.
>>    */
>>   struct MachineClass {
>>       /*< private >*/
>> @@ -98,6 +123,7 @@ struct MachineClass {
>>       HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>                                              DeviceState *dev);
>>       unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
>> +    CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>>   };
>>
>>   /**
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 8b3546e..3e09232 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -65,6 +65,7 @@ struct PCMachineState {
>>       /* CPU and apic information: */
>>       bool apic_xrupt_override;
>>       unsigned apic_id_limit;
>> +    CPUArchIdList *possible_cpus;
>>
>>       /* NUMA information: */
>>       uint64_t numa_nodes;
>
>