| Message ID | 20201111054356.793390-4-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL 2.0 Support | expand |
Hi, On 11/10/20 9:43 PM, Ben Widawsky wrote: > --- > drivers/cxl/Kconfig | 20 +++++++++++ > drivers/cxl/Makefile | 2 ++ > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/pci.h | 15 ++++++++ > 4 files changed, 119 insertions(+) > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > index dd724bd364df..15548f5c77ff 100644 > --- a/drivers/cxl/Kconfig > +++ b/drivers/cxl/Kconfig > @@ -27,4 +27,24 @@ config CXL_ACPI > resources described by the CEDT (CXL Early Discovery Table) > > Say 'y' to enable CXL (Compute Express Link) drivers. > + > +config CXL_MEM > + tristate "CXL.mem Device Support" > + depends on PCI && CXL_BUS_PROVIDER != n > + default m if CXL_BUS_PROVIDER The "if CXL_BUS_PROVIDER" should be redundant due to the "depends on" clause. However, having any default or 'm' or 'y' needs to be justified. > + help > + The CXL.mem protocol allows a device to act as a provider of > + "System RAM" and/or "Persistent Memory" that is fully coherent > + as if the memory was attached to the typical CPU memory > + controller. > + > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach The builtin driver will not be named cxl-mem.ko. > + to CXL.mem devices for configuration, provisioning, and health > + monitoring, the so called "type-3 mailbox". Note, this driver > + is required for dynamic provisioning of CXL.mem attached > + memory, a pre-requisite for persistent memory support, but prerequisite > + devices that provide volatile memory may be fully described by > + existing platform firmware memory enumeration. > + > + If unsure say 'n'. > endif
On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: > +config CXL_MEM > + tristate "CXL.mem Device Support" > + depends on PCI && CXL_BUS_PROVIDER != n depend on PCI && CXL_BUS_PROVIDER > + default m if CXL_BUS_PROVIDER Please don't set weird defaults for new code. Especially not default to module crap like this. > +// Copyright(c) 2020 Intel Corporation. All rights reserved. Please don't use '//' for anything but the SPDX header. > + > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > + return pos; > + > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); Overly long lines again. > +static void cxl_mem_remove(struct pci_dev *pdev) > +{ > +} No need for the empty remove callback. > +MODULE_AUTHOR("Intel Corporation"); A module author is not a company.
On Tue, Nov 10, 2020 at 11:12 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: > > +config CXL_MEM > > + tristate "CXL.mem Device Support" > > + depends on PCI && CXL_BUS_PROVIDER != n > > depend on PCI && CXL_BUS_PROVIDER > > > + default m if CXL_BUS_PROVIDER > > Please don't set weird defaults for new code. Especially not default > to module crap like this. This goes back to what people like Dave C. asked for LIBNVDIMM / DAX, a way to blanket turn on a subsystem without needing to go hunt down individual configs. All of CXL is "default n", but if someone turns on a piece of it they get all of it by default. The user can then opt-out on pieces after that first opt-in. If there's a better way to turn on suggested configs I'm open to switch to that style. As for the "default m" I was worried that it would be "default y" without the specificity, but I did not test that... will check. There have been times when I wished that distros defaulted bleeding edge new enabling to 'm' and putting that default in the Kconfig maybe saves me from needing to file individual config changes to distros after the fact. > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > Please don't use '//' for anything but the SPDX header. Ok, I find // following by /* */ a bit ugly, but I don't care enough to fight. > > > + > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > + return pos; > > + > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > Overly long lines again. I thought 100 is the new 80 these days? > > +static void cxl_mem_remove(struct pci_dev *pdev) > > +{ > > +} > > No need for the empty remove callback. True, will fix. > > > +MODULE_AUTHOR("Intel Corporation"); > > A module author is not a company. At least I don't have a copyright assignment clause, I don't agree with the vanity of listing multiple people here especially when MAINTAINERS has the contact info, and I don't want to maintain a list as people do drive-by contributions and we need to figure out at what level of contribution mandates a new MODULE_AUTHOR line. Now, that said I would be ok to duplicate the MAINTAINERS as MODULE_AUTHOR lines, but I otherwise expect MAINTAINERS is the central source for module contact info.
On Wed, Nov 11, 2020 at 9:17 AM Dan Williams <dan.j.williams@intel.com> wrote: [..] > > > + > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > > + return pos; > > > + > > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > > > Overly long lines again. > > I thought 100 is the new 80 these days? Saw your clarification to Vishal, I had missed that. Will trim.
On 11/11/20 9:17 AM, Dan Williams wrote: > On Tue, Nov 10, 2020 at 11:12 PM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: >>> +config CXL_MEM >>> + tristate "CXL.mem Device Support" >>> + depends on PCI && CXL_BUS_PROVIDER != n >> >> depend on PCI && CXL_BUS_PROVIDER >> >>> + default m if CXL_BUS_PROVIDER >> >> Please don't set weird defaults for new code. Especially not default >> to module crap like this. > > This goes back to what people like Dave C. asked for LIBNVDIMM / DAX, > a way to blanket turn on a subsystem without needing to go hunt down > individual configs. All of CXL is "default n", but if someone turns on > a piece of it they get all of it by default. The user can then opt-out > on pieces after that first opt-in. If there's a better way to turn on > suggested configs I'm open to switch to that style. As for the > "default m" I was worried that it would be "default y" without the > specificity, but I did not test that... will check. There have been > times when I wished that distros defaulted bleeding edge new enabling > to 'm' and putting that default in the Kconfig maybe saves me from > needing to file individual config changes to distros after the fact. What we as developers put into mainline kernel Kconfig files has nothing to do with what distros use in their distro config files. Or at least it shouldn't. Maybe your experience has been different. >> >>> +// Copyright(c) 2020 Intel Corporation. All rights reserved. >> >> Please don't use '//' for anything but the SPDX header. > > Ok, I find // following by /* */ a bit ugly, but I don't care enough to fight. > Hm, it's not in coding-style AFAICT but Linus has OK-ed C99 style comments: http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html >>> +MODULE_AUTHOR("Intel Corporation"); >> >> A module author is not a company. > > At least I don't have a copyright assignment clause, I don't agree > with the vanity of listing multiple people here especially when > MAINTAINERS has the contact info, and I don't want to maintain a list > as people do drive-by contributions and we need to figure out at what > level of contribution mandates a new MODULE_AUTHOR line. Now, that > said I would be ok to duplicate the MAINTAINERS as MODULE_AUTHOR > lines, but I otherwise expect MAINTAINERS is the central source for > module contact info. Sure, MAINTAINERS is fine, but the MODULE_AUTHOR() above provides no useful information. Even saying (made up) linux-devel@linux.intel.com would be slightly better, but some kind of contact info would be great. Otherwise just delete that line.
On Wed, Nov 11, 2020 at 1:42 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > On 11/11/20 9:17 AM, Dan Williams wrote: > > On Tue, Nov 10, 2020 at 11:12 PM Christoph Hellwig <hch@infradead.org> wrote: > >> > >> On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: > >>> +config CXL_MEM > >>> + tristate "CXL.mem Device Support" > >>> + depends on PCI && CXL_BUS_PROVIDER != n > >> > >> depend on PCI && CXL_BUS_PROVIDER > >> > >>> + default m if CXL_BUS_PROVIDER > >> > >> Please don't set weird defaults for new code. Especially not default > >> to module crap like this. > > > > This goes back to what people like Dave C. asked for LIBNVDIMM / DAX, > > a way to blanket turn on a subsystem without needing to go hunt down > > individual configs. All of CXL is "default n", but if someone turns on > > a piece of it they get all of it by default. The user can then opt-out > > on pieces after that first opt-in. If there's a better way to turn on > > suggested configs I'm open to switch to that style. As for the > > "default m" I was worried that it would be "default y" without the > > specificity, but I did not test that... will check. There have been > > times when I wished that distros defaulted bleeding edge new enabling > > to 'm' and putting that default in the Kconfig maybe saves me from > > needing to file individual config changes to distros after the fact. > > What we as developers put into mainline kernel Kconfig files has nothing > to do with what distros use in their distro config files. > Or at least it shouldn't. Maybe your experience has been different. I agree with that sentiment, but walk it back through the requirement I mentioned above... *if* we want a top-level CXL option (default n) that goes and enables many CXL sub-options the default for those sub-options is something that needs to be listed in the Kconfig. 'm' is more flexible than 'y', so if a user wants CXL at all, and doesn't care about how, I'd prefer it's 'm' rather than 'y'. I have had to go submit distro config fixes when Kconfig defaulted to 'y' when 'm' was available, and the reasoning for why it was 'y' was "oh, that was the Kconfig default when I flipped this other option". > >>> +// Copyright(c) 2020 Intel Corporation. All rights reserved. > >> > >> Please don't use '//' for anything but the SPDX header. > > > > Ok, I find // following by /* */ a bit ugly, but I don't care enough to fight. > > > > Hm, it's not in coding-style AFAICT but Linus has OK-ed C99 style comments: > http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html > > > >>> +MODULE_AUTHOR("Intel Corporation"); > >> > >> A module author is not a company. > > > > At least I don't have a copyright assignment clause, I don't agree > > with the vanity of listing multiple people here especially when > > MAINTAINERS has the contact info, and I don't want to maintain a list > > as people do drive-by contributions and we need to figure out at what > > level of contribution mandates a new MODULE_AUTHOR line. Now, that > > said I would be ok to duplicate the MAINTAINERS as MODULE_AUTHOR > > lines, but I otherwise expect MAINTAINERS is the central source for > > module contact info. > > Sure, MAINTAINERS is fine, but the MODULE_AUTHOR() above provides > no useful information. > Even saying (made up) linux-devel@linux.intel.com would be slightly better, > but some kind of contact info would be great. Otherwise just delete that line. True, if the goal is to allow random end users to email support questions about this module I'd rather not put my email there. Instead, if it's someone that has kernel development questions then they should be able to use MAINTAINERS for that contact.
On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: > From: Dan Williams <dan.j.williams@intel.com> > > The CXL.mem protocol allows a device to act as a provider of "System > RAM" and/or "Persistent Memory" that is fully coherent as if the memory > was attached to the typical CPU memory controller. > > The memory range exported by the device may optionally be described by > the platform firmware memory map, or by infrastructure like LIBNVDIMM to > provision persistent memory capacity from one, or more, CXL.mem devices. > > A pre-requisite for Linux-managed memory-capacity provisioning is this > cxl_mem driver that can speak the "type-3 mailbox" protocol. "Type 3" to indicate that this is a proper adjective that can be looked up in the spec and to match the usage there. The r1.1 spec I have doesn't mention "mailbox". Is that also something defined in the 2.0 spec? A URL or similar citation for the spec would be nice somewhere. > For now just land the driver boiler-plate and fill it in with > functionality in subsequent commits. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/Kconfig | 20 +++++++++++ > drivers/cxl/Makefile | 2 ++ > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/pci.h | 15 ++++++++ > 4 files changed, 119 insertions(+) > create mode 100644 drivers/cxl/mem.c > create mode 100644 drivers/cxl/pci.h > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > index dd724bd364df..15548f5c77ff 100644 > --- a/drivers/cxl/Kconfig > +++ b/drivers/cxl/Kconfig > @@ -27,4 +27,24 @@ config CXL_ACPI > resources described by the CEDT (CXL Early Discovery Table) > > Say 'y' to enable CXL (Compute Express Link) drivers. > + > +config CXL_MEM > + tristate "CXL.mem Device Support" > + depends on PCI && CXL_BUS_PROVIDER != n > + default m if CXL_BUS_PROVIDER > + help > + The CXL.mem protocol allows a device to act as a provider of > + "System RAM" and/or "Persistent Memory" that is fully coherent > + as if the memory was attached to the typical CPU memory > + controller. > + > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach > + to CXL.mem devices for configuration, provisioning, and health > + monitoring, the so called "type-3 mailbox". Note, this driver "Type 3" > + is required for dynamic provisioning of CXL.mem attached > + memory, a pre-requisite for persistent memory support, but > + devices that provide volatile memory may be fully described by > + existing platform firmware memory enumeration. > + > + If unsure say 'n'. > endif > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > index d38cd34a2582..97fdffb00f2d 100644 > --- a/drivers/cxl/Makefile > +++ b/drivers/cxl/Makefile > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > +obj-$(CONFIG_CXL_MEM) += cxl_mem.o > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > cxl_acpi-y := acpi.o > +cxl_mem-y := mem.o > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > new file mode 100644 > index 000000000000..aa7d881fa47b > --- /dev/null > +++ b/drivers/cxl/mem.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/io.h> > +#include "acpi.h" > +#include "pci.h" > + > +struct cxl_mem { > + void __iomem *regs; > +}; Unused, maybe move it to the patch that adds the use? > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > +{ > + int pos; > + > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > + if (!pos) > + return 0; > + > + while (pos) { > + u16 vendor, id; > + > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > + return pos; > + > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > + } > + > + return 0; > +} I assume we'll refactor and move this into the PCI core after we resolve the several places this is needed. When we do that, the vendor would be passed in, so maybe we should do that here to make it simpler to move this to the PCI core. > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + struct device *dev = &pdev->dev; > + struct cxl_mem *cxlm; > + int rc, regloc; > + > + rc = cxl_bus_prepared(pdev); > + if (rc != 0) { > + dev_err(dev, "failed to acquire interface\n"); Interesting naming: apparently when cxl_bus_prepared() returns a non-zero ("true") value, it is actually *not* prepared? > + return rc; > + } > + > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > + if (!regloc) { > + dev_err(dev, "register location dvsec not found\n"); > + return -ENXIO; > + } > + > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > + if (!cxlm) > + return -ENOMEM; Unused. And [4/9] removes it before it's *ever* used :) > + return 0; > +} > + > +static void cxl_mem_remove(struct pci_dev *pdev) > +{ > +} > + > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > + /* PCI class code for CXL.mem Type-3 Devices */ > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > + { /* terminate list */ }, > +}; > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > + > +static struct pci_driver cxl_mem_driver = { > + .name = KBUILD_MODNAME, > + .id_table = cxl_mem_pci_tbl, > + .probe = cxl_mem_probe, > + .remove = cxl_mem_remove, > +}; > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Intel Corporation"); > +module_pci_driver(cxl_mem_driver); > +MODULE_IMPORT_NS(CXL); > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > new file mode 100644 > index 000000000000..beb03921e6da > --- /dev/null > +++ b/drivers/cxl/pci.h > @@ -0,0 +1,15 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. /* SPDX-... */ /* Copyright ...*/ The SPDX rules are a bit arcane and annoyingly hard to grep for, but I found them in Documentation/process/license-rules.rst > +#ifndef __CXL_PCI_H__ > +#define __CXL_PCI_H__ > + > +#define PCI_CLASS_MEMORY_CXL 0x050210 > + > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > +#define PCI_DVSEC_ID_OFFSET 0x8 > +#define PCI_DVSEC_ID_CXL 0x0 > +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 I assume these will go in include/linux/pci_ids.h (PCI_CLASS_...) and include/uapi/linux/pci_regs.h (the rest) eventually, after we get the merge issues sorted out. But if they're only used in cxl/mem.c, I'd put them there for now. > +#endif /* __CXL_PCI_H__ */ > -- > 2.29.2 >
On 20-11-13 12:17:28, Bjorn Helgaas wrote: > On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote: > > From: Dan Williams <dan.j.williams@intel.com> > > > > The CXL.mem protocol allows a device to act as a provider of "System > > RAM" and/or "Persistent Memory" that is fully coherent as if the memory > > was attached to the typical CPU memory controller. > > > > The memory range exported by the device may optionally be described by > > the platform firmware memory map, or by infrastructure like LIBNVDIMM to > > provision persistent memory capacity from one, or more, CXL.mem devices. > > > > A pre-requisite for Linux-managed memory-capacity provisioning is this > > cxl_mem driver that can speak the "type-3 mailbox" protocol. > > "Type 3" to indicate that this is a proper adjective that can be > looked up in the spec and to match the usage there. > > The r1.1 spec I have doesn't mention "mailbox". Is that also > something defined in the 2.0 spec? Yes, these device types are new to 2.0. > > A URL or similar citation for the spec would be nice somewhere. > Agreed. For the patches I authored at least, it seemed repetitive to put a Link: in each one to the spec. It was meant to be in the cover letter, but obviously I missed that. Do you have a suggestion there, is cover letter good enough? > > For now just land the driver boiler-plate and fill it in with > > functionality in subsequent commits. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/Kconfig | 20 +++++++++++ > > drivers/cxl/Makefile | 2 ++ > > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/pci.h | 15 ++++++++ > > 4 files changed, 119 insertions(+) > > create mode 100644 drivers/cxl/mem.c > > create mode 100644 drivers/cxl/pci.h > > > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > > index dd724bd364df..15548f5c77ff 100644 > > --- a/drivers/cxl/Kconfig > > +++ b/drivers/cxl/Kconfig > > @@ -27,4 +27,24 @@ config CXL_ACPI > > resources described by the CEDT (CXL Early Discovery Table) > > > > Say 'y' to enable CXL (Compute Express Link) drivers. > > + > > +config CXL_MEM > > + tristate "CXL.mem Device Support" > > + depends on PCI && CXL_BUS_PROVIDER != n > > + default m if CXL_BUS_PROVIDER > > + help > > + The CXL.mem protocol allows a device to act as a provider of > > + "System RAM" and/or "Persistent Memory" that is fully coherent > > + as if the memory was attached to the typical CPU memory > > + controller. > > + > > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach > > + to CXL.mem devices for configuration, provisioning, and health > > + monitoring, the so called "type-3 mailbox". Note, this driver > > "Type 3" > > > + is required for dynamic provisioning of CXL.mem attached > > + memory, a pre-requisite for persistent memory support, but > > + devices that provide volatile memory may be fully described by > > + existing platform firmware memory enumeration. > > + > > + If unsure say 'n'. > > endif > > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > > index d38cd34a2582..97fdffb00f2d 100644 > > --- a/drivers/cxl/Makefile > > +++ b/drivers/cxl/Makefile > > @@ -1,5 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > > +obj-$(CONFIG_CXL_MEM) += cxl_mem.o > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > > cxl_acpi-y := acpi.o > > +cxl_mem-y := mem.o > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > new file mode 100644 > > index 000000000000..aa7d881fa47b > > --- /dev/null > > +++ b/drivers/cxl/mem.c > > @@ -0,0 +1,82 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > +#include <linux/module.h> > > +#include <linux/pci.h> > > +#include <linux/io.h> > > +#include "acpi.h" > > +#include "pci.h" > > + > > +struct cxl_mem { > > + void __iomem *regs; > > +}; > > Unused, maybe move it to the patch that adds the use? > This is a remnant from when Dan gave me the basis to do the mmio work. I agree it can be removed now. > > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > +{ > > + int pos; > > + > > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > > + if (!pos) > > + return 0; > > + > > + while (pos) { > > + u16 vendor, id; > > + > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > + return pos; > > + > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > + } > > + > > + return 0; > > +} > > I assume we'll refactor and move this into the PCI core after we > resolve the several places this is needed. When we do that, the > vendor would be passed in, so maybe we should do that here to make it > simpler to move this to the PCI core. > I think we'll need to keep this in order to try to keep the dream alive of loading a CXL kernel module on an older kernel. However, PCI code would benefit from having it (in an ideal world, it'd only be there). > > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cxl_mem *cxlm; > > + int rc, regloc; > > + > > + rc = cxl_bus_prepared(pdev); > > + if (rc != 0) { > > + dev_err(dev, "failed to acquire interface\n"); > > Interesting naming: apparently when cxl_bus_prepared() returns a > non-zero ("true") value, it is actually *not* prepared? > This looks like a rebase fail to me, but I'll let Dan answer. > > + return rc; > > + } > > + > > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > > + if (!regloc) { > > + dev_err(dev, "register location dvsec not found\n"); > > + return -ENXIO; > > + } > > + > > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > > + if (!cxlm) > > + return -ENOMEM; > > Unused. And [4/9] removes it before it's *ever* used :) > Same as a few above, I think Dan was providing this for me to implement the reset. It could go away... > > + return 0; > > +} > > + > > +static void cxl_mem_remove(struct pci_dev *pdev) > > +{ > > +} > > + > > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > > + /* PCI class code for CXL.mem Type-3 Devices */ > > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > > + { /* terminate list */ }, > > +}; > > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > > + > > +static struct pci_driver cxl_mem_driver = { > > + .name = KBUILD_MODNAME, > > + .id_table = cxl_mem_pci_tbl, > > + .probe = cxl_mem_probe, > > + .remove = cxl_mem_remove, > > +}; > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_AUTHOR("Intel Corporation"); > > +module_pci_driver(cxl_mem_driver); > > +MODULE_IMPORT_NS(CXL); > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > new file mode 100644 > > index 000000000000..beb03921e6da > > --- /dev/null > > > +++ b/drivers/cxl/pci.h > > @@ -0,0 +1,15 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > /* SPDX-... */ > /* Copyright ...*/ > > The SPDX rules are a bit arcane and annoyingly hard to grep for, but > I found them in Documentation/process/license-rules.rst > > > +#ifndef __CXL_PCI_H__ > > +#define __CXL_PCI_H__ > > + > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > + > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > +#define PCI_DVSEC_ID_CXL 0x0 > > +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 > > I assume these will go in include/linux/pci_ids.h (PCI_CLASS_...) and > include/uapi/linux/pci_regs.h (the rest) eventually, after we get the > merge issues sorted out. But if they're only used in cxl/mem.c, I'd > put them there for now. > > > +#endif /* __CXL_PCI_H__ */ > > -- > > 2.29.2 > >
On Fri, Nov 13, 2020 at 5:09 PM Ben Widawsky <ben.widawsky@intel.com> wrote: [..] > > Unused, maybe move it to the patch that adds the use? > > > > This is a remnant from when Dan gave me the basis to do the mmio work. I agree > it can be removed now. Yes. > > > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > > +{ > > > + int pos; > > > + > > > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > > > + if (!pos) > > > + return 0; > > > + > > > + while (pos) { > > > + u16 vendor, id; > > > + > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > > + return pos; > > > + > > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > > + } > > > + > > > + return 0; > > > +} > > > > I assume we'll refactor and move this into the PCI core after we > > resolve the several places this is needed. When we do that, the > > vendor would be passed in, so maybe we should do that here to make it > > simpler to move this to the PCI core. > > > > I think we'll need to keep this in order to try to keep the dream alive of > loading a CXL kernel module on an older kernel. However, PCI code would benefit > from having it (in an ideal world, it'd only be there). So I think this is fine / expected to move standalone common code like this to the PCI core. What I'm aiming to avoid with "the dream" Ben references is unnecessary dependencies on core changes. CXL is large enough that it will generate more backport pressure than ACPI NFIT / LIBNVDIMM ever did. From a self interest perspective maximizing how much of CXL can be enabled without core dependencies is a goal just to lighten my own backport load. The internals of cxl_mem_dvsec() are simple enough to backport. > > > > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct cxl_mem *cxlm; > > > + int rc, regloc; > > > + > > > + rc = cxl_bus_prepared(pdev); > > > + if (rc != 0) { > > > + dev_err(dev, "failed to acquire interface\n"); > > > > Interesting naming: apparently when cxl_bus_prepared() returns a > > non-zero ("true") value, it is actually *not* prepared? > > > > This looks like a rebase fail to me, but I'll let Dan answer. Yeah, I originally envisioned this as a ternary result with -EPROBE_DEFER as a possible return value, but now that we've found a way to handle CXL _OSC without colliding with legacy PCIE _OSC this can indeed move to a boolean result. Will fix up. > > > > + return rc; > > > + } > > > + > > > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > > > + if (!regloc) { > > > + dev_err(dev, "register location dvsec not found\n"); > > > + return -ENXIO; > > > + } > > > + > > > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > > > + if (!cxlm) > > > + return -ENOMEM; > > > > Unused. And [4/9] removes it before it's *ever* used :) > > > > Same as a few above, I think Dan was providing this for me to implement the > reset. It could go away... Yes, a collaboration artifact that we can clean up. > > > > + return 0; > > > +} > > > + > > > +static void cxl_mem_remove(struct pci_dev *pdev) > > > +{ > > > +} > > > + > > > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > > > + /* PCI class code for CXL.mem Type-3 Devices */ > > > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > > > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > > > + { /* terminate list */ }, > > > +}; > > > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > > > + > > > +static struct pci_driver cxl_mem_driver = { > > > + .name = KBUILD_MODNAME, > > > + .id_table = cxl_mem_pci_tbl, > > > + .probe = cxl_mem_probe, > > > + .remove = cxl_mem_remove, > > > +}; > > > + > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_AUTHOR("Intel Corporation"); > > > +module_pci_driver(cxl_mem_driver); > > > +MODULE_IMPORT_NS(CXL); > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > new file mode 100644 > > > index 000000000000..beb03921e6da > > > --- /dev/null > > > > > +++ b/drivers/cxl/pci.h > > > @@ -0,0 +1,15 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > > > /* SPDX-... */ > > /* Copyright ...*/ > > > > The SPDX rules are a bit arcane and annoyingly hard to grep for, but > > I found them in Documentation/process/license-rules.rst Yes, I did not realize the header vs source /* */ vs // SPDX style. > > > > > +#ifndef __CXL_PCI_H__ > > > +#define __CXL_PCI_H__ > > > + > > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > > + > > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > > +#define PCI_DVSEC_ID_CXL 0x0 > > > +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 > > > > I assume these will go in include/linux/pci_ids.h (PCI_CLASS_...) and > > include/uapi/linux/pci_regs.h (the rest) eventually, after we get the > > merge issues sorted out. But if they're only used in cxl/mem.c, I'd > > put them there for now. Yes, I assume they'll move eventually. I'm cheating a standalone backport driver organization in the meantime.
On Wed, Nov 11, 2020 at 09:17:37AM -0800, Dan Williams wrote: > > > +config CXL_MEM > > > + tristate "CXL.mem Device Support" > > > + depends on PCI && CXL_BUS_PROVIDER != n > > > > depend on PCI && CXL_BUS_PROVIDER > > > > > + default m if CXL_BUS_PROVIDER > > > > Please don't set weird defaults for new code. Especially not default > > to module crap like this. > > This goes back to what people like Dave C. asked for LIBNVDIMM / DAX, > a way to blanket turn on a subsystem without needing to go hunt down > individual configs. Then at least do a default CXL_BUS_PROVIDER but we really don't do this elsewhere. E.g. we don't default the scsi disk driver on if there is some host adapter selected. > > > +MODULE_AUTHOR("Intel Corporation"); > > > > A module author is not a company. > > At least I don't have a copyright assignment clause, I don't agree > with the vanity of listing multiple people here especially when > MAINTAINERS has the contact info, and I don't want to maintain a list > as people do drive-by contributions and we need to figure out at what > level of contribution mandates a new MODULE_AUTHOR line. Now, that > said I would be ok to duplicate the MAINTAINERS as MODULE_AUTHOR > lines, but I otherwise expect MAINTAINERS is the central source for > module contact info. IMHO MODULE_AUTHOR is completely pointless. I haven't used for ~15 years. Especially as the concept that a module has a single author is a rather strange one.
On Tue, 10 Nov 2020 21:43:50 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > From: Dan Williams <dan.j.williams@intel.com> > > The CXL.mem protocol allows a device to act as a provider of "System > RAM" and/or "Persistent Memory" that is fully coherent as if the memory > was attached to the typical CPU memory controller. > > The memory range exported by the device may optionally be described by > the platform firmware memory map, or by infrastructure like LIBNVDIMM to > provision persistent memory capacity from one, or more, CXL.mem devices. > > A pre-requisite for Linux-managed memory-capacity provisioning is this > cxl_mem driver that can speak the "type-3 mailbox" protocol. > > For now just land the driver boiler-plate and fill it in with > functionality in subsequent commits. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> I've tried to avoid repeats, so mostly this is me moaning about naming! Jonathan > --- > drivers/cxl/Kconfig | 20 +++++++++++ > drivers/cxl/Makefile | 2 ++ > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/pci.h | 15 ++++++++ > 4 files changed, 119 insertions(+) > create mode 100644 drivers/cxl/mem.c > create mode 100644 drivers/cxl/pci.h > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > index dd724bd364df..15548f5c77ff 100644 > --- a/drivers/cxl/Kconfig > +++ b/drivers/cxl/Kconfig > @@ -27,4 +27,24 @@ config CXL_ACPI > resources described by the CEDT (CXL Early Discovery Table) > > Say 'y' to enable CXL (Compute Express Link) drivers. > + > +config CXL_MEM > + tristate "CXL.mem Device Support" > + depends on PCI && CXL_BUS_PROVIDER != n > + default m if CXL_BUS_PROVIDER > + help > + The CXL.mem protocol allows a device to act as a provider of > + "System RAM" and/or "Persistent Memory" that is fully coherent > + as if the memory was attached to the typical CPU memory > + controller. > + > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach > + to CXL.mem devices for configuration, provisioning, and health > + monitoring, the so called "type-3 mailbox". Note, this driver > + is required for dynamic provisioning of CXL.mem attached > + memory, a pre-requisite for persistent memory support, but > + devices that provide volatile memory may be fully described by > + existing platform firmware memory enumeration. > + > + If unsure say 'n'. > endif > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > index d38cd34a2582..97fdffb00f2d 100644 > --- a/drivers/cxl/Makefile > +++ b/drivers/cxl/Makefile > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > +obj-$(CONFIG_CXL_MEM) += cxl_mem.o > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > cxl_acpi-y := acpi.o > +cxl_mem-y := mem.o > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > new file mode 100644 > index 000000000000..aa7d881fa47b > --- /dev/null > +++ b/drivers/cxl/mem.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/io.h> > +#include "acpi.h" > +#include "pci.h" > + > +struct cxl_mem { > + void __iomem *regs; > +}; > + > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > +{ > + int pos; > + > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > + if (!pos) > + return 0; > + > + while (pos) { > + u16 vendor, id; > + > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > + return pos; > + > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); This is good generic code and wouldn't cause much backport effort (even if needed to bring in a local copy), so perhaps make it a generic function and move to core PCI code? Mind you I guess that can happen the 'second' time someone wants to find a DVSEC. > + } > + > + return 0; > +} > + > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + struct device *dev = &pdev->dev; > + struct cxl_mem *cxlm; > + int rc, regloc; > + > + rc = cxl_bus_prepared(pdev); > + if (rc != 0) { > + dev_err(dev, "failed to acquire interface\n"); > + return rc; > + } > + > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > + if (!regloc) { > + dev_err(dev, "register location dvsec not found\n"); > + return -ENXIO; > + } > + > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > + if (!cxlm) > + return -ENOMEM; > + > + return 0; > +} > + > +static void cxl_mem_remove(struct pci_dev *pdev) > +{ > +} I'd bring this in only when needed in later patch. > + > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > + /* PCI class code for CXL.mem Type-3 Devices */ > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > + { /* terminate list */ }, > +}; > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > + > +static struct pci_driver cxl_mem_driver = { > + .name = KBUILD_MODNAME, > + .id_table = cxl_mem_pci_tbl, > + .probe = cxl_mem_probe, > + .remove = cxl_mem_remove, > +}; > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Intel Corporation"); > +module_pci_driver(cxl_mem_driver); > +MODULE_IMPORT_NS(CXL); > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > new file mode 100644 > index 000000000000..beb03921e6da > --- /dev/null > +++ b/drivers/cxl/pci.h > @@ -0,0 +1,15 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > +#ifndef __CXL_PCI_H__ > +#define __CXL_PCI_H__ > + > +#define PCI_CLASS_MEMORY_CXL 0x050210 > + > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 Hmm. The magic question of what to call a vendor ID that isn't a vendor ID but just a magic number that talks like a duck and quacks like a duck (for anyone wondering what I'm talking about, there is a nice bit of legal boilerplate on this in the CXL spec) This name is definitely not accurate however. PCI_UNIQUE_VALUE_CXL maybe? It is used for other things than DVSEC (VDMs etc), though possibly this is the only software visible use. > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > +#define PCI_DVSEC_ID_OFFSET 0x8 Put a line break here perhaps and maybe a spec reference to where to find the various DVSEC IDs. > +#define PCI_DVSEC_ID_CXL 0x0 That's definitely a confusing name as well. PCI_DEVSEC_ID_CXL_DEVICE maybe? > +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 > + > +#endif /* __CXL_PCI_H__ */
On Tue, Nov 17, 2020 at 6:50 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Tue, 10 Nov 2020 21:43:50 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > > > The CXL.mem protocol allows a device to act as a provider of "System > > RAM" and/or "Persistent Memory" that is fully coherent as if the memory > > was attached to the typical CPU memory controller. > > > > The memory range exported by the device may optionally be described by > > the platform firmware memory map, or by infrastructure like LIBNVDIMM to > > provision persistent memory capacity from one, or more, CXL.mem devices. > > > > A pre-requisite for Linux-managed memory-capacity provisioning is this > > cxl_mem driver that can speak the "type-3 mailbox" protocol. > > > > For now just land the driver boiler-plate and fill it in with > > functionality in subsequent commits. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > I've tried to avoid repeats, so mostly this is me moaning about naming! > > Jonathan > > > --- > > drivers/cxl/Kconfig | 20 +++++++++++ > > drivers/cxl/Makefile | 2 ++ > > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/pci.h | 15 ++++++++ > > 4 files changed, 119 insertions(+) > > create mode 100644 drivers/cxl/mem.c > > create mode 100644 drivers/cxl/pci.h > > > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > > index dd724bd364df..15548f5c77ff 100644 > > --- a/drivers/cxl/Kconfig > > +++ b/drivers/cxl/Kconfig > > @@ -27,4 +27,24 @@ config CXL_ACPI > > resources described by the CEDT (CXL Early Discovery Table) > > > > Say 'y' to enable CXL (Compute Express Link) drivers. > > + > > +config CXL_MEM > > + tristate "CXL.mem Device Support" > > + depends on PCI && CXL_BUS_PROVIDER != n > > + default m if CXL_BUS_PROVIDER > > + help > > + The CXL.mem protocol allows a device to act as a provider of > > + "System RAM" and/or "Persistent Memory" that is fully coherent > > + as if the memory was attached to the typical CPU memory > > + controller. > > + > > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach > > + to CXL.mem devices for configuration, provisioning, and health > > + monitoring, the so called "type-3 mailbox". Note, this driver > > + is required for dynamic provisioning of CXL.mem attached > > + memory, a pre-requisite for persistent memory support, but > > + devices that provide volatile memory may be fully described by > > + existing platform firmware memory enumeration. > > + > > + If unsure say 'n'. > > endif > > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > > index d38cd34a2582..97fdffb00f2d 100644 > > --- a/drivers/cxl/Makefile > > +++ b/drivers/cxl/Makefile > > @@ -1,5 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > > +obj-$(CONFIG_CXL_MEM) += cxl_mem.o > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > > cxl_acpi-y := acpi.o > > +cxl_mem-y := mem.o > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > new file mode 100644 > > index 000000000000..aa7d881fa47b > > --- /dev/null > > +++ b/drivers/cxl/mem.c > > @@ -0,0 +1,82 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > +#include <linux/module.h> > > +#include <linux/pci.h> > > +#include <linux/io.h> > > +#include "acpi.h" > > +#include "pci.h" > > + > > +struct cxl_mem { > > + void __iomem *regs; > > +}; > > + > > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > +{ > > + int pos; > > + > > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > > + if (!pos) > > + return 0; > > + > > + while (pos) { > > + u16 vendor, id; > > + > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > + return pos; > > + > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > This is good generic code and wouldn't cause much backport effort (even if needed > to bring in a local copy), so perhaps make it a generic function and move to > core PCI code? > > Mind you I guess that can happen the 'second' time someone wants to find a DVSEC. > > > + } > > + > > + return 0; > > +} > > + > > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > +{ > > + struct device *dev = &pdev->dev; > > + struct cxl_mem *cxlm; > > + int rc, regloc; > > + > > + rc = cxl_bus_prepared(pdev); > > + if (rc != 0) { > > + dev_err(dev, "failed to acquire interface\n"); > > + return rc; > > + } > > + > > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > > + if (!regloc) { > > + dev_err(dev, "register location dvsec not found\n"); > > + return -ENXIO; > > + } > > + > > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > > + if (!cxlm) > > + return -ENOMEM; > > + > > + return 0; > > +} > > + > > +static void cxl_mem_remove(struct pci_dev *pdev) > > +{ > > +} > > I'd bring this in only when needed in later patch. > > > + > > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > > + /* PCI class code for CXL.mem Type-3 Devices */ > > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > > + { /* terminate list */ }, > > +}; > > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > > + > > +static struct pci_driver cxl_mem_driver = { > > + .name = KBUILD_MODNAME, > > + .id_table = cxl_mem_pci_tbl, > > + .probe = cxl_mem_probe, > > + .remove = cxl_mem_remove, > > +}; > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_AUTHOR("Intel Corporation"); > > +module_pci_driver(cxl_mem_driver); > > +MODULE_IMPORT_NS(CXL); > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > new file mode 100644 > > index 000000000000..beb03921e6da > > --- /dev/null > > +++ b/drivers/cxl/pci.h > > @@ -0,0 +1,15 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > +#ifndef __CXL_PCI_H__ > > +#define __CXL_PCI_H__ > > + > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > + > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > Hmm. The magic question of what to call a vendor ID that isn't a vendor > ID but just a magic number that talks like a duck and quacks like a duck > (for anyone wondering what I'm talking about, there is a nice bit of legal > boilerplate on this in the CXL spec) > > This name is definitely not accurate however. > > PCI_UNIQUE_VALUE_CXL maybe? It is used for other things than DVSEC (VDMs etc), > though possibly this is the only software visible use. Finally working my way back through this review to make the changes. If 0x1E98 becomes visible to software somewhere else then this can become something like the following: #define PCI_DVSEC_VENDOR_CXL PCI_UNIQUE_VALUE_CXL ...or whatever the generic name is, but this field per the specification is the DVSEC-vendor-id and calling it PCI_UNIQUE_VALUE_CXL does not have any basis in the spec. I will rename it though to: PCI_DVSEC_VENDOR_ID_CXL ...since include/linux/pci_ids.h includes the _ID_ part. > > > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > Put a line break here perhaps and maybe a spec reference to where to find > the various DVSEC IDs. Ok. > > > +#define PCI_DVSEC_ID_CXL 0x0 > > That's definitely a confusing name as well. Yeah, should be PCI_DVSEC_DEVICE_ID_CXL > > PCI_DEVSEC_ID_CXL_DEVICE maybe? > > > > +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 > > + > > +#endif /* __CXL_PCI_H__ */ >
On Thu, Dec 3, 2020 at 11:22 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Tue, Nov 17, 2020 at 6:50 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Tue, 10 Nov 2020 21:43:50 -0800 > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > From: Dan Williams <dan.j.williams@intel.com> > > > > > > The CXL.mem protocol allows a device to act as a provider of "System > > > RAM" and/or "Persistent Memory" that is fully coherent as if the memory > > > was attached to the typical CPU memory controller. > > > > > > The memory range exported by the device may optionally be described by > > > the platform firmware memory map, or by infrastructure like LIBNVDIMM to > > > provision persistent memory capacity from one, or more, CXL.mem devices. > > > > > > A pre-requisite for Linux-managed memory-capacity provisioning is this > > > cxl_mem driver that can speak the "type-3 mailbox" protocol. > > > > > > For now just land the driver boiler-plate and fill it in with > > > functionality in subsequent commits. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > I've tried to avoid repeats, so mostly this is me moaning about naming! > > > > Jonathan > > > > > --- > > > drivers/cxl/Kconfig | 20 +++++++++++ > > > drivers/cxl/Makefile | 2 ++ > > > drivers/cxl/mem.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ > > > drivers/cxl/pci.h | 15 ++++++++ > > > 4 files changed, 119 insertions(+) > > > create mode 100644 drivers/cxl/mem.c > > > create mode 100644 drivers/cxl/pci.h > > > > > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > > > index dd724bd364df..15548f5c77ff 100644 > > > --- a/drivers/cxl/Kconfig > > > +++ b/drivers/cxl/Kconfig > > > @@ -27,4 +27,24 @@ config CXL_ACPI > > > resources described by the CEDT (CXL Early Discovery Table) > > > > > > Say 'y' to enable CXL (Compute Express Link) drivers. > > > + > > > +config CXL_MEM > > > + tristate "CXL.mem Device Support" > > > + depends on PCI && CXL_BUS_PROVIDER != n > > > + default m if CXL_BUS_PROVIDER > > > + help > > > + The CXL.mem protocol allows a device to act as a provider of > > > + "System RAM" and/or "Persistent Memory" that is fully coherent > > > + as if the memory was attached to the typical CPU memory > > > + controller. > > > + > > > + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach > > > + to CXL.mem devices for configuration, provisioning, and health > > > + monitoring, the so called "type-3 mailbox". Note, this driver > > > + is required for dynamic provisioning of CXL.mem attached > > > + memory, a pre-requisite for persistent memory support, but > > > + devices that provide volatile memory may be fully described by > > > + existing platform firmware memory enumeration. > > > + > > > + If unsure say 'n'. > > > endif > > > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > > > index d38cd34a2582..97fdffb00f2d 100644 > > > --- a/drivers/cxl/Makefile > > > +++ b/drivers/cxl/Makefile > > > @@ -1,5 +1,7 @@ > > > # SPDX-License-Identifier: GPL-2.0 > > > obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > > > +obj-$(CONFIG_CXL_MEM) += cxl_mem.o > > > > > > ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > > > cxl_acpi-y := acpi.o > > > +cxl_mem-y := mem.o > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > new file mode 100644 > > > index 000000000000..aa7d881fa47b > > > --- /dev/null > > > +++ b/drivers/cxl/mem.c > > > @@ -0,0 +1,82 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > > +#include <linux/module.h> > > > +#include <linux/pci.h> > > > +#include <linux/io.h> > > > +#include "acpi.h" > > > +#include "pci.h" > > > + > > > +struct cxl_mem { > > > + void __iomem *regs; > > > +}; > > > + > > > +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > > +{ > > > + int pos; > > > + > > > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); > > > + if (!pos) > > > + return 0; > > > + > > > + while (pos) { > > > + u16 vendor, id; > > > + > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); > > > + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); > > > + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) > > > + return pos; > > > + > > > + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); > > > > This is good generic code and wouldn't cause much backport effort (even if needed > > to bring in a local copy), so perhaps make it a generic function and move to > > core PCI code? > > > > Mind you I guess that can happen the 'second' time someone wants to find a DVSEC. > > > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct cxl_mem *cxlm; > > > + int rc, regloc; > > > + > > > + rc = cxl_bus_prepared(pdev); > > > + if (rc != 0) { > > > + dev_err(dev, "failed to acquire interface\n"); > > > + return rc; > > > + } > > > + > > > + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > > > + if (!regloc) { > > > + dev_err(dev, "register location dvsec not found\n"); > > > + return -ENXIO; > > > + } > > > + > > > + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); > > > + if (!cxlm) > > > + return -ENOMEM; > > > + > > > + return 0; > > > +} > > > + > > > +static void cxl_mem_remove(struct pci_dev *pdev) > > > +{ > > > +} > > > > I'd bring this in only when needed in later patch. > > > > > + > > > +static const struct pci_device_id cxl_mem_pci_tbl[] = { > > > + /* PCI class code for CXL.mem Type-3 Devices */ > > > + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > > > + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, > > > + { /* terminate list */ }, > > > +}; > > > +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); > > > + > > > +static struct pci_driver cxl_mem_driver = { > > > + .name = KBUILD_MODNAME, > > > + .id_table = cxl_mem_pci_tbl, > > > + .probe = cxl_mem_probe, > > > + .remove = cxl_mem_remove, > > > +}; > > > + > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_AUTHOR("Intel Corporation"); > > > +module_pci_driver(cxl_mem_driver); > > > +MODULE_IMPORT_NS(CXL); > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > new file mode 100644 > > > index 000000000000..beb03921e6da > > > --- /dev/null > > > +++ b/drivers/cxl/pci.h > > > @@ -0,0 +1,15 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > > +#ifndef __CXL_PCI_H__ > > > +#define __CXL_PCI_H__ > > > + > > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > > + > > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > > > Hmm. The magic question of what to call a vendor ID that isn't a vendor > > ID but just a magic number that talks like a duck and quacks like a duck > > (for anyone wondering what I'm talking about, there is a nice bit of legal > > boilerplate on this in the CXL spec) > > > > This name is definitely not accurate however. > > > > PCI_UNIQUE_VALUE_CXL maybe? It is used for other things than DVSEC (VDMs etc), > > though possibly this is the only software visible use. > > Finally working my way back through this review to make the changes. > If 0x1E98 becomes visible to software somewhere else then this can > become something like the following: > > #define PCI_DVSEC_VENDOR_CXL PCI_UNIQUE_VALUE_CXL > > ...or whatever the generic name is, but this field per the > specification is the DVSEC-vendor-id and calling it > PCI_UNIQUE_VALUE_CXL does not have any basis in the spec. > > I will rename it though to: > > PCI_DVSEC_VENDOR_ID_CXL > > ...since include/linux/pci_ids.h includes the _ID_ part. > > > > > > > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > > > Put a line break here perhaps and maybe a spec reference to where to find > > the various DVSEC IDs. > > Ok. > > > > > > +#define PCI_DVSEC_ID_CXL 0x0 > > > > That's definitely a confusing name as well. > > Yeah, should be PCI_DVSEC_DEVICE_ID_CXL Actually, no, the spec calls this the "DVSEC id" so PCI_DVSEC_ID_CXL seems suitable to me. This is from: Table 126. PCI Express DVSEC Register Settings for CXL Device In the CXL 2.0 Specification.
... > > > > +MODULE_IMPORT_NS(CXL); > > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > > new file mode 100644 > > > > index 000000000000..beb03921e6da > > > > --- /dev/null > > > > +++ b/drivers/cxl/pci.h > > > > @@ -0,0 +1,15 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > > > +#ifndef __CXL_PCI_H__ > > > > +#define __CXL_PCI_H__ > > > > + > > > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > > > + > > > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > > > > > Hmm. The magic question of what to call a vendor ID that isn't a vendor > > > ID but just a magic number that talks like a duck and quacks like a duck > > > (for anyone wondering what I'm talking about, there is a nice bit of legal > > > boilerplate on this in the CXL spec) > > > > > > This name is definitely not accurate however. > > > > > > PCI_UNIQUE_VALUE_CXL maybe? It is used for other things than DVSEC (VDMs etc), > > > though possibly this is the only software visible use. > > > > Finally working my way back through this review to make the changes. > > If 0x1E98 becomes visible to software somewhere else then this can > > become something like the following: > > > > #define PCI_DVSEC_VENDOR_CXL PCI_UNIQUE_VALUE_CXL > > > > ...or whatever the generic name is, but this field per the > > specification is the DVSEC-vendor-id and calling it > > PCI_UNIQUE_VALUE_CXL does not have any basis in the spec. There is a big statement about it as a footnote to 3.1.2 in CXL 2.0 "The Unique Value that is provided in this specification for use in ... Designated Vendor Specific Extended Capabilities.." And for extra amusement in the "Notice Regarding PCI-SIG Unique Value" that forms part of the click through https://www.computeexpresslink.org/download-the-specification (that's the only use of "PCI-SIG Unique Value" that Google finds but I know of one other similar statement) However, I agree it's being used in DVSEC field only (from software point of view) so fair enough to name it after where it is used rather than what it is. > > > > I will rename it though to: > > > > PCI_DVSEC_VENDOR_ID_CXL > > > > ...since include/linux/pci_ids.h includes the _ID_ part. > > > > > > > > > > > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > > > > > Put a line break here perhaps and maybe a spec reference to where to find > > > the various DVSEC IDs. > > > > Ok. > > > > > > > > > +#define PCI_DVSEC_ID_CXL 0x0 > > > > > > That's definitely a confusing name as well. > > > > Yeah, should be PCI_DVSEC_DEVICE_ID_CXL > > Actually, no, the spec calls this the "DVSEC id" so PCI_DVSEC_ID_CXL > seems suitable to me. This is from: > > Table 126. PCI Express DVSEC Register Settings for CXL Device > > In the CXL 2.0 Specification. The DVSEC ID naming is straight from the PCI spec so that part is fine, my issue is this is one of a whole bunch of CXL related DVSEC ID so it needs a more specific name. PCI_DVSEC_ID_CXL_DEVICE would work in line with table 124. I'm not that bothered though. Jonathan
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig index dd724bd364df..15548f5c77ff 100644 --- a/drivers/cxl/Kconfig +++ b/drivers/cxl/Kconfig @@ -27,4 +27,24 @@ config CXL_ACPI resources described by the CEDT (CXL Early Discovery Table) Say 'y' to enable CXL (Compute Express Link) drivers. + +config CXL_MEM + tristate "CXL.mem Device Support" + depends on PCI && CXL_BUS_PROVIDER != n + default m if CXL_BUS_PROVIDER + help + The CXL.mem protocol allows a device to act as a provider of + "System RAM" and/or "Persistent Memory" that is fully coherent + as if the memory was attached to the typical CPU memory + controller. + + Say 'y/m' to enable a driver named "cxl_mem.ko" that will attach + to CXL.mem devices for configuration, provisioning, and health + monitoring, the so called "type-3 mailbox". Note, this driver + is required for dynamic provisioning of CXL.mem attached + memory, a pre-requisite for persistent memory support, but + devices that provide volatile memory may be fully described by + existing platform firmware memory enumeration. + + If unsure say 'n'. endif diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile index d38cd34a2582..97fdffb00f2d 100644 --- a/drivers/cxl/Makefile +++ b/drivers/cxl/Makefile @@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o +obj-$(CONFIG_CXL_MEM) += cxl_mem.o ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL cxl_acpi-y := acpi.o +cxl_mem-y := mem.o diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c new file mode 100644 index 000000000000..aa7d881fa47b --- /dev/null +++ b/drivers/cxl/mem.c @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright(c) 2020 Intel Corporation. All rights reserved. +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/io.h> +#include "acpi.h" +#include "pci.h" + +struct cxl_mem { + void __iomem *regs; +}; + +static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) +{ + int pos; + + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DVSEC); + if (!pos) + return 0; + + while (pos) { + u16 vendor, id; + + pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor); + pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id); + if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id) + return pos; + + pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC); + } + + return 0; +} + +static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) +{ + struct device *dev = &pdev->dev; + struct cxl_mem *cxlm; + int rc, regloc; + + rc = cxl_bus_prepared(pdev); + if (rc != 0) { + dev_err(dev, "failed to acquire interface\n"); + return rc; + } + + regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); + if (!regloc) { + dev_err(dev, "register location dvsec not found\n"); + return -ENXIO; + } + + cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL); + if (!cxlm) + return -ENOMEM; + + return 0; +} + +static void cxl_mem_remove(struct pci_dev *pdev) +{ +} + +static const struct pci_device_id cxl_mem_pci_tbl[] = { + /* PCI class code for CXL.mem Type-3 Devices */ + { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, + PCI_CLASS_MEMORY_CXL, 0xffffff, 0 }, + { /* terminate list */ }, +}; +MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl); + +static struct pci_driver cxl_mem_driver = { + .name = KBUILD_MODNAME, + .id_table = cxl_mem_pci_tbl, + .probe = cxl_mem_probe, + .remove = cxl_mem_remove, +}; + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Intel Corporation"); +module_pci_driver(cxl_mem_driver); +MODULE_IMPORT_NS(CXL); diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h new file mode 100644 index 000000000000..beb03921e6da --- /dev/null +++ b/drivers/cxl/pci.h @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright(c) 2020 Intel Corporation. All rights reserved. +#ifndef __CXL_PCI_H__ +#define __CXL_PCI_H__ + +#define PCI_CLASS_MEMORY_CXL 0x050210 + +#define PCI_EXT_CAP_ID_DVSEC 0x23 +#define PCI_DVSEC_VENDOR_CXL 0x1E98 +#define PCI_DVSEC_VENDOR_OFFSET 0x4 +#define PCI_DVSEC_ID_OFFSET 0x8 +#define PCI_DVSEC_ID_CXL 0x0 +#define PCI_DVSEC_ID_CXL_REGLOC 0x8 + +#endif /* __CXL_PCI_H__ */