Message-ID: <390c6b69-bbf4-47cb-95d9-215a92ee3cbf@amd.com>
Date: Thu, 20 Feb 2025 18:17:06 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, linux-cxl@...r.kernel.org,
 netdev@...r.kernel.org, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v10 01/26] cxl: make memdev creation type agnostic


On 2/19/25 02:29, Dan Williams wrote:
> Alejandro Lucero Palau wrote:
>> On 2/6/25 19:37, Dan Williams wrote:
>>>
>>> I was envisioning that accelerators only consider 'struct cxl_dev_state'
>>> and that 'struct cxl_memdev_state' is exclusively for
>>> CXL_DEVTYPE_CLASSMEM memory expander use case.
>>
>> That was the original idea and what I have followed since the RFC, but
>> since the patchset has gone through some assumptions which turned wrong,
>> I seized the "revolution" for changing this as well.
>>
>>
>> A type2 is a memdev, and what makes it different is the exposure, so I
>> can not see why an accel driver, at least a Type2, should not use a
>> cxl_memdev_state struct. This simplifies the type2 support and after
>> all, a Type2 could require the exact same things like a type3, like
>> mbox, perf, poison, ... .
> I disagree, I think it avoids the discipline of maintaining Accelerator
> CXL.mem infrastructure alongside the sometimes super-set sometimes
> disjoint-set of generic CXL memory expander support.
>
> Specifically, the reason I think the implementation is worse off reusing
> cxl_memdev_state for accelerators is because accelerators are not
> subject to the same requirements as "memory device" expanders that emit
> the class code from CXL 3.1 8.1.12.1 "PCI Header - Class Code Register
> (Offset 09h)".
>
> The whole point of the CXL_DEVTYPE_CLASSMEM vs CXL_DEVTYPE_DEVMEM
> distinction was for cases where accelerators are not mandated to support
> the same functionality as a generic expander.
>
> It is not until patch12 that this set notices that to_cxl_memdev_state()
> has been returning NULL for accelerator created 'cxl_memdev_state'
> instances. However, patch12 is confused because to_cxl_memdev_state()
> has no reason to exist if it can be assumed that 'struct
> cxl_memdev_state' always wraps 'struct cxl_dev_state'.
>
> The fact that it requires thought and care to decide how accelerators
> can share code paths with the generic memory class device case is a
> *feature*.
>
> So either 'enum cxl_devtype' needs to be removed altogether (would need
> a strong argument that is currently absent from this set), or we need to
> carefully think about the optional functionality that an accelerator may
> want to reuse from expanders. As it stands, most of cxl_memdev_state
> makes little sense for an accelerator:


OK. I'll go back to using cxl_dev_state. More about this below.

>>>    Something roughly like
>>> the below. Note, this borrows from the fwctl_alloc_device() example
>>> which captures the spirit of registering a c4ore object wrapped by an end
>>> driver provided structure).
>>>
>>> #define cxl_dev_state_create(parent, serial, dvsec, type, drv_struct, member)  \
>>>           ({                                                                     \
>>>                   static_assert(__same_type(struct cxl_dev_state,                \
>>>                                             ((drv_struct *)NULL)->member));      \
>>>                   static_assert(offsetof(drv_struct, member) == 0);              \
>>>                   (drv_struct *)_cxl_dev_state_create(parent, serial, dvsec,     \
>>>                                                       type, sizeof(drv_struct)); \
>>>           })
>>
>> If you prefer the accel driver keeping a struct embedding the core cxl
>> object, that is fine. I can not see a reason for not doing it, although
>> I can not see a reason either for imposing this.
> The cxl_pci driver would use it to align on a single way to wrap its class device
> driver context around 'struct cxl_dev_state'. So it is more about
> setting common expectations across cxl_pci and accelerator drivers for
> how they wrap 'struct cxl_dev_state'.
>
> [..]


I have tried this but it is not so simple as with the fwctl case where 
the embedded struct is simpler.

As a result, a good bunch of until now internal CXL structs need to go 
to include/cxl/cxl.h, otherwise the size for the whole accel driver 
struct embedding cxl_dev_state can not be known.

That makes those struct visible to accel drivers which we tried to 
avoid, but I guess this is fine and we should ensure accel drivers do 
not use those structs freely, whatever the reason.


I will post a RFC just with that change, so only first two patches of 
the whole type2 support patchset. Let's be sure we agree on how the 
accel driver does this first initialization before I send the whole 
patchset again.

Powered by blists - more mailing lists