Messages in this thread - First message in thread
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- "Moger, Babu"
- Thomas Gleixner
- Fenghua Yu
|  | | Subject | Re: [RFC PATCH 06/10] arch/x86: Initialize the resource functions that are different | From | Reinette Chatre <> | Date | Tue, 2 Oct 2018 15:06:15 -0700 |
| |
Hi Babu,
On 9/24/2018 12:19 PM, Moger, Babu wrote: > Initialize the resource functions that are different between the > vendors. Some features are initialized differently between the vendors. > > For example, MBA feature varies significantly between Intel and AMD. > Separate the initialization of these resource functions. That way we > can easily add AMD's functions later. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > arch/x86/kernel/cpu/rdt.c | 28 +++++++++++++++++++++++++--- > arch/x86/kernel/cpu/rdt.h | 4 ++++ > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/cpu/rdt.c b/arch/x86/kernel/cpu/rdt.c > index 736715b81fd8..6dec45bf81d6 100644 > --- a/arch/x86/kernel/cpu/rdt.c > +++ b/arch/x86/kernel/cpu/rdt.c > @@ -174,10 +174,7 @@ struct rdt_resource rdt_resources_all[] = { > .rid = RDT_RESOURCE_MBA, > .name = "MB", > .domains = domain_init(RDT_RESOURCE_MBA), > - .msr_base = IA32_MBA_THRTL_BASE, > - .msr_update = mba_wrmsr, > .cache_level = 3, > - .parse_ctrlval = parse_bw, > .format_str = "%d=%*u", > .fflags = RFTYPE_RES_MB, > }, > @@ -865,6 +862,25 @@ static __init void rdt_check_mba(void) > rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]); > } > > +static __init void rdt_init_res_defs_intel(void) > +{ > + struct rdt_resource *r; > + > + for_each_rdt_resource(r) { > + if (r->rid == RDT_RESOURCE_MBA) { > + r->msr_base = IA32_MBA_THRTL_BASE; > + r->msr_update = mba_wrmsr; > + r->parse_ctrlval = parse_bw; > + } > + } > +}
Patch 10 introduces parse_bw_amd and mba_wrmsr_amd as you prepare us for in the commit message. I think it would reduce confusion if these functions, mba_wrmsr and parse_bw, also follow this pattern to contain the vendor name. So, mba_wrmsr -> mba_wrmsr_intel, parse_bw -> parse_bw_intel.
> + > +static __init void rdt_init_res_defs(void) > +{ > + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) > + rdt_init_res_defs_intel(); > +} > + > static enum cpuhp_state rdt_online; > > static int __init rdt_late_init(void) > @@ -875,6 +891,12 @@ static int __init rdt_late_init(void) > /* Run quirks first */ > rdt_quirks(); > > + /* > + * Initialize functions(or definitions) that are different > + * between vendors here. > + */ > + rdt_init_res_defs(); > +
While it does seem as though currently rdt_quirks() is not using any of the settings made in rdt_init_res_defs() it (rdt_quirks()) does use the partially initialized resources structure and this may in the future include using parameters that have not been initialized yet.
I thus think it would be safer to do this initialization before rdt_quirks().
Reinette
|  |