| Message ID | 20240516081202.27023-3-alucerop@amd.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: add Type2 device support | expand |
On Thu, 16 May 2024 09:11:51 +0100 <alucerop@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiating Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Adding a type2 driver for a CXL emulated device inside CXL kernel > 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> If you are going to keep Dan's sign-off it needs to be clear why. Either put the author to Dan and swap these two, or use a Co-developed tag. A few drive my trivial comments. > 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) > + > +{ > + u16 dvsec; > + > + dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); Add a line break somewhere in there as no real reason it needs to be all in eonline. > + Trivial: Drop this blank line to keep error check associated with the call. > + if (!dvsec) { > + pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor); > + return 0; Returned, so no point in the else. > + } 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"); > + > + 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)}, > + {0} /* end of list */ {} is more common. > +}; > + > +static struct pci_driver type2_pci_driver = { > + .name = KBUILD_MODNAME, > + .id_table = type2_pci_table, > + .probe = type2_pci_probe, > + .remove = type2_pci_remove, Add remove only when it does something. > +}; > + > +static int __init type2_cxl_init(void) > +{ > + int rc; > + > + rc = pci_register_driver(&type2_pci_driver); > + > + return rc; > +} > + > +static void __exit type2_cxl_exit(void) > +{ > + pci_unregister_driver(&type2_pci_driver); Unless you are adding more in here later in the series there is a macro to cover all this. module_pci_driver() > +} > + > +module_init(type2_cxl_init); > +module_exit(type2_cxl_exit); > + > +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); > +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(CXL); > +MODULE_DEVICE_TABLE(pci, type2_pci_table);
On 5/17/24 15:30, Jonathan Cameron wrote: > On Thu, 16 May 2024 09:11:51 +0100 > <alucerop@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Differientiating Type3, aka memory expanders, from Type2, aka device >> accelerators, with a new function for initializing cxl_dev_state. >> >> Adding a type2 driver for a CXL emulated device inside CXL kernel >> 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> > If you are going to keep Dan's sign-off it needs to be clear why. > Either put the author to Dan and swap these two, or use a > Co-developed tag. Yes, Dan commented on this an he prefers the co-developed flag, so I'll change to that. > A few drive my trivial comments. > >> 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) >> + >> +{ >> + u16 dvsec; >> + >> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); > Add a line break somewhere in there as no real reason it needs to be all in eonline. Sure. >> + > Trivial: Drop this blank line to keep error check associated with the call. OK >> + if (!dvsec) { >> + pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor); >> + return 0; > Returned, so no point in the else. Right. >> + } 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"); >> + >> + 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)}, >> + {0} /* end of list */ > {} is more common. OK >> +}; >> + >> +static struct pci_driver type2_pci_driver = { >> + .name = KBUILD_MODNAME, >> + .id_table = type2_pci_table, >> + .probe = type2_pci_probe, >> + .remove = type2_pci_remove, > Add remove only when it does something. OK >> +}; >> + >> +static int __init type2_cxl_init(void) >> +{ >> + int rc; >> + >> + rc = pci_register_driver(&type2_pci_driver); >> + >> + return rc; >> +} >> + >> +static void __exit type2_cxl_exit(void) >> +{ >> + pci_unregister_driver(&type2_pci_driver); > Unless you are adding more in here later in the series there is a macro > to cover all this. module_pci_driver() > I did not know about it. I'll use it. Thanks >> +} >> + >> +module_init(type2_cxl_init); >> +module_exit(type2_cxl_exit); >> + >> +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); >> +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); >> +MODULE_LICENSE("GPL"); >> +MODULE_IMPORT_NS(CXL); >> +MODULE_DEVICE_TABLE(pci, type2_pci_table);
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. [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. > 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. 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? > --- > 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. > + > +{ > + 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.
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.
Differentiate Type3, aka memory expanders, from Type2, aka device accelerators, with a new function for initializing cxl_dev_state. Add a type2 driver for a CXL emulated device inside CXL kernel testing infrastructure as a client for the functionality added. Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#mef63c11ff2ffc265b75785caeb2c2370953ccf44 Signed-off-by: Alejandro Lucero <alucerop@amd.com> Co-developed-by: Dan Williams <dan.j.williams@intel.com> On 5/16/24 09:11, alucerop@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiating Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Adding a type2 driver for a CXL emulated device inside CXL kernel > 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> > --- > 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) > + > +{ > + 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"); > + > + 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)}, > + {0} /* end of list */ > +}; > + > +static struct pci_driver type2_pci_driver = { > + .name = KBUILD_MODNAME, > + .id_table = type2_pci_table, > + .probe = type2_pci_probe, > + .remove = type2_pci_remove, > +}; > + > +static int __init type2_cxl_init(void) > +{ > + int rc; > + > + rc = pci_register_driver(&type2_pci_driver); > + > + return rc; > +} > + > +static void __exit type2_cxl_exit(void) > +{ > + pci_unregister_driver(&type2_pci_driver); > +} > + > +module_init(type2_cxl_init); > +module_exit(type2_cxl_exit); > + > +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); > +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(CXL); > +MODULE_DEVICE_TABLE(pci, type2_pci_table);
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.
On Tue, Jun 11, 2024 at 09:43:57PM -0700, 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. > > [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html > Hi Dan, Imperative tense is the default expectation thoughout the kernel, so 'we' in CXL don't need to create any special rules around that. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. -- Alison >
Alison Schofield wrote: > On Tue, Jun 11, 2024 at 09:43:57PM -0700, 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. > > > > [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html > > > > Hi Dan, > > Imperative tense is the default expectation thoughout the kernel, so > 'we' in CXL don't need to create any special rules around that. > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Oh, yeah, forgot about that, thanks! However, the motivation for a CXL maintainer-handbook entry would also be to convey subsystem-local details like how often to ping on a series, when the merge window cutoff typically lands, what baseline to use for new patch submissions, do submissions need to worry about reverse-xmas-tree variable declartations, etc... Sidenote on that topic of formatting, I think it would also be useful to declare "clang-format is good enough", i.e. even if clang-format spits out something that could be made to look prettier by hand, the auto-formatted patch is good enough. If someone really cares they can update the clang-format template.
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) + +{ + 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"); + + 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)}, + {0} /* end of list */ +}; + +static struct pci_driver type2_pci_driver = { + .name = KBUILD_MODNAME, + .id_table = type2_pci_table, + .probe = type2_pci_probe, + .remove = type2_pci_remove, +}; + +static int __init type2_cxl_init(void) +{ + int rc; + + rc = pci_register_driver(&type2_pci_driver); + + return rc; +} + +static void __exit type2_cxl_exit(void) +{ + pci_unregister_driver(&type2_pci_driver); +} + +module_init(type2_cxl_init); +module_exit(type2_cxl_exit); + +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(CXL); +MODULE_DEVICE_TABLE(pci, type2_pci_table);