RE: [PATCH v2] x86/resctrl: Fix AMD L3 QOS CDP enable/disable

[Babu Moger]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Sent: Tuesday, November 24, 2020 11:23 AM
> To: Moger, Babu <Babu.Moger@xxxxxxx>; bp@xxxxxxxxx
> Cc: fenghua.yu@xxxxxxxxx; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; hpa@xxxxxxxxx; tglx@xxxxxxxxxxxxx
> 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&amp;data=04%7C01%7C
> >
> babu.moger%40amd.com%7C5dd411c029da43716aad08d8909daa39%7C3dd89
> 61fe488
> >
> 4e608e11a82d994e183d%7C0%7C1%7C637418354605231589%7CUnknown%7
> CTWFpbGZs
> >
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D
> >
> %7C1000&amp;sdata=uhSBwxk%2BvcdCjgkq%2B0ew%2Fx1abL32KJEoe7Dil1CF
> qX4%3D
> > &amp;reserved=0
> > Link:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> >
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D206537&amp;data=04%7C01%7Cbab
> u.m
> >
> oger%40amd.com%7C5dd411c029da43716aad08d8909daa39%7C3dd8961fe48
> 84e608e
> >
> 11a82d994e183d%7C0%7C1%7C637418354605231589%7CUnknown%7CTWFpb
> GZsb3d8ey
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 100
> >
> 0&amp;sdata=5LWDsBKkTmKfKrDfALJQlo6PySMtBVX2iVna9KaiWwE%3D&amp;r
> eserve
> > d=0
> >
> > Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
> > Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> > ---
> > 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&amp;data=04%7C01%7Cbabu.moger%40amd.com%7C5dd411c029
> da4
> >
> 3716aad08d8909daa39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%
> 7C63741
> >
> 8354605241539%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2lu
> >
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=VNVZzPr9IvV4Hp
> tYI9
> > VqCN8uXLtlKBVtG%2FUaGEavRLM%3D&amp;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.chatre@xxxxxxxxx>
> 
Thanks
-Babu