Messages in this thread |  | | From | "Moger, Babu" <> | Subject | Re: [RFC PATCH 06/10] arch/x86: Initialize the resource functions that are different | Date | Wed, 3 Oct 2018 15:25:48 +0000 |
| |
On 10/02/2018 05:06 PM, Reinette Chatre wrote: > 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.
Yes. Sure. Will make this change.
> >> + >> +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().
Yes. Makes sense.
> > Reinette >
|  |