Re: [PATCH v3 07/18] machine: Add a new function init_apicid_fn in MachineClass

From: Babu Moger <babu.moger@amd.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: ehabkost@redhat.com, mst@redhat.com, armbru@redhat.com,
	qemu-devel@nongnu.org, pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v3 07/18] machine: Add a new function init_apicid_fn in MachineClass
Date: Wed, 29 Jan 2020 10:17:11 -0600	[thread overview]
Message-ID: <b75928ec-c062-7edf-bc24-8dc0ccd7439b@amd.com> (raw)
In-Reply-To: <20200129101458.6a108431@redhat.com>

On 1/29/20 3:14 AM, Igor Mammedov wrote:
> On Tue, 28 Jan 2020 13:45:31 -0600
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> On 1/28/20 10:29 AM, Igor Mammedov wrote:
>>> On Tue, 03 Dec 2019 18:37:42 -0600
>>> Babu Moger <babu.moger@amd.com> wrote:
>>>   
>>>> Add a new function init_apicid_fn in MachineClass to initialize the mode
>>>> specific handlers to decode the apic ids.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>>  include/hw/boards.h |    1 +
>>>>  vl.c                |    3 +++
>>>>  2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>> index d4fab218e6..ce5aa365cb 100644
>>>> --- a/include/hw/boards.h
>>>> +++ b/include/hw/boards.h
>>>> @@ -238,6 +238,7 @@ struct MachineClass {
>>>>                                                           unsigned cpu_index);
>>>>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>>>>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
>>>> +    void (*init_apicid_fn)(MachineState *ms);  
>>> it's x86 specific, so why it wasn put into PCMachineClass?  
>>
>> Yes. It is x86 specific for now. I tried to make it generic function so
>> other OSes can use it if required(like we have done in
>> possible_cpu_arch_ids). It initializes functions required to build the
>> apicid for each CPUs. We need these functions much early in the
>> initialization. It should be initialized before parse_numa_opts or
>> machine_run_board_init(in v1.c) which are called from generic context. We
>> cannot use PCMachineClass at this time.
> 
> could you point to specific patches in this series that require
> apic ids being initialized before parse_numa_opts and elaborate why?
> 
> we already have possible_cpu_arch_ids() which could be called very
> early and calculates APIC IDs in x86 case, so why not reuse it?

The current code(before this series) parses the numa information and then
sequentially builds the apicid. Both are done together.

But this series separates the numa parsing and apicid generation. Numa
parsing is done first and after that the apicid is generated. Reason is we
need to know the number of numa nodes in advance to decode the apicid.

Look at this patch.
https://lore.kernel.org/qemu-devel/157541988471.46157.6587693720990965800.stgit@naples-babu.amd.com/

static inline apic_id_t apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
+                                                  const X86CPUTopoIDs
*topo_ids)
+{
+    return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
+           (topo_ids->llc_id << apicid_llc_offset_epyc(topo_info)) |
+           (topo_ids->die_id  << apicid_die_offset(topo_info)) |
+           (topo_ids->core_id << apicid_core_offset(topo_info)) |
+           topo_ids->smt_id;
+}

The function apicid_from_topo_ids_epyc builds the apicid. New decode adds
llc_id(which is numa id here) to the current decoding. Other fields are
mostly remains same.

Details from the bug https://bugzilla.redhat.com/show_bug.cgi?id=1728166

Processor Programming Reference (PPR) for AMD Family 17h Model 01h,
Revision B1 Processors:

"""
2.1.10.2.1.3
ApicId Enumeration Requirements
Operating systems are expected to use
Core::X86::Cpuid::SizeId[ApicIdCoreIdSize], the number of least
significant bits in the Initial APIC ID that indicate core ID within a
processor, in constructing per-core CPUID
masks. Core::X86::Cpuid::SizeId[ApicIdCoreIdSize] determines the maximum
number of cores (MNC) that the
processor could theoretically support, not the actual number of cores that
are actually implemented or enabled on
the processor, as indicated by Core::X86::Cpuid::SizeId[NC].
Each Core::X86::Apic::ApicId[ApicId] register is preset as follows:
• ApicId[6] = Socket ID.
• ApicId[5:4] = Node ID.
• ApicId[3] = Logical CCX L3 complex ID
• ApicId[2:0]= (SMT) ? {LogicalCoreID[1:0],ThreadId} :
{1'b0,LogicalCoreID[1:0]}.
"""

> 
>>
>>>
>>>   
>>>>  };
>>>>  
>>>>  /**
>>>> diff --git a/vl.c b/vl.c
>>>> index a42c24a77f..b6af604e11 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp)
>>>>      current_machine->cpu_type = machine_class->default_cpu_type;
>>>>      if (cpu_option) {
>>>>          current_machine->cpu_type = parse_cpu_option(cpu_option);
>>>> +        if (machine_class->init_apicid_fn) {
>>>> +            machine_class->init_apicid_fn(current_machine);
>>>> +        }
>>>>      }
>>>>      parse_numa_opts(current_machine);
>>>>  
>>>>
>>>>  
>>>   
>>
> 