| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20181120121606.GF2527@zn.tnic>
Date: Tue, 20 Nov 2018 13:16:06 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Moger, Babu" <Babu.Moger@....com>
Cc: "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"corbet@....net" <corbet@....net>,
"fenghua.yu@...el.com" <fenghua.yu@...el.com>,
"reinette.chatre@...el.com" <reinette.chatre@...el.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
"mchehab+samsung@...nel.org" <mchehab+samsung@...nel.org>,
"arnd@...db.de" <arnd@...db.de>,
"kstewart@...uxfoundation.org" <kstewart@...uxfoundation.org>,
"pombredanne@...b.com" <pombredanne@...b.com>,
"rafael@...nel.org" <rafael@...nel.org>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"qianyue.zj@...baba-inc.com" <qianyue.zj@...baba-inc.com>,
"xiaochen.shen@...el.com" <xiaochen.shen@...el.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"Singh, Brijesh" <brijesh.singh@....com>,
"Hurwitz, Sherry" <sherry.hurwitz@....com>,
"dwmw2@...radead.org" <dwmw2@...radead.org>,
"Lendacky, Thomas" <Thomas.Lendacky@....com>,
"luto@...nel.org" <luto@...nel.org>,
"joro@...tes.org" <joro@...tes.org>,
"jannh@...gle.com" <jannh@...gle.com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"rian@...m.mit.edu" <rian@...m.mit.edu>,
"jpoimboe@...hat.com" <jpoimboe@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
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."
> 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.
> 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.
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> 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.
> + r->membw.delay_linear = false;
> +
> + /* FIX ME - May need to be read from MSR */
FIX ME?
> + 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.
> + 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:
if (!rdt_cpu_has(X86_FEATURE_MBA))
return false;
if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
...
> + 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_...
>
> #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.
> + * 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..."
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.
> + return -EINVAL;
> + }
> +
> + if (!bw_validate_amd(data->buf, &bw_val, r))
> + return -EINVAL;
<---- newline here.
> + 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.
Powered by blists - more mailing lists