Message-ID: <e597747e-17be-0f1a-8dbc-0682ec3522b2@amd.com>
Date: Mon, 19 Aug 2024 14:14:45 +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, Vikram Sethi
 <vsethi@...dia.com>, zhiwang@...nel.org
Subject: Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev_state


On 8/18/24 07:55, Zhi Wang wrote:
> On Thu, 15 Aug 2024 16:37:21 +0100
> Alejandro Lucero Palau <alucerop@....com> wrote:
>
>> 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!
>>
> Hi Alejandro:
>
> No worries. Let's push forward. :)
>
> For a type-2, I think cxl_await_media_ready() still gives value on
> provide a type-2 vendor driver a generic core call to make sure the HDM
> region is ready to use. Because judging CXL_RANGE active & valid in
> CXL_RANGE_{1,2}_SIZE_LO can be useful to type-2.
>
> I think the problem of cxl_await_media_ready() is: it assumes the
> Memory Device Status Register is always present, which is true for
> type-3 but not always true for type-2. I think we need:
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index a663e7566c48..0ba1cedfc0ba 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -203,6 +203,9 @@ int cxl_await_media_ready(struct cxl_dev_state
> *cxlds)
>                          return rc;
>          }
>
> +       if (!cxlds->regs.memdev)
> +               return 0;
> +
>          md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
>          if (!CXLMDEV_READY(md_status))
>                  return -EIO;
>
> Then for the type-2 device, if it doesn't implement regloc=3, it can
> still call cxl_await_media_ready() to make sure the media is ready. For
> type-2 and type-3 which implements regloc=3, the check can continue.


In this case I think the driver should know if calling this function 
makes sense, apart from the code checking if the proper register does exist.


>
> I think SFC can use this as well, because according to the spec 8.1.3.8
> DVSEC CXL Range Registers:
>
> "The DVSEC CXL Range 1 register set must be implemented if
> Mem_Capable=1 in the DVSEC CXL Capability register. The DVSEC CXL Range
> 2 register set must be implemented if (Mem_Capable=1 and HDM_Count=10b
> in the DVSEC CXL Capability register)."


I have discussed this internally, and what you point to implies it is, 
as we understand it, only mandatory for memory devices what we are not. 
I guess this is an ambiguity in the specs but the fact is the current 
hardware design which will be part of the silicon coming has not such 
register implemented.

> So SFC should have this. With the change above maybe you don't need
> set_media_ready stuff in the later patch. Just simply call
> cxl_await_media_ready(), everything should be fine then.


The media_ready field inside cxl_dev_state needs to be set to true for 
avoiding later checks to preclude further initialization.

I could avoid this accessor as we have decided to not make cxl_dev_state 
opaque but in prevision of core cxl struct refactoring in the future, I 
think it is worth to keep the accessor.

Thanks


>
> Thanks,
> Zhi.
>
>>> 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.
>>>>
>>>>

Powered by blists - more mailing lists