| Message ID | 20201111054356.793390-2-ben.widawsky@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | CXL 2.0 Support | expand |
Hi, On 11/10/20 9:43 PM, Ben Widawsky wrote: > --- > drivers/Kconfig | 1 + > drivers/Makefile | 1 + > drivers/cxl/Kconfig | 30 +++++++++++ > drivers/cxl/Makefile | 5 ++ > drivers/cxl/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/acpi.h | 15 ++++++ > include/acpi/actbl1.h | 52 ++++++++++++++++++ > 7 files changed, 223 insertions(+) > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > new file mode 100644 > index 000000000000..dd724bd364df > --- /dev/null > +++ b/drivers/cxl/Kconfig > @@ -0,0 +1,30 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +menuconfig CXL_BUS > + tristate "CXL (Compute Express Link) Devices Support" > + help > + CXL is a bus that is electrically compatible with PCI-E, but layers > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The > + CXL.cache protocol allows devices to hold cachelines locally, the > + CXL.mem protocol allows devices to be fully coherent memory targets, the > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for > + the configuration and management of devices supporting these protocols. > + > +if CXL_BUS > + > +config CXL_BUS_PROVIDER > + tristate > + > +config CXL_ACPI > + tristate "CXL Platform Support" > + depends on ACPI > + default CXL_BUS Please provide some justification for something other than the default default of 'n'. We try hard not to add drivers/modules that are not required for bootup. > + select CXL_BUS_PROVIDER > + help > + CXL Platform Support is a prerequisite for any CXL device driver that > + wants to claim ownership of the component register space. By default > + platform firmware assumes Linux is unaware of CXL capabilities and > + requires explicit opt-in. This platform component also mediates > + resources described by the CEDT (CXL Early Discovery Table) end sentence with '.' > + > + Say 'y' to enable CXL (Compute Express Link) drivers. or 'm' > +endif
On Tue, Nov 10, 2020 at 09:43:48PM -0800, Ben Widawsky wrote: > +menuconfig CXL_BUS > + tristate "CXL (Compute Express Link) Devices Support" > + help > + CXL is a bus that is electrically compatible with PCI-E, but layers > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The > + CXL.cache protocol allows devices to hold cachelines locally, the > + CXL.mem protocol allows devices to be fully coherent memory targets, the > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for > + the configuration and management of devices supporting these protocols. > + Please fix the overly long lines. > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) Another overly long line. > +{ > + dev_set_drvdata(dev, acpi_desc); > + acpi_desc->dev = dev; > +} But this helper seems pretty pointless to start with. > +static int acpi_cxl_remove(struct acpi_device *adev) > +{ > + return 0; > +} The emptry remove callback is not needed. > +/* > + * If/when CXL support is defined by other platform firmware the kernel > + * will need a mechanism to select between the platform specific version > + * of this routine, until then, hard-code ACPI assumptions > + */ > +int cxl_bus_prepared(struct pci_dev *pdev) > +{ > + struct acpi_device *adev; > + struct pci_dev *root_port; > + struct device *root; > + > + root_port = pcie_find_root_port(pdev); > + if (!root_port) > + return -ENXIO; > + > + root = root_port->dev.parent; > + if (!root) > + return -ENXIO; > + > + adev = ACPI_COMPANION(root); > + if (!adev) > + return -ENXIO; > + > + /* TODO: OSC enabling */ > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cxl_bus_prepared); What is the point of this function? I doesn't realy do anything, not even a CXL specific check. > > +/******************************************************************************* > + * > + * CEDT - CXL Early Discovery Table (ACPI 6.4) > + * Version 1 > + * > + ******************************************************************************/ > + Pleae use the normal Linux comment style. > +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0) > +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1) > + > +/* Values for length field above */ > + > +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000) > +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000) No need for the braces.
On Wed, 2020-11-11 at 07:10 +0000, Christoph Hellwig wrote: > On Tue, Nov 10, 2020 at 09:43:48PM -0800, Ben Widawsky wrote: > > +menuconfig CXL_BUS > > + tristate "CXL (Compute Express Link) Devices Support" > > + help > > + CXL is a bus that is electrically compatible with PCI-E, but layers > > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The > > + CXL.cache protocol allows devices to hold cachelines locally, the > > + CXL.mem protocol allows devices to be fully coherent memory targets, the > > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for > > + the configuration and management of devices supporting these protocols. > > + > > Please fix the overly long lines. > > > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) > > Another overly long line. Hi Christpph, I thought 100 col. lines were acceptable now. > > > +{ > > + dev_set_drvdata(dev, acpi_desc); > > + acpi_desc->dev = dev; > > +} > > But this helper seems pretty pointless to start with. > > > +static int acpi_cxl_remove(struct acpi_device *adev) > > +{ > > + return 0; > > +} > > The emptry remove callback is not needed. Agreed on both of the above comments - these are just boilerplate for now, I expect they will get filled in in the next revision as more functionality gets fleshed out. If they are still empty/no-op by then I will remove them. > > > +/* > > + * If/when CXL support is defined by other platform firmware the kernel > > + * will need a mechanism to select between the platform specific version > > + * of this routine, until then, hard-code ACPI assumptions > > + */ > > +int cxl_bus_prepared(struct pci_dev *pdev) > > +{ > > + struct acpi_device *adev; > > + struct pci_dev *root_port; > > + struct device *root; > > + > > + root_port = pcie_find_root_port(pdev); > > + if (!root_port) > > + return -ENXIO; > > + > > + root = root_port->dev.parent; > > + if (!root) > > + return -ENXIO; > > + > > + adev = ACPI_COMPANION(root); > > + if (!adev) > > + return -ENXIO; > > + > > + /* TODO: OSC enabling */ > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(cxl_bus_prepared); > > What is the point of this function? I doesn't realy do anything, > not even a CXL specific check. This gets a bit more fleshed out in patch 2. I kept that separate so that it is easier to review the bulk of the _OSC work in that patch without this driver boilerplate getting in the way. > > > > > +/******************************************************************************* > > + * > > + * CEDT - CXL Early Discovery Table (ACPI 6.4) > > + * Version 1 > > + * > > + ******************************************************************************/ > > + > > Pleae use the normal Linux comment style. > > > > +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0) > > +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1) > > + > > +/* Values for length field above */ > > + > > +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000) > > +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000) > > No need for the braces. For both of these - see the note in the commit message. I just followed the ACPI header's style, and these hunks are only in this series to make it usable. I expect the 'actual' struct definitions, naming etc will come through ACPICA.
On Wed, Nov 11, 2020 at 07:30:34AM +0000, Verma, Vishal L wrote: > Hi Christpph, > > I thought 100 col. lines were acceptable now. Quote from the coding style document: "The preferred limit on the length of a single line is 80 columns. Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information." So yes, they are acceptable as an expception. Not for crap like this.
On Wed, 2020-11-11 at 07:34 +0000, hch@infradead.org wrote: > On Wed, Nov 11, 2020 at 07:30:34AM +0000, Verma, Vishal L wrote: > > Hi Christpph, > > > > I thought 100 col. lines were acceptable now. > > Quote from the coding style document: > > "The preferred limit on the length of a single line is 80 columns. > > Statements longer than 80 columns should be broken into sensible chunks, > unless exceeding 80 columns significantly increases readability and does > not hide information." > > So yes, they are acceptable as an expception. Not for crap like this. Ah fair enough, I'll reflow all of these for the next revision.
On Tue, Nov 10, 2020 at 09:43:48PM -0800, Ben Widawsky wrote: > +static int acpi_cxl_add(struct acpi_device *adev) > +{ > + struct acpi_cxl_desc *acpi_desc; > + struct device *dev = &adev->dev; > + struct acpi_table_header *tbl; > + acpi_status status = AE_OK; Pointless init. > + acpi_size sz; > + int rc = 0; Pointless init. > + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl); > + if (ACPI_FAILURE(status)) { > + dev_err(dev, "failed to find CEDT at startup\n"); > + return 0; > + } > + > + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl); > + if (rc) > + return rc;
On Tue, 10 Nov 2020 21:43:48 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > From: Vishal Verma <vishal.l.verma@intel.com> > > Add an acpi_cxl module to coordinate the ACPI portions of the CXL > (Compute eXpress Link) interconnect. This driver binds to ACPI0017 > objects in the ACPI tree, and coordinates access to the resources > provided by the ACPI CEDT (CXL Early Discovery Table). I think the qemu series notes that this ACPI0017 is just a proposal at this stage. Please make sure that's highlighted here as well unless that status is out of date. > > It also coordinates operations of the root port _OSC object to notify > platform firmware that the OS has native support for the CXL > capabilities of endpoints. > > Note: the actbl1.h changes are speculative. The expectation is that they > will arrive through the ACPICA tree in due time. > > Cc: Ben Widawsky <ben.widawsky@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/Kconfig | 1 + > drivers/Makefile | 1 + > drivers/cxl/Kconfig | 30 +++++++++++ > drivers/cxl/Makefile | 5 ++ > drivers/cxl/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/acpi.h | 15 ++++++ > include/acpi/actbl1.h | 52 ++++++++++++++++++ > 7 files changed, 223 insertions(+) > create mode 100644 drivers/cxl/Kconfig > create mode 100644 drivers/cxl/Makefile > create mode 100644 drivers/cxl/acpi.c > create mode 100644 drivers/cxl/acpi.h > > diff --git a/drivers/Kconfig b/drivers/Kconfig > index dcecc9f6e33f..62c753a73651 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -6,6 +6,7 @@ menu "Device Drivers" > source "drivers/amba/Kconfig" > source "drivers/eisa/Kconfig" > source "drivers/pci/Kconfig" > +source "drivers/cxl/Kconfig" > source "drivers/pcmcia/Kconfig" > source "drivers/rapidio/Kconfig" > > diff --git a/drivers/Makefile b/drivers/Makefile > index c0cd1b9075e3..5dad349de73b 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -73,6 +73,7 @@ obj-$(CONFIG_NVM) += lightnvm/ > obj-y += base/ block/ misc/ mfd/ nfc/ > obj-$(CONFIG_LIBNVDIMM) += nvdimm/ > obj-$(CONFIG_DAX) += dax/ > +obj-$(CONFIG_CXL_BUS) += cxl/ > obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/ > obj-$(CONFIG_NUBUS) += nubus/ > obj-y += macintosh/ > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > new file mode 100644 > index 000000000000..dd724bd364df > --- /dev/null > +++ b/drivers/cxl/Kconfig > @@ -0,0 +1,30 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +menuconfig CXL_BUS > + tristate "CXL (Compute Express Link) Devices Support" > + help > + CXL is a bus that is electrically compatible with PCI-E, but layers > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The > + CXL.cache protocol allows devices to hold cachelines locally, the > + CXL.mem protocol allows devices to be fully coherent memory targets, the > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for > + the configuration and management of devices supporting these protocols. > + > +if CXL_BUS > + > +config CXL_BUS_PROVIDER > + tristate > + > +config CXL_ACPI > + tristate "CXL Platform Support" > + depends on ACPI > + default CXL_BUS > + select CXL_BUS_PROVIDER > + help > + CXL Platform Support is a prerequisite for any CXL device driver that > + wants to claim ownership of the component register space. By default > + platform firmware assumes Linux is unaware of CXL capabilities and > + requires explicit opt-in. This platform component also mediates > + resources described by the CEDT (CXL Early Discovery Table) > + > + Say 'y' to enable CXL (Compute Express Link) drivers. > +endif > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > new file mode 100644 > index 000000000000..d38cd34a2582 > --- /dev/null > +++ b/drivers/cxl/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > + > +ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > +cxl_acpi-y := acpi.o > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > new file mode 100644 > index 000000000000..26e4f73838a7 > --- /dev/null > +++ b/drivers/cxl/acpi.c > @@ -0,0 +1,119 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright(c) 2020 Intel Corporation. All rights reserved. > + */ > +#include <linux/list_sort.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/sysfs.h> > +#include <linux/list.h> > +#include <linux/acpi.h> > +#include <linux/sort.h> > +#include <linux/pci.h> > +#include "acpi.h" > + > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) > +{ > + dev_set_drvdata(dev, acpi_desc); No need to have this wrapper + it hides the fact you are not just initialsing the acpi_desc structure. > + acpi_desc->dev = dev; > +} > + > +static void acpi_cedt_put_table(void *table) > +{ > + acpi_put_table(table); > +} > + > +static int acpi_cxl_add(struct acpi_device *adev) > +{ > + struct acpi_cxl_desc *acpi_desc; > + struct device *dev = &adev->dev; > + struct acpi_table_header *tbl; > + acpi_status status = AE_OK; Set below, so don't do it here. > + acpi_size sz; > + int rc = 0; Set in paths in which it's used so don't do it here. > + > + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl); > + if (ACPI_FAILURE(status)) { > + dev_err(dev, "failed to find CEDT at startup\n"); > + return 0; > + } > + > + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl); > + if (rc) > + return rc; blank line here preferred for readability (do something, then check errors as block) > + sz = tbl->length; > + dev_info(dev, "found CEDT at startup: %lld bytes\n", sz); > + > + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); > + if (!acpi_desc) > + return -ENOMEM; blank line here slightly helps readability. > + acpi_cxl_desc_init(acpi_desc, &adev->dev); > + > + acpi_desc->acpi_header = *tbl; > + > + return 0; > +} > + > +static int acpi_cxl_remove(struct acpi_device *adev) > +{ > + return 0; Don't think empty remove is needed. > +} > + > +static const struct acpi_device_id acpi_cxl_ids[] = { > + { "ACPI0017", 0 }, > + { "", 0 }, > +}; > +MODULE_DEVICE_TABLE(acpi, acpi_cxl_ids); > + > +static struct acpi_driver acpi_cxl_driver = { > + .name = KBUILD_MODNAME, > + .ids = acpi_cxl_ids, > + .ops = { > + .add = acpi_cxl_add, > + .remove = acpi_cxl_remove, > + }, > +}; > + > +/* > + * If/when CXL support is defined by other platform firmware the kernel > + * will need a mechanism to select between the platform specific version > + * of this routine, until then, hard-code ACPI assumptions > + */ > +int cxl_bus_prepared(struct pci_dev *pdev) > +{ > + struct acpi_device *adev; > + struct pci_dev *root_port; > + struct device *root; > + > + root_port = pcie_find_root_port(pdev); > + if (!root_port) > + return -ENXIO; > + > + root = root_port->dev.parent; > + if (!root) > + return -ENXIO; > + > + adev = ACPI_COMPANION(root); > + if (!adev) > + return -ENXIO; > + > + /* TODO: OSC enabling */ > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cxl_bus_prepared); > + > +static __init int acpi_cxl_init(void) > +{ > + return acpi_bus_register_driver(&acpi_cxl_driver); > +} > + > +static __exit void acpi_cxl_exit(void) > +{ > + acpi_bus_unregister_driver(&acpi_cxl_driver); > +} > + > +module_init(acpi_cxl_init); > +module_exit(acpi_cxl_exit); > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Intel Corporation"); > diff --git a/drivers/cxl/acpi.h b/drivers/cxl/acpi.h > new file mode 100644 > index 000000000000..011505475cc6 > --- /dev/null > +++ b/drivers/cxl/acpi.h > @@ -0,0 +1,15 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > + > +#ifndef __CXL_ACPI_H__ > +#define __CXL_ACPI_H__ > +#include <linux/acpi.h> > + > +struct acpi_cxl_desc { > + struct acpi_table_header acpi_header; > + struct device *dev; > +}; > + > +int cxl_bus_prepared(struct pci_dev *pci_dev); > + > +#endif /* __CXL_ACPI_H__ */ > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 43549547ed3e..70f745f526e3 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -28,6 +28,7 @@ > #define ACPI_SIG_BERT "BERT" /* Boot Error Record Table */ > #define ACPI_SIG_BGRT "BGRT" /* Boot Graphics Resource Table */ > #define ACPI_SIG_BOOT "BOOT" /* Simple Boot Flag Table */ > +#define ACPI_SIG_CEDT "CEDT" /* CXL Early Discovery Table */ > #define ACPI_SIG_CPEP "CPEP" /* Corrected Platform Error Polling table */ > #define ACPI_SIG_CSRT "CSRT" /* Core System Resource Table */ > #define ACPI_SIG_DBG2 "DBG2" /* Debug Port table type 2 */ > @@ -1624,6 +1625,57 @@ struct acpi_ibft_target { > u16 reverse_chap_secret_offset; > }; > > +/******************************************************************************* > + * > + * CEDT - CXL Early Discovery Table (ACPI 6.4) > + * Version 1 > + * > + ******************************************************************************/ > + > +struct acpi_table_cedt { > + struct acpi_table_header header; /* Common ACPI table header */ > + u32 reserved; > +}; > + > +/* Values for CEDT structure types */ > + > +enum acpi_cedt_type { > + ACPI_CEDT_TYPE_HOST_BRIDGE = 0, /* CHBS - CXL Host Bridge Structure */ > + ACPI_CEDT_TYPE_CFMWS = 1, /* CFMWS - CXL Fixed Memory Window Structure */ This isn't in the 2.0 spec, so I guess also part of some proposed changes. > +}; > + > +struct acpi_cedt_structure { > + u8 type; > + u8 reserved; > + u16 length; > +}; > + > +/* > + * CEDT Structures, correspond to Type in struct acpi_cedt_structure > + */ > + > +/* 0: CXL Host Bridge Structure */ > + > +struct acpi_cedt_chbs { > + struct acpi_cedt_structure header; > + u32 uid; > + u32 version; > + u32 reserved1; > + u64 base; > + u32 length; > + u32 reserved2; > +}; > + > +/* Values for version field above */ > + > +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0) > +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1) > + > +/* Values for length field above */ > + > +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000) > +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000) > + > /* Reset to default packing */ > > #pragma pack()
On Mon, 2020-11-16 at 17:59 +0000, Jonathan Cameron wrote: > On Tue, 10 Nov 2020 21:43:48 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > From: Vishal Verma <vishal.l.verma@intel.com> > > > > Add an acpi_cxl module to coordinate the ACPI portions of the CXL > > (Compute eXpress Link) interconnect. This driver binds to ACPI0017 > > objects in the ACPI tree, and coordinates access to the resources > > provided by the ACPI CEDT (CXL Early Discovery Table). > > I think the qemu series notes that this ACPI0017 is just a proposal at > this stage. Please make sure that's highlighted here as well unless > that status is out of date. Hi Jonathan, Thank you for the review. The cover letter talks about this, but I agree it would be worth repeating in this patch briefly as I did with the OSC patch. If it is still in a proposal state by the next posting, I'll make sure to add a note here too. > > > + > > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) > > +{ > > + dev_set_drvdata(dev, acpi_desc); > > No need to have this wrapper + it hides the fact you are not just initialsing > the acpi_desc structure. > > > + acpi_desc->dev = dev; > > +} > > + > > +static void acpi_cedt_put_table(void *table) > > +{ > > + acpi_put_table(table); > > +} > > + > > +static int acpi_cxl_add(struct acpi_device *adev) > > +{ > > + struct acpi_cxl_desc *acpi_desc; > > + struct device *dev = &adev->dev; > > + struct acpi_table_header *tbl; > > + acpi_status status = AE_OK; > > Set below, so don't do it here. > > > + acpi_size sz; > > + int rc = 0; > > Set in paths in which it's used so don't do it here. > > > + > > + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl); > > + if (ACPI_FAILURE(status)) { > > + dev_err(dev, "failed to find CEDT at startup\n"); > > + return 0; > > + } > > + > > + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl); > > + if (rc) > > + return rc; > > blank line here preferred for readability (do something, then check errors as > block) > > > + sz = tbl->length; > > + dev_info(dev, "found CEDT at startup: %lld bytes\n", sz); > > + > > + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); > > + if (!acpi_desc) > > + return -ENOMEM; > > blank line here slightly helps readability. > > > + acpi_cxl_desc_init(acpi_desc, &adev->dev); > > + > > + acpi_desc->acpi_header = *tbl; > > + > > + return 0; > > +} > > + > > +static int acpi_cxl_remove(struct acpi_device *adev) > > +{ > > + return 0; > > Don't think empty remove is needed. > Agreed with all of the above, I'll fix for the next posting. > > + > > +/* Values for CEDT structure types */ > > + > > +enum acpi_cedt_type { > > + ACPI_CEDT_TYPE_HOST_BRIDGE = 0, /* CHBS - CXL Host Bridge Structure */ > > + ACPI_CEDT_TYPE_CFMWS = 1, /* CFMWS - CXL Fixed Memory Window Structure */ > > This isn't in the 2.0 spec, so I guess also part of some proposed changes. Yes this got in accidentally, will remove.
On Wed, Nov 11, 2020 at 6:45 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > From: Vishal Verma <vishal.l.verma@intel.com> > > Add an acpi_cxl module to coordinate the ACPI portions of the CXL > (Compute eXpress Link) interconnect. This driver binds to ACPI0017 > objects in the ACPI tree, and coordinates access to the resources > provided by the ACPI CEDT (CXL Early Discovery Table). > > It also coordinates operations of the root port _OSC object to notify > platform firmware that the OS has native support for the CXL > capabilities of endpoints. > > Note: the actbl1.h changes are speculative. The expectation is that they > will arrive through the ACPICA tree in due time. > > Cc: Ben Widawsky <ben.widawsky@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/Kconfig | 1 + > drivers/Makefile | 1 + > drivers/cxl/Kconfig | 30 +++++++++++ > drivers/cxl/Makefile | 5 ++ > drivers/cxl/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/acpi.h | 15 ++++++ > include/acpi/actbl1.h | 52 ++++++++++++++++++ > 7 files changed, 223 insertions(+) > create mode 100644 drivers/cxl/Kconfig > create mode 100644 drivers/cxl/Makefile > create mode 100644 drivers/cxl/acpi.c > create mode 100644 drivers/cxl/acpi.h > > diff --git a/drivers/Kconfig b/drivers/Kconfig > index dcecc9f6e33f..62c753a73651 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -6,6 +6,7 @@ menu "Device Drivers" > source "drivers/amba/Kconfig" > source "drivers/eisa/Kconfig" > source "drivers/pci/Kconfig" > +source "drivers/cxl/Kconfig" > source "drivers/pcmcia/Kconfig" > source "drivers/rapidio/Kconfig" > > diff --git a/drivers/Makefile b/drivers/Makefile > index c0cd1b9075e3..5dad349de73b 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -73,6 +73,7 @@ obj-$(CONFIG_NVM) += lightnvm/ > obj-y += base/ block/ misc/ mfd/ nfc/ > obj-$(CONFIG_LIBNVDIMM) += nvdimm/ > obj-$(CONFIG_DAX) += dax/ > +obj-$(CONFIG_CXL_BUS) += cxl/ > obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/ > obj-$(CONFIG_NUBUS) += nubus/ > obj-y += macintosh/ > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > new file mode 100644 > index 000000000000..dd724bd364df > --- /dev/null > +++ b/drivers/cxl/Kconfig > @@ -0,0 +1,30 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +menuconfig CXL_BUS > + tristate "CXL (Compute Express Link) Devices Support" > + help > + CXL is a bus that is electrically compatible with PCI-E, but layers > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The > + CXL.cache protocol allows devices to hold cachelines locally, the > + CXL.mem protocol allows devices to be fully coherent memory targets, the > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for > + the configuration and management of devices supporting these protocols. > + > +if CXL_BUS > + > +config CXL_BUS_PROVIDER > + tristate > + > +config CXL_ACPI > + tristate "CXL Platform Support" > + depends on ACPI > + default CXL_BUS > + select CXL_BUS_PROVIDER > + help > + CXL Platform Support is a prerequisite for any CXL device driver that > + wants to claim ownership of the component register space. By default > + platform firmware assumes Linux is unaware of CXL capabilities and > + requires explicit opt-in. This platform component also mediates > + resources described by the CEDT (CXL Early Discovery Table) > + > + Say 'y' to enable CXL (Compute Express Link) drivers. > +endif > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile > new file mode 100644 > index 000000000000..d38cd34a2582 > --- /dev/null > +++ b/drivers/cxl/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o > + > +ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL > +cxl_acpi-y := acpi.o > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > new file mode 100644 > index 000000000000..26e4f73838a7 > --- /dev/null > +++ b/drivers/cxl/acpi.c > @@ -0,0 +1,119 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright(c) 2020 Intel Corporation. All rights reserved. > + */ > +#include <linux/list_sort.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/sysfs.h> > +#include <linux/list.h> > +#include <linux/acpi.h> > +#include <linux/sort.h> > +#include <linux/pci.h> > +#include "acpi.h" > + > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) > +{ > + dev_set_drvdata(dev, acpi_desc); > + acpi_desc->dev = dev; > +} > + > +static void acpi_cedt_put_table(void *table) > +{ > + acpi_put_table(table); > +} > + > +static int acpi_cxl_add(struct acpi_device *adev) > +{ > + struct acpi_cxl_desc *acpi_desc; > + struct device *dev = &adev->dev; > + struct acpi_table_header *tbl; > + acpi_status status = AE_OK; > + acpi_size sz; > + int rc = 0; > + > + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl); > + if (ACPI_FAILURE(status)) { > + dev_err(dev, "failed to find CEDT at startup\n"); > + return 0; > + } > + > + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl); > + if (rc) > + return rc; > + sz = tbl->length; > + dev_info(dev, "found CEDT at startup: %lld bytes\n", sz); > + > + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); > + if (!acpi_desc) > + return -ENOMEM; > + acpi_cxl_desc_init(acpi_desc, &adev->dev); > + > + acpi_desc->acpi_header = *tbl; > + > + return 0; > +} > + > +static int acpi_cxl_remove(struct acpi_device *adev) > +{ > + return 0; > +} > + > +static const struct acpi_device_id acpi_cxl_ids[] = { > + { "ACPI0017", 0 }, > + { "", 0 }, > +}; > +MODULE_DEVICE_TABLE(acpi, acpi_cxl_ids); > + > +static struct acpi_driver acpi_cxl_driver = { First of all, no new acpi_driver instances, pretty please! acpi_default_enumeration() should create a platform device with the ACPI0017 ID for you. Can't you provide a driver for this one? > + .name = KBUILD_MODNAME, > + .ids = acpi_cxl_ids, > + .ops = { > + .add = acpi_cxl_add, > + .remove = acpi_cxl_remove, > + }, > +}; > + > +/* > + * If/when CXL support is defined by other platform firmware the kernel > + * will need a mechanism to select between the platform specific version > + * of this routine, until then, hard-code ACPI assumptions > + */ > +int cxl_bus_prepared(struct pci_dev *pdev) > +{ > + struct acpi_device *adev; > + struct pci_dev *root_port; > + struct device *root; > + > + root_port = pcie_find_root_port(pdev); > + if (!root_port) > + return -ENXIO; > + > + root = root_port->dev.parent; > + if (!root) > + return -ENXIO; > + > + adev = ACPI_COMPANION(root); > + if (!adev) > + return -ENXIO; > + > + /* TODO: OSC enabling */ > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(cxl_bus_prepared); > + > +static __init int acpi_cxl_init(void) > +{ > + return acpi_bus_register_driver(&acpi_cxl_driver); > +} > + > +static __exit void acpi_cxl_exit(void) > +{ > + acpi_bus_unregister_driver(&acpi_cxl_driver); > +} > + > +module_init(acpi_cxl_init); > +module_exit(acpi_cxl_exit); > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Intel Corporation"); > diff --git a/drivers/cxl/acpi.h b/drivers/cxl/acpi.h > new file mode 100644 > index 000000000000..011505475cc6 > --- /dev/null > +++ b/drivers/cxl/acpi.h > @@ -0,0 +1,15 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > + > +#ifndef __CXL_ACPI_H__ > +#define __CXL_ACPI_H__ > +#include <linux/acpi.h> > + > +struct acpi_cxl_desc { > + struct acpi_table_header acpi_header; > + struct device *dev; > +}; > + > +int cxl_bus_prepared(struct pci_dev *pci_dev); > + > +#endif /* __CXL_ACPI_H__ */ > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 43549547ed3e..70f745f526e3 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -28,6 +28,7 @@ > #define ACPI_SIG_BERT "BERT" /* Boot Error Record Table */ > #define ACPI_SIG_BGRT "BGRT" /* Boot Graphics Resource Table */ > #define ACPI_SIG_BOOT "BOOT" /* Simple Boot Flag Table */ > +#define ACPI_SIG_CEDT "CEDT" /* CXL Early Discovery Table */ > #define ACPI_SIG_CPEP "CPEP" /* Corrected Platform Error Polling table */ > #define ACPI_SIG_CSRT "CSRT" /* Core System Resource Table */ > #define ACPI_SIG_DBG2 "DBG2" /* Debug Port table type 2 */ > @@ -1624,6 +1625,57 @@ struct acpi_ibft_target { > u16 reverse_chap_secret_offset; > }; > > +/******************************************************************************* > + * > + * CEDT - CXL Early Discovery Table (ACPI 6.4) > + * Version 1 > + * > + ******************************************************************************/ > + > +struct acpi_table_cedt { > + struct acpi_table_header header; /* Common ACPI table header */ > + u32 reserved; > +}; > + > +/* Values for CEDT structure types */ > + > +enum acpi_cedt_type { > + ACPI_CEDT_TYPE_HOST_BRIDGE = 0, /* CHBS - CXL Host Bridge Structure */ > + ACPI_CEDT_TYPE_CFMWS = 1, /* CFMWS - CXL Fixed Memory Window Structure */ > +}; > + > +struct acpi_cedt_structure { > + u8 type; > + u8 reserved; > + u16 length; > +}; > + > +/* > + * CEDT Structures, correspond to Type in struct acpi_cedt_structure > + */ > + > +/* 0: CXL Host Bridge Structure */ > + > +struct acpi_cedt_chbs { > + struct acpi_cedt_structure header; > + u32 uid; > + u32 version; > + u32 reserved1; > + u64 base; > + u32 length; > + u32 reserved2; > +}; > + > +/* Values for version field above */ > + > +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0) > +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1) > + > +/* Values for length field above */ > + > +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000) > +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000) > + > /* Reset to default packing */ > > #pragma pack() > -- > 2.29.2 >
On Tue, Nov 17, 2020 at 6:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote: [..] > > +static struct acpi_driver acpi_cxl_driver = { > > First of all, no new acpi_driver instances, pretty please! > > acpi_default_enumeration() should create a platform device with the > ACPI0017 ID for you. Can't you provide a driver for this one? > Ah, yes, I recall we had this discussion around the time the ACPI0012 NFIT driver was developed. IIRC the reason ACPI0012 remaining an acpi_driver was due to a need to handle ACPI notifications, is that the deciding factor? ACPI0017 does not have any notifications so it seems like platform driver is the way to go.
On Tue, Nov 17, 2020 at 10:45 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Tue, Nov 17, 2020 at 6:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > [..] > > > +static struct acpi_driver acpi_cxl_driver = { > > > > First of all, no new acpi_driver instances, pretty please! > > > > acpi_default_enumeration() should create a platform device with the > > ACPI0017 ID for you. Can't you provide a driver for this one? > > > > Ah, yes, I recall we had this discussion around the time the ACPI0012 > NFIT driver was developed. IIRC the reason ACPI0012 remaining an > acpi_driver was due to a need to handle ACPI notifications, is that > the deciding factor? Sort of. In fact, a platform device driver can still handle ACPI notifications just fine, it just needs to install a notify handler for that. The cases when an acpi_driver is really needed are basically when creating the platform device during the enumeration is not desirable, like in the PCI or PNP cases (because they both create device objects of a different type to represent the "physical" device). It doesn't look like it is really needed for ACPI0012, but since it is there already, well ... > ACPI0017 does not have any notifications so it seems like platform driver is the way to go. Indeed.
diff --git a/drivers/Kconfig b/drivers/Kconfig index dcecc9f6e33f..62c753a73651 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -6,6 +6,7 @@ menu "Device Drivers" source "drivers/amba/Kconfig" source "drivers/eisa/Kconfig" source "drivers/pci/Kconfig" +source "drivers/cxl/Kconfig" source "drivers/pcmcia/Kconfig" source "drivers/rapidio/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index c0cd1b9075e3..5dad349de73b 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -73,6 +73,7 @@ obj-$(CONFIG_NVM) += lightnvm/ obj-y += base/ block/ misc/ mfd/ nfc/ obj-$(CONFIG_LIBNVDIMM) += nvdimm/ obj-$(CONFIG_DAX) += dax/ +obj-$(CONFIG_CXL_BUS) += cxl/ obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/ obj-$(CONFIG_NUBUS) += nubus/ obj-y += macintosh/ diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig new file mode 100644 index 000000000000..dd724bd364df --- /dev/null +++ b/drivers/cxl/Kconfig @@ -0,0 +1,30 @@ +# SPDX-License-Identifier: GPL-2.0-only +menuconfig CXL_BUS + tristate "CXL (Compute Express Link) Devices Support" + help + CXL is a bus that is electrically compatible with PCI-E, but layers + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The + CXL.cache protocol allows devices to hold cachelines locally, the + CXL.mem protocol allows devices to be fully coherent memory targets, the + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for + the configuration and management of devices supporting these protocols. + +if CXL_BUS + +config CXL_BUS_PROVIDER + tristate + +config CXL_ACPI + tristate "CXL Platform Support" + depends on ACPI + default CXL_BUS + select CXL_BUS_PROVIDER + help + CXL Platform Support is a prerequisite for any CXL device driver that + wants to claim ownership of the component register space. By default + platform firmware assumes Linux is unaware of CXL capabilities and + requires explicit opt-in. This platform component also mediates + resources described by the CEDT (CXL Early Discovery Table) + + Say 'y' to enable CXL (Compute Express Link) drivers. +endif diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile new file mode 100644 index 000000000000..d38cd34a2582 --- /dev/null +++ b/drivers/cxl/Makefile @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o + +ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL +cxl_acpi-y := acpi.o diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c new file mode 100644 index 000000000000..26e4f73838a7 --- /dev/null +++ b/drivers/cxl/acpi.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright(c) 2020 Intel Corporation. All rights reserved. + */ +#include <linux/list_sort.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/sysfs.h> +#include <linux/list.h> +#include <linux/acpi.h> +#include <linux/sort.h> +#include <linux/pci.h> +#include "acpi.h" + +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev) +{ + dev_set_drvdata(dev, acpi_desc); + acpi_desc->dev = dev; +} + +static void acpi_cedt_put_table(void *table) +{ + acpi_put_table(table); +} + +static int acpi_cxl_add(struct acpi_device *adev) +{ + struct acpi_cxl_desc *acpi_desc; + struct device *dev = &adev->dev; + struct acpi_table_header *tbl; + acpi_status status = AE_OK; + acpi_size sz; + int rc = 0; + + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl); + if (ACPI_FAILURE(status)) { + dev_err(dev, "failed to find CEDT at startup\n"); + return 0; + } + + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl); + if (rc) + return rc; + sz = tbl->length; + dev_info(dev, "found CEDT at startup: %lld bytes\n", sz); + + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); + if (!acpi_desc) + return -ENOMEM; + acpi_cxl_desc_init(acpi_desc, &adev->dev); + + acpi_desc->acpi_header = *tbl; + + return 0; +} + +static int acpi_cxl_remove(struct acpi_device *adev) +{ + return 0; +} + +static const struct acpi_device_id acpi_cxl_ids[] = { + { "ACPI0017", 0 }, + { "", 0 }, +}; +MODULE_DEVICE_TABLE(acpi, acpi_cxl_ids); + +static struct acpi_driver acpi_cxl_driver = { + .name = KBUILD_MODNAME, + .ids = acpi_cxl_ids, + .ops = { + .add = acpi_cxl_add, + .remove = acpi_cxl_remove, + }, +}; + +/* + * If/when CXL support is defined by other platform firmware the kernel + * will need a mechanism to select between the platform specific version + * of this routine, until then, hard-code ACPI assumptions + */ +int cxl_bus_prepared(struct pci_dev *pdev) +{ + struct acpi_device *adev; + struct pci_dev *root_port; + struct device *root; + + root_port = pcie_find_root_port(pdev); + if (!root_port) + return -ENXIO; + + root = root_port->dev.parent; + if (!root) + return -ENXIO; + + adev = ACPI_COMPANION(root); + if (!adev) + return -ENXIO; + + /* TODO: OSC enabling */ + + return 0; +} +EXPORT_SYMBOL_GPL(cxl_bus_prepared); + +static __init int acpi_cxl_init(void) +{ + return acpi_bus_register_driver(&acpi_cxl_driver); +} + +static __exit void acpi_cxl_exit(void) +{ + acpi_bus_unregister_driver(&acpi_cxl_driver); +} + +module_init(acpi_cxl_init); +module_exit(acpi_cxl_exit); +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Intel Corporation"); diff --git a/drivers/cxl/acpi.h b/drivers/cxl/acpi.h new file mode 100644 index 000000000000..011505475cc6 --- /dev/null +++ b/drivers/cxl/acpi.h @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright(c) 2020 Intel Corporation. All rights reserved. + +#ifndef __CXL_ACPI_H__ +#define __CXL_ACPI_H__ +#include <linux/acpi.h> + +struct acpi_cxl_desc { + struct acpi_table_header acpi_header; + struct device *dev; +}; + +int cxl_bus_prepared(struct pci_dev *pci_dev); + +#endif /* __CXL_ACPI_H__ */ diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index 43549547ed3e..70f745f526e3 100644 --- a/include/acpi/actbl1.h +++ b/include/acpi/actbl1.h @@ -28,6 +28,7 @@ #define ACPI_SIG_BERT "BERT" /* Boot Error Record Table */ #define ACPI_SIG_BGRT "BGRT" /* Boot Graphics Resource Table */ #define ACPI_SIG_BOOT "BOOT" /* Simple Boot Flag Table */ +#define ACPI_SIG_CEDT "CEDT" /* CXL Early Discovery Table */ #define ACPI_SIG_CPEP "CPEP" /* Corrected Platform Error Polling table */ #define ACPI_SIG_CSRT "CSRT" /* Core System Resource Table */ #define ACPI_SIG_DBG2 "DBG2" /* Debug Port table type 2 */ @@ -1624,6 +1625,57 @@ struct acpi_ibft_target { u16 reverse_chap_secret_offset; }; +/******************************************************************************* + * + * CEDT - CXL Early Discovery Table (ACPI 6.4) + * Version 1 + * + ******************************************************************************/ + +struct acpi_table_cedt { + struct acpi_table_header header; /* Common ACPI table header */ + u32 reserved; +}; + +/* Values for CEDT structure types */ + +enum acpi_cedt_type { + ACPI_CEDT_TYPE_HOST_BRIDGE = 0, /* CHBS - CXL Host Bridge Structure */ + ACPI_CEDT_TYPE_CFMWS = 1, /* CFMWS - CXL Fixed Memory Window Structure */ +}; + +struct acpi_cedt_structure { + u8 type; + u8 reserved; + u16 length; +}; + +/* + * CEDT Structures, correspond to Type in struct acpi_cedt_structure + */ + +/* 0: CXL Host Bridge Structure */ + +struct acpi_cedt_chbs { + struct acpi_cedt_structure header; + u32 uid; + u32 version; + u32 reserved1; + u64 base; + u32 length; + u32 reserved2; +}; + +/* Values for version field above */ + +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0) +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1) + +/* Values for length field above */ + +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000) +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000) + /* Reset to default packing */ #pragma pack()