Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <reinette.cha...@intel.com>
> Sent: Tuesday, November 24, 2020 11:23 AM
> To: Moger, Babu <babu.mo...@amd.com>; b...@alien8.de
> Cc: fenghua...@intel.com; x...@kernel.org; linux-kernel@vger.kernel.org;
> mi...@redhat.com; h...@zytor.com; t...@linutronix.de
> Subject: Re: [PATCH v2] x86/resctrl: Fix AMD L3 QOS CDP enable/disable
>
> Hi Babu,
>
> On 11/20/2020 9:25 AM, Babu Moger wrote:
> > When the AMD QoS feature CDP(code and data prioritization) is enabled
> > or disabled, the CDP bit in MSR 0000_0C81 is written on one of the
> > CPUs in L3 domain(core complex). That is not correct. The CDP bit
> > needs to be updated all the logical CPUs in the domain.
> >
> > This was not spelled out clearly in the spec earlier. The
> > specification has been updated. The updated specification, "AMD64
> > Technology Platform Quality of Service Extensions Publication # 56375
> > Revision: 1.02 Issue
> > Date: October 2020" is available now. Refer the section: Code and Data
> > Prioritization.
> >
> > Fix the issue by adding a new flag arch_has_per_cpu_cfg in rdt_cache
> > data structure.
> >
> > The documentation can be obtained at the links below:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeve
> > loper.amd.com%2Fwp-
> content%2Fresources%2F56375.pdf&data=04%7C01%7C
> >
> babu.moger%40amd.com%7C5dd411c029da43716aad08d8909daa39%7C3dd89
> 61fe488
> >
> 4e608e11a82d994e183d%7C0%7C1%7C637418354605231589%7CUnknown%7
> CTWFpbGZs
> >
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D
> >
> %7C1000&sdata=uhSBwxk%2BvcdCjgkq%2B0ew%2Fx1abL32KJEoe7Dil1CF
> qX4%3D
> > &reserved=0
> > Link:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> >
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&data=04%7C01%7Cbab
> u.m
> >
> oger%40amd.com%7C5dd411c029da43716aad08d8909daa39%7C3dd8961fe48
> 84e608e
> >
> 11a82d994e183d%7C0%7C1%7C637418354605231589%7CUnknown%7CTWFpb
> GZsb3d8ey
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 100
> >
> 0&sdata=5LWDsBKkTmKfKrDfALJQlo6PySMtBVX2iVna9KaiWwE%3D&r
> eserve
> > d=0
> >
> > Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
> > Signed-off-by: Babu Moger <babu.mo...@amd.com>
> > ---
> > v2: Taken care of Reinette's comments. Changed the field name to
> > arch_has_per_cpu_cfg to be bit more meaningful about the CPU scope.
> > Also fixed some wordings.
> >
> > v1:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> >
> .kernel.org%2Flkml%2F160469365104.21002.2901190946502347327.stgit%40b
> m
> > oger-
> ubuntu%2F&data=04%7C01%7Cbabu.moger%40amd.com%7C5dd411c029
> da4
> >
> 3716aad08d8909daa39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%
> 7C63741
> >
> 8354605241539%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2lu
> >
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=VNVZzPr9IvV4Hp
> tYI9
> > VqCN8uXLtlKBVtG%2FUaGEavRLM%3D&reserved=0
> >
> > arch/x86/kernel/cpu/resctrl/core.c | 4 ++++
> > arch/x86/kernel/cpu/resctrl/internal.h | 3 +++
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 9 +++++++--
> > 3 files changed, 14 insertions(+), 2 deletions(-)
> >
>
> ...
>
> > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> > b/arch/x86/kernel/cpu/resctrl/internal.h
> > index 80fa997fae60..bcd9b517c765 100644
> > --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > @@ -360,6 +360,8 @@ struct msr_param {
> > * executing entities
> > * @arch_has_sparse_bitmaps: True if a bitmap like f00f is valid.
> > * @arch_has_empty_bitmaps: True if the '0' bitmap is valid.
> > + * @arch_has_per_cpu_cfg: True if QOS_CFG register for this cache
> > + * level has CPU scope.
>
> Please fixup the spacing to not have spaces before tabs. This will make
> checkpatch happy and fit with in with the rest of the comments for this
> struct.
Sure. Will fix it.
>
> > */
> > struct rdt_cache {
> > unsigned int cbm_len;
> > @@ -369,6 +371,7 @@ struct rdt_cache {
> > unsigned int shareable_bits;
> > bool arch_has_sparse_bitmaps;
> > bool arch_has_empty_bitmaps;
> > + bool arch_has_per_cpu_cfg;
> > };
> >
> > /**
>
> ...
>
> This patch looks good to me.
>
> With the one style comment addressed you can add:
> Reviewed-by: Reinette Chatre <reinette.cha...@intel.com>
>
Thanks
-Babu
