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

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/08/2016 04:43 PM, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 16:03:43 +0200
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 03/08/2016 02:28 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?
>>
>> Definitely, thanks.
>>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Ops, I'm sorry but I've forgot to CC qemu-devel
> could you reply to list list pls?
>

Sure, cc-ed qemu-devel.

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