netdev - Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev

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: <2482b931-010f-30fe-14cb-2a483b0d8c38@amd.com>
Date: Thu, 15 Aug 2024 16:37:21 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Zhi Wang <zhiw@...dia.com>
Cc: Dave Jiang <dave.jiang@...el.com>, 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,
 richard.hughes@....com, targupta@...dia.com
Subject: Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev_state


On 8/9/24 11:25, Zhi Wang wrote:
> On Tue, 23 Jul 2024 14:43:24 +0100
> Alejandro Lucero Palau <alucerop@....com> wrote:
>
>> On 7/19/24 20:01, Dave Jiang wrote:
>>>>    
>>>> -static int cxl_probe_regs(struct cxl_register_map *map)
>>>> +static int cxl_probe_regs(struct cxl_register_map *map, uint8_t
>>>> caps) {
>>>>    	struct cxl_component_reg_map *comp_map;
>>>>    	struct cxl_device_reg_map *dev_map;
>>>> @@ -437,11 +437,12 @@ static int cxl_probe_regs(struct
>>>> cxl_register_map *map) case CXL_REGLOC_RBI_MEMDEV:
>>>>    		dev_map = &map->device_map;
>>>>    		cxl_probe_device_regs(host, base, dev_map);
>>>> -		if (!dev_map->status.valid ||
>>>> !dev_map->mbox.valid ||
>>>> +		if (!dev_map->status.valid ||
>>>> +		    ((caps & CXL_DRIVER_CAP_MBOX) &&
>>>> !dev_map->mbox.valid) || !dev_map->memdev.valid) {
>>>>    			dev_err(host, "registers not found:
>>>> %s%s%s\n", !dev_map->status.valid ? "status " : "",
>>>> -				!dev_map->mbox.valid ? "mbox " :
>>>> "",
>>>> +				((caps & CXL_DRIVER_CAP_MBOX) &&
>>>> !dev_map->mbox.valid) ? "mbox " : "",
>>> According to the r3.1 8.2.8.2.1, the device status registers and
>>> the primary mailbox registers are both mandatory if regloc id=3
>>> block is found. So if the type2 device does not implement a mailbox
>>> then it shouldn't be calling cxl_pci_setup_regs(pdev,
>>> CXL_REGLOC_RBI_MEMDEV, &map) to begin with from the driver init
>>> right? If the type2 device defines a regblock with id=3 but without
>>> a mailbox, then isn't that a spec violation?
>>>
>>> DJ
>>
>> Right. The code needs to support the possibility of a Type2 having a
>> mailbox, and if it is not supported, the rest of the dvsec regs
>> initialization needs to be performed. This is not what the code does
>> now, so I'll fix this.
>>
>>
>> A wider explanation is, for the RFC I used a test driver based on
>> QEMU emulating a Type2 which had a CXL Device Register Interface
>> defined (03h) but not a CXL Device Capability with id 2 for the
>> primary mailbox register, breaking the spec as you spotted.
>>
>>
> Because SFC driver uses (the 8.2.8.5.1.1 Memory Device Status
> Register) to determine if the memory media is ready or not (in PATCH 6).
> That register should be in a regloc id=3 block.


Right. Note patch 6 calls first cxl_await_media_ready and if it returns 
error, what happens if the register is not found, it sets the media 
ready field since it is required later on.

Damn it! I realize the code is wrong because the manual setting is based 
on no error. The testing has been a pain until recently with a partial 
emulation, so I had to follow undesired development steps. This is 
better now so v3 will fix some minor bugs like this one.

I also realize in our case this first call is useless, so I plan to 
remove it in next version.

Thanks!


> According to the spec paste above, the device that has regloc block
> id=3 needs to have device status and mailbox.
>
> Curious, does the SFC device have to implement the mailbox in this case
> for spec compliance?


I think It should, but no status register either in our case.


> Previously, I always think that "CXL Memory Device" == "CXL Type-3
> device" in the CXL spec.
>
> Now I am little bit confused if a type-2 device that supports cxl.mem
> == "CXL Memory Device" mentioned in the spec.
>
> If the answer == Y, then having regloc id ==3 and mailbox turn
> mandatory for a type-2 device that support cxl.mem for the spec
> compliance.
>
> If the answer == N, then a type-2 device can use approaches other than
> Memory Device Status Register to determine the readiness of the memory?


Right again. Our device is not advertised as a Memory Device but as a 
ethernet one, so we are not implementing those mandatory ones for a 
memory device.

Regarding the readiness of the CXL memory, I have been told this is so 
once some initial negotiation is performed (I do not know the details). 
That is the reason for setting this manually by our driver and the 
accessor added.


> ZW
>
>> Thanks.
>>
>>