diff mbox series

[RFC,3/9] cxl/mem: Add a driver for the type-3 mailbox

Message ID 20201111054356.793390-4-ben.widawsky@intel.com (mailing list archive)
State RFC, archived
Headers show
Series CXL 2.0 Support | expand

Commit Message

Ben Widawsky Nov. 11, 2020, 5:43 a.m. UTC
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>
---
 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

Comments

Randy Dunlap Nov. 11, 2020, 6:17 a.m. UTC | #1
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
Christoph Hellwig Nov. 11, 2020, 7:12 a.m. UTC | #2
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.
Dan Williams Nov. 11, 2020, 5:17 p.m. UTC | #3
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.
Dan Williams Nov. 11, 2020, 6:27 p.m. UTC | #4
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.
Randy Dunlap Nov. 11, 2020, 9:41 p.m. UTC | #5
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.
Dan Williams Nov. 11, 2020, 10:40 p.m. UTC | #6
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.
Bjorn Helgaas Nov. 13, 2020, 6:17 p.m. UTC | #7
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
>
Ben Widawsky Nov. 14, 2020, 1:08 a.m. UTC | #8
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
> >
Dan Williams Nov. 15, 2020, 12:23 a.m. UTC | #9
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.
Christoph Hellwig Nov. 16, 2020, 4:56 p.m. UTC | #10
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.
Jonathan Cameron Nov. 17, 2020, 2:49 p.m. UTC | #11
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__ */
Dan Williams Dec. 4, 2020, 7:22 a.m. UTC | #12
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__ */
>
Dan Williams Dec. 4, 2020, 7:27 a.m. UTC | #13
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.
Jonathan Cameron Dec. 4, 2020, 5:39 p.m. UTC | #14
...

> > > > +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 mbox series

Patch

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__ */