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

[Eduardo Habkost <ehabkost@redhat.com>]

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 <babu.moger@amd.com> 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.

-- 
Eduardo