Re: [PATCH v10 01/26] cxl: make memdev creation type agnostic

[Alejandro Lucero Palau <alucerop@amd.com>]

On 2/6/25 19:37, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> In preparation for Type2 support, change memdev creation making
>> type based on argument.
>>
>> Integrate initialization of dvsec and serial fields in the related
>> cxl_dev_state within same function creating the memdev.
>>
>> Move the code from mbox file to memdev file.
>>
>> Add new header files with type2 required definitions for memdev
>> state creation.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/mbox.c   | 20 --------------------
>>   drivers/cxl/core/memdev.c | 23 +++++++++++++++++++++++
>>   drivers/cxl/cxlmem.h      | 18 +++---------------
>>   drivers/cxl/cxlpci.h      | 17 +----------------
>>   drivers/cxl/pci.c         | 16 +++++++++-------
>>   include/cxl/cxl.h         | 26 ++++++++++++++++++++++++++
>>   include/cxl/pci.h         | 23 +++++++++++++++++++++++
>>   7 files changed, 85 insertions(+), 58 deletions(-)
>>   create mode 100644 include/cxl/cxl.h
>>   create mode 100644 include/cxl/pci.h
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 4d22bb731177..96155b8af535 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1435,26 +1435,6 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, "CXL");
>>   
>> -struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>> -{
>> -	struct cxl_memdev_state *mds;
>> -
>> -	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
>> -	if (!mds) {
>> -		dev_err(dev, "No memory available\n");
>> -		return ERR_PTR(-ENOMEM);
>> -	}
>> -
>> -	mutex_init(&mds->event.log_lock);
>> -	mds->cxlds.dev = dev;
>> -	mds->cxlds.reg_map.host = dev;
>> -	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>> -	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
>> -
>> -	return mds;
>> -}
>> -EXPORT_SYMBOL_NS_GPL(cxl_memdev_state_create, "CXL");
>> -
>>   void __init cxl_mbox_init(void)
>>   {
>>   	struct dentry *mbox_debugfs;
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 63c6c681125d..456d505f1bc8 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -632,6 +632,29 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial,
>> +						 u16 dvsec, enum cxl_devtype type)
>> +{
>> +	struct cxl_memdev_state *mds;
>> +
>> +	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
>> +	if (!mds) {
>> +		dev_err(dev, "No memory available\n");
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	mutex_init(&mds->event.log_lock);
>> +	mds->cxlds.dev = dev;
>> +	mds->cxlds.reg_map.host = dev;
>> +	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>> +	mds->cxlds.cxl_dvsec = dvsec;
>> +	mds->cxlds.serial = serial;
>> +	mds->cxlds.type = type;
>> +
>> +	return mds;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_memdev_state_create, "CXL");
> 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, ... .


>   Something roughly like
> the below. Note, this borrows from the fwctl_alloc_device() example
> which captures the spirit of registering a core 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.


> struct cxl_memdev_state *cxl_memdev_state_create(parent, serial, dvsec)
> {
>          struct cxl_memdev_state *mds = cxl_dev_state_create(
>                  parent, serial, dvsec, CXL_DEVTYPE_CLASSMEM,
>                  struct cxl_memdev_state, cxlds);
>
>          if (IS_ERR(mds))
>                  return mds;
>          
>          mutex_init(&mds->event.log_lock);
>          mds->cxlds.dev = dev;
>          mds->cxlds.reg_map.host = dev;
>          mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>          mds->cxlds.cxl_dvsec = dvsec;
>          mds->cxlds.serial = serial;
>          mds->cxlds.type = type;
>
>          return mds;
> }
>
> If an accelerator wants to share infrastructure that is currently housed
> in 'struct cxl_memdev_state', then that functionality should first move
> to 'struct cxl_dev_state'.


If you see the full patchset, you will realize the accel driver does not 
use the cxl objects except for doing further initialization with them. 
In other words, we keep the idea of avoiding the accel driver 
manipulating those structs freely, and having an API for accel drivers. 
Using cxl_memdev_struct now implies to change some core functions for 
using that struct as the argument what should not be a problem.


We will need to think about this when type2 cache support comes, which 
will mean type1 support as well. But it is hard to think about what will 
be required then, because it is not clear yet how we should implement 
that support. So for the impending need of having Type2 support for 
CXL.mem, I really think this is all what we need by now.