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

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

On 6/12/24 07:04, Alejandro Lucero Palau wrote:
>
> 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.
>>

I did use a slightly modified qemu hw/mem/cxl_type2.c for adding 
CXL.cache bits, and planned to submit it for anyone interested in Type2 
development ...


>>> +
>>> +    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.
>
And this is the PCI IDs used by such emulation. It is emulating a 
non-existing CXL Type2 device what I guess is more problematic than 
emulating a memory expander.

My interested was to test configuration and not any kind of emulated 
acceleration related to CXL.mem writes or reads, what could be a project 
by itself but not sure if useful at all.

If this is valuable, I will submit it. I think extending qemu CXL 
support for helping development could be really useful for all those 
things hard to test like interleaving and sooner or later, complex 
fabrics with a good number of CXL switches, plus x-FAM support.