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

[Babu Moger]

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 the reply I sent at:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F20200128200438.GJ18770%40habkost.net%2F&amp;data=02%7C01%7Cbabu.moger%40amd.com%7Cda1d1c9f34af4475596108d7b9795fef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637181803279890359&amp;sdata=Z%2B%2BA2%2FMIVQZfGtUe1aBLzttCQnCpZKEwshOhoVAg1%2BU%3D&amp;reserved=0
> 
> ] If you need a CPU model to provide special behavior,
> ] you have two options:
> ] 
> ] * Add a method pointer to X86CPUClass and/or X86CPUDefinition
> ] * Add a QOM property to enable/disable special behavior, and
> ]   include the property in the CPU model definition.
> ] 
> ] The second option might be preferable long term, but might
> ] require more work because the property would become visible in
> ] query-cpu-model-expansion and in the command line.  The first
> ] option may be acceptable to avoid extra user-visible complexity
> ] in the first version.
> ] 
> ] 
> ] 
> ] > +        pcms->apicid_from_cpu_idx = x86_apicid_from_cpu_idx_epyc;
> ] > +        pcms->topo_ids_from_apicid = x86_topo_ids_from_apicid_epyc;
> ] > +        pcms->apicid_from_topo_ids = x86_apicid_from_topo_ids_epyc;
> ] 
> ] Why do you need to override the function pointers in
> ] PCMachineState instead of just looking up the relevant info at
> ] X86CPUClass?
>