[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineSta
From: |
Eduardo Habkost |
Subject: |
Re: [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState |
Date: |
Tue, 25 Feb 2020 10:32:23 -0500 |
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.
--
Eduardo
- Re: [PATCH v4 09/16] target/i386: Cleanup and use the EPYC mode topology functions, (continued)
- [PATCH v4 10/16] hw/i386: Introduce apicid functions inside X86MachineState, Babu Moger, 2020/02/13
- [PATCH v4 11/16] target/i386: Load apicid model specific handlers from X86CPUDefinition, Babu Moger, 2020/02/13
- [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState, Babu Moger, 2020/02/13
- Re: [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState, Igor Mammedov, 2020/02/25
- Re: [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState, Eduardo Habkost, 2020/02/25
[PATCH v4 13/16] target/i386: Add EPYC model specific handlers, Babu Moger, 2020/02/13
[PATCH v4 14/16] hw/i386: Move arch_id decode inside x86_cpus_init, Babu Moger, 2020/02/13
[PATCH v4 15/16] i386: Fix pkg_id offset for EPYC cpu models, Babu Moger, 2020/02/13
[PATCH v4 16/16] tests: Update the Unit tests, Babu Moger, 2020/02/13