linux-kernel - Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 25 Aug 2022 05:48:22 +0000
From:   Adam Manzanares <a.manzanares@...sung.com>
To:     Davidlohr Bueso <dave@...olabs.net>
CC:     "alison.schofield@...el.com" <alison.schofield@...el.com>,
        "vishal.l.verma@...el.com" <vishal.l.verma@...el.com>,
        "ira.weiny@...el.com" <ira.weiny@...el.com>,
        "widawsk@...nel.org" <widawsk@...nel.org>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

On Mon, Aug 22, 2022 at 10:17:03AM -0700, Davidlohr Bueso wrote:
> On Mon, 22 Aug 2022, Adam Manzanares wrote:
> 
> > When reviewing the CFMWS parsing code that deals with the HDM decoders,
> > I noticed a couple of magic numbers. This commit replaces these magic numbers
> > with constants defined by the CXL 2.0 specification.
> 
> Please use 3.0 spec :)
> 
> Actually the whole drivers/cxl/* could use updating the comments for 3.0.
> 
> > Signed-off-by: Adam Manzanares <a.manzanares@...sung.com>
> > ---
> > drivers/cxl/cxl.h | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index f680450f0b16..ba3a66b5b9cd 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -61,6 +61,10 @@
> > #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
> > #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
> > 
> > +/* HDM decoder control register constants CXL 2.0 8.2.5.12.7 */
> 
> This now becomes 8.2.4.19.7.
> 
> > +#define CXL_DECODER_MIN_GRANULARITY 256
> > +#define CXL_DECODER_MAX_GRANULARITY_ORDER 6
> 
> Considering there is a single user, I don't think we need to add
> the CXL_DECODER_MAX_GRANULARITY_ORDER. And if a new one is added
> it would be better to keep symmetry (bytes) and just do
> CXL_DECODER_MAX_GRANULARITY, but that's probably the reason why
> it doesn't exist in the first place.

I wasn't so worried about the single user. I was hoping to give some additional
context for the constants used in the granularity calculations. 

Given Dans suggestion for the name change are you more comfortable with the 
proposed change. If not, feel free to suggest an alternative to eliminate the
magic number.

> 
> > +
> > static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> > {
> > 	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
> > @@ -71,9 +75,9 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> > /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> > static inline int cxl_to_granularity(u16 ig, unsigned int *val)
> > {
> > -	if (ig > 6)
> > +	if (ig > CXL_DECODER_MAX_GRANULARITY_ORDER)
> > 		return -EINVAL;
> > -	*val = 256 << ig;
> > +	*val = CXL_DECODER_MIN_GRANULARITY << ig;
> > 	return 0;
> > }
> > 
> > @@ -96,7 +100,7 @@ static inline int cxl_to_ways(u8 eniw, unsigned int *val)
> > 
> > static inline int granularity_to_cxl(int g, u16 *ig)
> > {
> > -	if (g > SZ_16K || g < 256 || !is_power_of_2(g))
> > +	if (g > SZ_16K || g < CXL_DECODER_MIN_GRANULARITY || !is_power_of_2(g))
> 
> Looks good.
> 
> Thanks,
> Davidlohr