Re: [RFC PATCH 02/13] cxl: add type2 device basic support

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

On 6/12/24 05:43, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Differientiating Type3, aka memory expanders, from Type2, aka device
> s/Differientiating/Differentiating/
>
> ...actually to make this imperative tense don't use gerund phrases, so:
>
> s/Differentiating/Differentiate/
>
> This "imperative tense" preference is borrowed from the x86 tip tree
> patch recommendations [1], which reminds me that CXL should create a
> document like that to make the grammar expectations known.


OK.


> [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>
>> accelerators, with a new function for initializing cxl_dev_state.
>>
>> Adding a type2 driver for a CXL emulated device inside CXL kernel
> s/Adding/Add/
>
> I will also note that ChatGPT does a decent job at converting patch
> changelogs to imperative tense.


OK.


>> testing infrastructure as a client for the functionality added.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> It would really help me out if the changelog mentions what you adopted
> and what you modified. With a Link: to the patch where the code
> originated.


The patchset is mentioned/referenced in the cover letter.

I will add individual references as well for any patch needing it.


> I will still review the parts I wrote previously to see if I still agree
> with them, but its taxing to come back to this patch cold and think "did
> I write this routine, or is this new?". Can you repost with the
> changelog commentary fixed up to reflect that?


I'll do.


>> ---
>>   drivers/cxl/core/memdev.c           | 15 ++++++
>>   include/linux/cxlmem.h              |  2 +
>>   tools/testing/cxl/Kbuild            |  1 +
>>   tools/testing/cxl/type2/Kbuild      |  7 +++
>>   tools/testing/cxl/type2/pci_type2.c | 80 +++++++++++++++++++++++++++++
>>   5 files changed, 105 insertions(+)
>>   create mode 100644 tools/testing/cxl/type2/Kbuild
>>   create mode 100644 tools/testing/cxl/type2/pci_type2.c
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 07cd0b8b026f..0336b3f14f4a 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
>> +{
>> +	struct cxl_dev_state *cxlds;
>> +
>> +	cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
>> +	if (!cxlds)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	cxlds->dev = dev;
>> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
>> +
>> +	return cxlds;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
>> +
>>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   					   const struct file_operations *fops)
>>   {
>> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
>> index 0d26a45a4af2..e8d12b543db1 100644
>> --- a/include/linux/cxlmem.h
>> +++ b/include/linux/cxlmem.h
>> @@ -859,4 +859,6 @@ struct cxl_hdm {
>>   struct seq_file;
>>   struct dentry *cxl_debugfs_create_dir(const char *dir);
>>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
>> +
>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>>   #endif /* __CXL_MEM_H__ */
>> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
>> index 030b388800f0..a285719c4db6 100644
>> --- a/tools/testing/cxl/Kbuild
>> +++ b/tools/testing/cxl/Kbuild
>> @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
>>   KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>>   
>>   obj-m += test/
>> +obj-m += type2/
>> diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild
>> new file mode 100644
>> index 000000000000..a96ad4d64924
>> --- /dev/null
>> +++ b/tools/testing/cxl/type2/Kbuild
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-m += pci_type2.o
>> +
>> +cxl_pci_type2-y := cxl_pci_type2.o
>> +
>> +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
>> new file mode 100644
>> index 000000000000..863ce7dc28ef
>> --- /dev/null
>> +++ b/tools/testing/cxl/type2/pci_type2.c
>> @@ -0,0 +1,80 @@
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/cxl.h>
>> +#include <linux/cxlpci.h>
>> +#include <linux/cxlmem.h>
>> +
>> +struct cxl_dev_state *cxlds;
>> +
>> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
>> +
>> +static int type2_pci_probe(struct pci_dev *pci_dev,
>> +			   const struct pci_device_id *entry)
> So to date, tools/testing/cxl/ has been for cxl_test which skips all the
> PCI register emulation and just runs based on mocking core-kernel and
> core-cxl interfaces. I would like to explore how far the cxl_test
> approach can go and leave the PCI integration to when a driver can
> reference real PCI ids.
>
> Otherwise, I feel like too much development effort can be diverted to
> this "proxy" and increase the timeline to seeing the real thing.


Fair enough.

I think I could add the real driver in a new patchset version or maybe a 
completely new one.

 From my previous comment about potentially using something like 
auxbus,that would obviously mean a complete refactoring.


>> +
>> +{
>> +	u16 dvsec;
>> +
>> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
>> +
>> +	if (!dvsec) {
>> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
>> +		return 0;
>> +	} else {
>> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
>> +	}
>> +
>> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
>> +	if (IS_ERR(cxlds))
>> +		return PTR_ERR(cxlds);
>> +
>> +	pci_info(pci_dev, "Initializing cxlds...");
>> +	cxlds->cxl_dvsec = dvsec;
>> +	cxlds->serial = pci_dev->dev.id;
>> +
>> +	/* Should not this be based on DVSEC range size registers */
>> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
> Especially at this stage of the driver there is nothing that require
> QEMU emulation versus cxl_test mocking.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void type2_pci_remove(struct pci_dev *pci_dev)
>> +{
>> +
>> +}
>> +
>> +/* PCI device ID table */
>> +static const struct pci_device_id type2_pci_table[] = {
>> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
> Real vendor-ids should have real device-ids, also that particular
> device-id choice is not appropriate.