Boris, > -----Original Message----- > From: Borislav Petkov <bp@xxxxxxxxx> > Sent: Tuesday, November 20, 2018 6:16 AM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; corbet@xxxxxxx; > fenghua.yu@xxxxxxxxx; reinette.chatre@xxxxxxxxx; peterz@xxxxxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; akpm@linux- > foundation.org; hpa@xxxxxxxxx; x86@xxxxxxxxxx; > mchehab+samsung@xxxxxxxxxx; arnd@xxxxxxxx; > kstewart@xxxxxxxxxxxxxxxxxxx; pombredanne@xxxxxxxx; > rafael@xxxxxxxxxx; kirill.shutemov@xxxxxxxxxxxxxxx; tony.luck@xxxxxxxxx; > qianyue.zj@xxxxxxxxxxxxxxx; xiaochen.shen@xxxxxxxxx; > pbonzini@xxxxxxxxxx; Singh, Brijesh <brijesh.singh@xxxxxxx>; Hurwitz, > Sherry <sherry.hurwitz@xxxxxxx>; dwmw2@xxxxxxxxxxxxx; Lendacky, > Thomas <Thomas.Lendacky@xxxxxxx>; luto@xxxxxxxxxx; joro@xxxxxxxxxx; > jannh@xxxxxxxxxx; vkuznets@xxxxxxxxxx; rian@xxxxxxxxxxxx; > jpoimboe@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > doc@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v8 11/13] arch/resctrl: Introduce QOS feature for AMD > > On Fri, Nov 16, 2018 at 08:54:43PM +0000, Moger, Babu wrote: > > Enables QOS feature on AMD. > > From Documentation/process/submitting-patches.rst: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." Ok. Sure. > > Following QoS sub-features are supported in AMD if the underlying > > hardware supports it. > > - L3 Cache allocation enforcement > > - L3 Cache occupancy monitoring > > - L3 Code-Data Prioritization support > > - Memory Bandwidth Enforcement(Allocation) > > > > The specification for this feature is available at > > https://developer.amd.com/wp-content/resources/56375.pdf > > I hope that URL is stable. Yes. It is. > > > There are differences in the way some of the features are implemented. > > Separate those functions and add those as vendor specific functions. > > The major difference is in MBA feature. > > - AMD uses CPUID leaf 0x80000020 to initialize the MBA features. > > - AMD uses direct bandwidth value instead of delay based on bandwidth > > values. > > - MSR register base addresses are different for MBA. > > - Also AMD allows non-contiguous L3 cache bit masks. > > > > Adds following functions to take care of the differences. > > rdt_get_mem_config_amd : MBA initialization function > > parse_bw_amd : Bandwidth parsing > > mba_wrmsr_amd: Writes bandwidth value > > cbm_validate_amd : L3 cache bitmask validation > > This paragraph is not needed - what you do is visible in the patch > itself. Ok. Will remove. > > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > > --- > > arch/x86/kernel/cpu/resctrl.c | 68 +++++++++++++++++++++- > > arch/x86/kernel/cpu/resctrl.h | 5 ++ > > arch/x86/kernel/cpu/resctrl_ctrlmondata.c | 70 > +++++++++++++++++++++++ > > 3 files changed, 141 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c > > index 3f26c7c114e7..0d700ab7fcf9 100644 > > --- a/arch/x86/kernel/cpu/resctrl.c > > +++ b/arch/x86/kernel/cpu/resctrl.c > > @@ -61,6 +61,9 @@ mba_wrmsr_intel(struct rdt_domain *d, struct > msr_param *m, > > struct rdt_resource *r); > > static void > > cat_wrmsr(struct rdt_domain *d, struct msr_param *m, struct > rdt_resource *r); > > +static void > > +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, > > + struct rdt_resource *r); > > > > #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].domains) > > > > @@ -280,6 +283,31 @@ static bool rdt_get_mem_config(struct > rdt_resource *r) > > return true; > > } > > > > +static bool rdt_get_mem_config_amd(struct rdt_resource *r) > > +{ > > + union cpuid_0x10_3_eax eax; > > + union cpuid_0x10_x_edx edx; > > + u32 ebx, ecx; > > + > > + cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full); > > + r->num_closid = edx.split.cos_max + 1; > > + r->default_ctrl = MAX_MBA_BW_AMD; > > + > > + /* AMD does not use delay. Set delay_linear to false by default */ > > You don't need to write in the comment *what* you do - that's obvious. > "AMD does not use delay" is more than enough. Ok. > > > + r->membw.delay_linear = false; > > + > > + /* FIX ME - May need to be read from MSR */ > > FIX ME? We don’t need fix me here. We are not going to read from MSR. I will drop it. > > > + r->membw.min_bw = 0; > > + r->membw.bw_gran = 1; > > + /* Max value is 2048, Data width should be 4 in decimal */ > > + r->data_width = 4; > > + > > + r->alloc_capable = true; > > + r->alloc_enabled = true; > > + > > + return true; > > +} > > + > > static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r) > > { > > union cpuid_0x10_1_eax eax; > > @@ -339,6 +367,16 @@ static int get_cache_id(int cpu, int level) > > return -1; > > } > > > > +static void > > +mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct > rdt_resource *r) > > +{ > > + unsigned int i; > > + > > + /* Write the bw values for mba. */ > > That's an obvious comment. Drop it. Ok. > > > + for (i = m->low; i < m->high; i++) > > + wrmsrl(r->msr_base + i, d->ctrl_val[i]); > > +} > > + > > /* > > * Map the memory b/w percentage value to delay values > > * that can be written to QOS_MSRs. > > @@ -792,8 +830,12 @@ static bool __init rdt_cpu_has(int flag) > > > > static __init bool rdt_mba_config(void) > > { > > - if (rdt_cpu_has(X86_FEATURE_MBA)) > > - return > rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]); > > + if (rdt_cpu_has(X86_FEATURE_MBA)) { > > Save yourself an indentation level: Ok. > > if (!rdt_cpu_has(X86_FEATURE_MBA)) > return false; > > if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) > ... Ok. Got it. > > > > + return > rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]); > > + else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > > + return > rdt_get_mem_config_amd(&rdt_resources_all[RDT_RESOURCE_MBA]); > > + } > > > > return false; > > } > > ... > > > diff --git a/arch/x86/kernel/cpu/resctrl.h b/arch/x86/kernel/cpu/resctrl.h > > index 102bcffbefd7..54ba21b7de2c 100644 > > --- a/arch/x86/kernel/cpu/resctrl.h > > +++ b/arch/x86/kernel/cpu/resctrl.h > > @@ -11,6 +11,7 @@ > > #define IA32_L3_CBM_BASE 0xc90 > > #define IA32_L2_CBM_BASE 0xd10 > > #define IA32_MBA_THRTL_BASE 0xd50 > > +#define IA32_MBA_BW_BASE 0xc0000200 > > MSR_... Ok. > > > > > #define IA32_QM_CTR 0x0c8e > > #define IA32_QM_EVTSEL 0x0c8d > > @@ -34,6 +35,7 @@ > > #define MAX_MBA_BW 100u > > #define MBA_IS_LINEAR 0x4 > > #define MBA_MAX_MBPS U32_MAX > > +#define MAX_MBA_BW_AMD 0x800 > > > > #define RMID_VAL_ERROR BIT_ULL(63) > > #define RMID_VAL_UNAVAIL BIT_ULL(62) > > @@ -448,6 +450,8 @@ int parse_cbm(struct rdt_parse_data *data, struct > rdt_resource *r, > > struct rdt_domain *d); > > int parse_bw_intel(struct rdt_parse_data *data, struct rdt_resource *r, > > struct rdt_domain *d); > > +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r, > > + struct rdt_domain *d); > > > > extern struct mutex rdtgroup_mutex; > > > > @@ -579,5 +583,6 @@ void cqm_handle_limbo(struct work_struct *work); > > bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d); > > void __check_limbo(struct rdt_domain *d, bool force_free); > > bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r); > > +bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r); > > > > #endif /* _ASM_X86_RESCTRL_H */ > > diff --git a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c > b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c > > index 71aa1d971430..b6ceb4db9322 100644 > > --- a/arch/x86/kernel/cpu/resctrl_ctrlmondata.c > > +++ b/arch/x86/kernel/cpu/resctrl_ctrlmondata.c > > @@ -28,6 +28,52 @@ > > #include <linux/slab.h> > > #include "resctrl.h" > > > > +/* > > + * Check whether MBA bandwidth percentage value is correct. The value > is > > + * checked against the minimum and max bandwidth values specified by > the > > Either write "min" and "max" or write them both out fully. Ok > > > + * hardware. The allocated bandwidth percentage is rounded to the next > > + * control step available on the hardware. > > + */ > > +static bool bw_validate_amd(char *buf, unsigned long *data, > > + struct rdt_resource *r) > > +{ > > + unsigned long bw; > > + int ret; > > + > > + ret = kstrtoul(buf, 10, &bw); > > + if (ret) { > > + rdt_last_cmd_printf("Non-decimal digit in MB value %s\n", > buf); > > + return false; > > + } > > + > > + if (bw < r->membw.min_bw || bw > r->default_ctrl) { > > + rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", > bw, > > + r->membw.min_bw, r->default_ctrl); > > + return false; > > + } > > + > > + *data = roundup(bw, (unsigned long)r->membw.bw_gran); > > + return true; > > +} > > + > > +int parse_bw_amd(struct rdt_parse_data *data, struct rdt_resource *r, > > + struct rdt_domain *d) > > +{ > > + unsigned long bw_val; > > + > > + if (d->have_new_ctrl) { > > + rdt_last_cmd_printf("duplicate domain %d\n", d->id); > > Start with a capital letter: "Duplicate domain..." ok > > And looking at the rest, some of them start with a capital letter and > some of them not. Please unify that in a separate patch. Yes, I see it. I will fix all the rdt_last_cmd_printf texts. > > > + return -EINVAL; > > + } > > + > > + if (!bw_validate_amd(data->buf, &bw_val, r)) > > + return -EINVAL; > > <---- newline here. Ok. > > > + d->new_ctrl = bw_val; > > + d->have_new_ctrl = true; > > + > > + return 0; > > +} > > + > > /* > > * Check whether MBA bandwidth percentage value is correct. The value is > > * checked against the minimum and max bandwidth values specified by > the > > -- > Regards/Gruss, > Boris. > > Good mailing practices for 400: avoid top-posting and trim the reply.
