Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev_state

[Zhi Wang <zhiw@nvidia.com>]

On Thu, 15 Aug 2024 16:37:21 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> 
> On 8/9/24 11:25, Zhi Wang wrote:
> > On Tue, 23 Jul 2024 14:43:24 +0100
> > Alejandro Lucero Palau <alucerop@amd.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.

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)."

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.

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.
> >>
> >>
>