Re: [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState

[Babu Moger]

On 2/25/20 9:32 AM, Eduardo Habkost wrote:
> On Mon, Feb 24, 2020 at 05:13:18PM -0600, Babu Moger wrote:
>>
>>
>> On 2/24/20 4:31 PM, Eduardo Habkost wrote:
>>> On Mon, Feb 24, 2020 at 11:58:09AM -0600, Babu Moger wrote:
>>>>
>>>>
>>>> On 2/24/20 11:19 AM, Igor Mammedov wrote:
>>>>> On Thu, 13 Feb 2020 12:17:46 -0600
>>>>> Babu Moger <address@hidden> wrote:
>>>>>
>>>>>> Check and Load the apicid handlers from X86CPUDefinition if available.
>>>>>> Update the calling convention for the apicid handlers.
>>>>>
>>>>> Previous and this patch look too complicated for the task at the hand.
>>>>> In particular, cpu_x86_init_apicid_fns() from previous patch adds 1 more
>>>>> reference to Machine into i386/cpu.c (even though it's just a helper 
>>>>> function)
>>>>> and I think un-necessary hooks to X86CPUDefinition (it's not really CPU's
>>>>> businesses to make up APIC-IDs).
>>>>>
>>>>> I'd rather do opposite and get rid of the last explicit dependency to
>>>>> ms->smp.cpus from cpu.c. But well, it's out of scope of this series,
>>>>> so for this series I'd just try to avoid adding more Machine dependencies.
>>>>>
>>>>> All 11/16 does is basically using hooks as a switch "I'm EPYC" to
>>>>> set epyc specific encoding topo routines.
>>>>>
>>>>> It could be accomplished by a simple Boolean flag like
>>>>>  X86CPUDefinition::use_epyc_apic_id_encoding
>>>>>
>>>>> and then cpu_x86_init_apicid_fns() could be replaced with trivial
>>>>> helper like:
>>>>>
>>>>>   x86_use_epyc_apic_id_encoding(char *cpu_type)
>>>>>   {
>>>>>       X86CPUClass *xcc = ... cpu_type ...
>>>>>       return xcc->model->cpudef->use_epyc_apic_id_encoding
>>>>>   }
>>>>>
>>>>> then machine could override default[1] hooks using this helper
>>>>> as the trigger
>>>>>   x86_cpus_init()
>>>>>   {
>>>>>       // no need in dedicated function as it's the only instance it's 
>>>>> going to be called ever
>>>>>       if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
>>>>>             x86ms->apicid_from_cpu_idx = ...epyc...
>>>>>             x86ms->topo_ids_from_apicid = ...epyc...
>>>>>             x86ms->apicid_from_topo_ids = ...epyc...
>>>>>             x86ms->apicid_pkg_offset = ...epyc...
>>>>>       }
>>>>>   }
>>>>>
>>>>> That would be less invasive and won't create non necessary dependencies.
>>>>
>>>> Yes. We can achieve the task here with your approach mentioned above. But,
>>>> we still will have a scaling issue. In future if a "new cpu model" comes
>>>> up its own decoding, then we need to add another bolean flag use_new
>>>> _cpu_apic_id_encoding. And then do that same check again. In that sense,
>>>> the current approach is bit generic. Lets also hear from Eduardo.
>>>
>>> To be honest, I really hope the number of APIC ID initialization
>>> variations won't grow in the future.
>>>
>>> In either case, X86MachineState really doesn't seem to be the
>>> right place to save the function pointers.  Whether we choose a
>>> boolean flag or a collection of function pointers, model-specific
>>> information belong to x86CPUClass and/or X86CPUDefinition, not
>>> MachineState.
>>
>> My bad. I completely missed that part. Yes. You mentioned that earlier.
>> I can move the functions pointers to X86CPUClass and initialize the
>> pointers from X86CPUDefinition. Thanks
> 
> See my reply to Igor before doing that.
> 
> Summary is: if the function implementations are provided by the
> CPU, the pointers belong to X86CPUClass.  If the APIC ID
> calculation logic lives in pc.c, the pointers probably can stay
> in X86MachineState.
> 
Ok. Sure. I will leave those pointers in X86MachineState.