netdev - Re: [PATCH v8 03/27] cxl: add capabilities field to cxl_dev_state and cxl

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <04a40923-d3ca-1b4a-7c05-2eedea707818@amd.com>
Date: Fri, 3 Jan 2025 07:16:51 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: alejandro.lucero-palau@....com, linux-cxl@...r.kernel.org,
 netdev@...r.kernel.org, dan.j.williams@...el.com, martin.habets@...inx.com,
 edward.cree@....com, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v8 03/27] cxl: add capabilities field to cxl_dev_state and
 cxl_port


On 1/2/25 12:49, Jonathan Cameron wrote:
>>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>>> index 19e5d883557a..f656fcd4945f 100644
>>>> --- a/include/cxl/cxl.h
>>>> +++ b/include/cxl/cxl.h
>>>> @@ -12,6 +12,25 @@ enum cxl_resource {
>>>>    	CXL_RES_PMEM,
>>>>    };
>>>>    
>>>> +/* Capabilities as defined for:
>>>> + *
>>>> + *	Component Registers (Table 8-22 CXL 3.1 specification)
>>>> + *	Device Registers (8.2.8.2.1 CXL 3.1 specification)
>>>> + *
>>>> + * and currently being used for kernel CXL support.
>>>> + */
>>>> +
>>>> +enum cxl_dev_cap {
>>>> +	/* capabilities from Component Registers */
>>>> +	CXL_DEV_CAP_RAS,
>>>> +	CXL_DEV_CAP_HDM,
>>>> +	/* capabilities from Device Registers */
>>>> +	CXL_DEV_CAP_DEV_STATUS,
>>>> +	CXL_DEV_CAP_MAILBOX_PRIMARY,
>>>> +	CXL_DEV_CAP_MEMDEV,
>>>> +	CXL_MAX_CAPS = 64
>>> Why set it to 64?  All the bitmaps etc will autosize so
>>> you just need to ensure you use correct set_bit() and test_bit()
>>> that are happy dealing with bitmaps of multiple longs.
>>>   
>> Initially it was set to 32, but DECLARE_BITMAP uses unsigned long, so
>> for initializing/zeroing the locally allocated bitmap in some functions,
>> bitmap_clear had to use sizeof for the size, and I was suggested to
>> define CXL_MAX_CAPS to 64 and use it instead, what seems cleaner.
> It should never have been using sizeof() once it was a bitmap.
> Just clear what is actually used and make sure no code assumes
> any particular length of bitmap.  Then you will never have
> to deal with changing it.


The problem I had was to zeroing a locally allocated bitmap for avoiding 
random bits set by the previous use of that memory.

The macros/functions like bitmap_clear or bitmap_zero require a start 
and a number of bits, and I did not find any other way than using sizeof.

I was not happy with it, although it was fine for current needs of a 
bitmap not bigger than unsigned long size. But I was told to use the 
CXL_MAX_CAPS as currently implemented for using that for the zeroing.


>
> Then CXL_MAX_CAP just becomes last entry in this enum.
>
> The only time this is becomes tricky with bitmaps is if you need
> to set a bits in a constant bitmap as then you can't use the
> set/get functions and have to assume something about the length.
>
> Don't think that applies here.
>
> Jonathan
>
>   
>>
>>>> +};
>>>> +
>>>>    struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>>>>    
>>>>    void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);