Changes since v1 [1]:
- Make use pci_info() and FLAG(), as in pcie_init().
- Wrap new cxl_cap in pci_dev struct within #ifdef PCI_CXL.
(Bjorn Helgaas)
- Added functionality for some CXL.mem and CXL.cache helper functions.
Snoop filter helper functions along with a performance hint as
well as a toggle for viral self-isolation mode could be implemented.
However, in the absence of CXL devices with their associated drivers, it
perhaps makes best sense to be in a holding pattern on this until we have
something to exercise it with.
Thoughts?
[1] https://lore.kernel.org/linux-pci/[email protected]/
This patch series implements basic Designated Vendor-Specific Extended
Capabilities (DVSEC) decode for Compute eXpress Link devices, a new CPU
interconnect building upon PCIe. As a basis for the CXL support it provides
PCI init handling for detection, decode, and caching of CXL device
capabilities. Moreover, it makes use of the DVSEC Vendor ID and DVSEC ID so
as to identify a CXL capable device. (PCIe r5.0, sec 7.9.6.2)
DocLink: https://www.computeexpresslink.org/
For your reference, a parallel series of patches have been submitted to enable
lspci decode of CXL DVSEC and may be tracked.
Link: https://lore.kernel.org/linux-pci/[email protected]/
This patch makes use of pending DVSEC related header additions and the
first patch of that series is included here. It can be sorted out when the
upstream merge is done.
Link: https://lore.kernel.org/linux-pci/[email protected]/
Sample dmesg output of a CXL Type 3 device (CXL.io, CXL.mem):
[ 2.997177] pci 0000:6b:00.0: CXL: Cache- IO+ Mem+ Viral- HDMCount 1
[ 2.997188] pci 0000:6b:00.0: CXL: cap ctrl status ctrl2 status2 lock
[ 2.997201] pci 0000:6b:00.0: CXL: 001e 0002 0000 0000 0000 0000
David E. Box (1):
pci: Add Designated Vendor Specific Capability
Sean V Kelley (2):
PCI: Add basic Compute eXpress Link DVSEC decode
PCI: Add helpers to enable/disable CXL.mem and CXL.cache
drivers/pci/Kconfig | 9 ++
drivers/pci/Makefile | 1 +
drivers/pci/cxl.c | 175 ++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 15 +++
drivers/pci/probe.c | 1 +
include/linux/pci.h | 3 +
include/uapi/linux/pci_regs.h | 5 +
7 files changed, 209 insertions(+)
create mode 100644 drivers/pci/cxl.c
--
2.26.2
With these helpers, a device driver can enable/disable access to
CXL.mem and CXL.cache. Note that the device driver is responsible for
managing the memory area.
Signed-off-by: Sean V Kelley <[email protected]>
---
drivers/pci/cxl.c | 93 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/pci/pci.h | 8 ++++
2 files changed, 96 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
index 4437ca69ad33..0d0a1b82ea98 100644
--- a/drivers/pci/cxl.c
+++ b/drivers/pci/cxl.c
@@ -24,6 +24,88 @@
#define PCI_CXL_HDM_COUNT(reg) (((reg) & (3 << 4)) >> 4)
#define PCI_CXL_VIRAL BIT(14)
+#define PCI_CXL_CONFIG_LOCK BIT(0)
+
+static void pci_cxl_unlock(struct pci_dev *dev)
+{
+ int pos = dev->cxl_cap;
+ u16 lock;
+
+ pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
+ lock &= ~PCI_CXL_CONFIG_LOCK;
+ pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
+}
+
+static void pci_cxl_lock(struct pci_dev *dev)
+{
+ int pos = dev->cxl_cap;
+ u16 lock;
+
+ pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
+ lock |= PCI_CXL_CONFIG_LOCK;
+ pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
+}
+
+static int pci_cxl_enable_disable_feature(struct pci_dev *dev, int enable,
+ u16 feature)
+{
+ int pos = dev->cxl_cap;
+ int ret;
+ u16 reg;
+
+ if (!dev->cxl_cap)
+ return -EINVAL;
+
+ /* Only for PCIe */
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+
+ /* Only for Device 0 Function 0, Root Complex Integrated Endpoints */
+ if (dev->devfn != 0 || (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_END))
+ return -EINVAL;
+
+ pci_cxl_unlock(dev);
+ ret = pci_read_config_word(dev, pos + PCI_CXL_CTRL, ®);
+ if (ret)
+ goto lock;
+
+ if (enable)
+ reg |= feature;
+ else
+ reg &= ~feature;
+
+ ret = pci_write_config_word(dev, pos + PCI_CXL_CTRL, reg);
+
+lock:
+ pci_cxl_lock(dev);
+
+ return ret;
+}
+
+int pci_cxl_mem_enable(struct pci_dev *dev)
+{
+ return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_MEM);
+}
+EXPORT_SYMBOL_GPL(pci_cxl_mem_enable);
+
+void pci_cxl_mem_disable(struct pci_dev *dev)
+{
+ pci_cxl_enable_disable_feature(dev, false, PCI_CXL_MEM);
+}
+EXPORT_SYMBOL_GPL(pci_cxl_mem_disable);
+
+int pci_cxl_cache_enable(struct pci_dev *dev)
+{
+ return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_CACHE);
+}
+EXPORT_SYMBOL_GPL(pci_cxl_cache_enable);
+
+void pci_cxl_cache_disable(struct pci_dev *dev)
+{
+ pci_cxl_enable_disable_feature(dev, false, PCI_CXL_CACHE);
+}
+EXPORT_SYMBOL_GPL(pci_cxl_cache_disable);
+
/*
* pci_find_cxl_capability - Identify and return offset to Vendor-Specific
* capabilities.
@@ -73,11 +155,6 @@ void pci_cxl_init(struct pci_dev *dev)
dev->cxl_cap = pos;
pci_read_config_word(dev, pos + PCI_CXL_CAP, &cap);
- pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
- pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
- pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
- pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
- pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
pci_info(dev, "CXL: Cache%c IO%c Mem%c Viral%c HDMCount %d\n",
FLAG(cap, PCI_CXL_CACHE),
@@ -86,6 +163,12 @@ void pci_cxl_init(struct pci_dev *dev)
FLAG(cap, PCI_CXL_VIRAL),
PCI_CXL_HDM_COUNT(cap));
+ pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
+ pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
+ pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
+ pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
+ pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
+
pci_info(dev, "CXL: cap ctrl status ctrl2 status2 lock\n");
pci_info(dev, "CXL: %04x %04x %04x %04x %04x %04x\n",
cap, ctrl, status, ctrl2, status2, lock);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d9905e2dee95..6336e16565ac 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -472,8 +472,16 @@ static inline void pci_restore_ats_state(struct pci_dev *dev) { }
#ifdef CONFIG_PCI_CXL
/* Compute eXpress Link */
void pci_cxl_init(struct pci_dev *dev);
+int pci_cxl_mem_enable(struct pci_dev *dev);
+void pci_cxl_mem_disable(struct pci_dev *dev);
+int pci_cxl_cache_enable(struct pci_dev *dev);
+void pci_cxl_cache_disable(struct pci_dev *dev);
#else
static inline void pci_cxl_init(struct pci_dev *dev) { }
+static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
+static inline void pci_cxl_mem_disable(struct pci_dev *dev) {}
+static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
+static inline void pci_cxl_cache_disable(struct pci_dev *dev) {}
#endif
#ifdef CONFIG_PCI_PRI
--
2.26.2
Compute eXpress Link is a new CPU interconnect created with
workload accelerators in mind. The interconnect relies on PCIe Electrical
and Physical interconnect for communication. CXL devices enumerate to the
OS as an ACPI-described PCIe Root Complex Integrated Endpoint.
This patch introduces the bare minimum support by simply looking for and
caching the DVSEC CXL Extended Capability. Currently, only CXL.io (which
is mandatory to be configured by BIOS) is enabled. In future, we will
also add support for CXL.cache and CXL.mem.
DocLink: https://www.computeexpresslink.org/
Originally-by: Felipe Balbi <[email protected]>
Signed-off-by: Sean V Kelley <[email protected]>
---
drivers/pci/Kconfig | 9 +++++
drivers/pci/Makefile | 1 +
drivers/pci/cxl.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 7 ++++
drivers/pci/probe.c | 1 +
include/linux/pci.h | 3 ++
6 files changed, 113 insertions(+)
create mode 100644 drivers/pci/cxl.c
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 4bef5c2bae9f..eafb200b320b 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -115,6 +115,15 @@ config XEN_PCIDEV_FRONTEND
The PCI device frontend driver allows the kernel to import arbitrary
PCI devices from a PCI backend to support PCI driver domains.
+config PCI_CXL
+ bool "Enable PCI Compute eXpress Link"
+ depends on PCI
+ help
+ Say Y here if you want the PCI core to detect CXL devices, decode, and
+ cache the DVSEC CXL Extended Capability as configured by BIOS.
+
+ When in doubt, say N.
+
config PCI_ATS
bool
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 522d2b974e91..465eee31e999 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
obj-$(CONFIG_PCI_ECAM) += ecam.o
obj-$(CONFIG_PCI_P2PDMA) += p2pdma.o
obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
+obj-$(CONFIG_PCI_CXL) += cxl.o
# Endpoint library must be initialized before its users
obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
new file mode 100644
index 000000000000..4437ca69ad33
--- /dev/null
+++ b/drivers/pci/cxl.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Compute eXpress Link Support
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/pci_regs.h>
+
+#define PCI_DVSEC_VENDOR_ID_CXL 0x1e98
+#define PCI_DVSEC_ID_CXL_DEV 0x0
+
+#define PCI_CXL_CAP 0x0a
+#define PCI_CXL_CTRL 0x0c
+#define PCI_CXL_STS 0x0e
+#define PCI_CXL_CTRL2 0x10
+#define PCI_CXL_STS2 0x12
+#define PCI_CXL_LOCK 0x14
+
+#define PCI_CXL_CACHE BIT(0)
+#define PCI_CXL_IO BIT(1)
+#define PCI_CXL_MEM BIT(2)
+#define PCI_CXL_HDM_COUNT(reg) (((reg) & (3 << 4)) >> 4)
+#define PCI_CXL_VIRAL BIT(14)
+
+/*
+ * pci_find_cxl_capability - Identify and return offset to Vendor-Specific
+ * capabilities.
+ *
+ * CXL makes use of Designated Vendor-Specific Extended Capability (DVSEC)
+ * to uniquely identify both DVSEC Vendor ID and DVSEC ID aligning with
+ * PCIe r5.0, sec 7.9.6.2
+ */
+static int pci_find_cxl_capability(struct pci_dev *dev)
+{
+ u16 vendor, id;
+ int pos = 0;
+
+ while ((pos = pci_find_next_ext_capability(dev, pos,
+ PCI_EXT_CAP_ID_DVSEC))) {
+ pci_read_config_word(dev, pos + PCI_DVSEC_HEADER1,
+ &vendor);
+ pci_read_config_word(dev, pos + PCI_DVSEC_HEADER2, &id);
+ if (vendor == PCI_DVSEC_VENDOR_ID_CXL &&
+ id == PCI_DVSEC_ID_CXL_DEV)
+ return pos;
+ }
+
+ return 0;
+}
+
+
+#define FLAG(x, y) (((x) & (y)) ? '+' : '-')
+
+void pci_cxl_init(struct pci_dev *dev)
+{
+ u16 cap, ctrl, status, ctrl2, status2, lock;
+ int pos;
+
+ /* Only for PCIe */
+ if (!pci_is_pcie(dev))
+ return;
+
+ /* Only for Device 0 Function 0, Root Complex Integrated Endpoints */
+ if (dev->devfn != 0 || (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_END))
+ return;
+
+ pos = pci_find_cxl_capability(dev);
+ if (!pos)
+ return;
+
+ dev->cxl_cap = pos;
+
+ pci_read_config_word(dev, pos + PCI_CXL_CAP, &cap);
+ pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
+ pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
+ pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
+ pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
+ pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
+
+ pci_info(dev, "CXL: Cache%c IO%c Mem%c Viral%c HDMCount %d\n",
+ FLAG(cap, PCI_CXL_CACHE),
+ FLAG(cap, PCI_CXL_IO),
+ FLAG(cap, PCI_CXL_MEM),
+ FLAG(cap, PCI_CXL_VIRAL),
+ PCI_CXL_HDM_COUNT(cap));
+
+ pci_info(dev, "CXL: cap ctrl status ctrl2 status2 lock\n");
+ pci_info(dev, "CXL: %04x %04x %04x %04x %04x %04x\n",
+ cap, ctrl, status, ctrl2, status2, lock);
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f75867106..d9905e2dee95 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -469,6 +469,13 @@ static inline void pci_ats_init(struct pci_dev *d) { }
static inline void pci_restore_ats_state(struct pci_dev *dev) { }
#endif /* CONFIG_PCI_ATS */
+#ifdef CONFIG_PCI_CXL
+/* Compute eXpress Link */
+void pci_cxl_init(struct pci_dev *dev);
+#else
+static inline void pci_cxl_init(struct pci_dev *dev) { }
+#endif
+
#ifdef CONFIG_PCI_PRI
void pci_pri_init(struct pci_dev *dev);
void pci_restore_pri_state(struct pci_dev *pdev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 77b8a145c39b..c55df0ae8f06 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2371,6 +2371,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
pci_ptm_init(dev); /* Precision Time Measurement */
pci_aer_init(dev); /* Advanced Error Reporting */
pci_dpc_init(dev); /* Downstream Port Containment */
+ pci_cxl_init(dev); /* Compute eXpress Link */
pcie_report_downtraining(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 83ce1cdf5676..9fd544f76447 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -314,6 +314,9 @@ struct pci_dev {
#ifdef CONFIG_PCIEAER
u16 aer_cap; /* AER capability offset */
struct aer_stats *aer_stats; /* AER stats for this device */
+#endif
+#ifdef CONFIG_PCI_CXL
+ u16 cxl_cap; /* CXL capability offset */
#endif
u8 pcie_cap; /* PCIe capability offset */
u8 msi_cap; /* MSI capability offset */
--
2.26.2
On Mon, May 18, 2020 at 09:35:20AM -0700, Sean V Kelley wrote:
> Changes since v1 [1]:
>
> - Make use pci_info() and FLAG(), as in pcie_init().
> - Wrap new cxl_cap in pci_dev struct within #ifdef PCI_CXL.
> (Bjorn Helgaas)
>
> - Added functionality for some CXL.mem and CXL.cache helper functions.
> Snoop filter helper functions along with a performance hint as
> well as a toggle for viral self-isolation mode could be implemented.
> However, in the absence of CXL devices with their associated drivers, it
> perhaps makes best sense to be in a holding pattern on this until we have
> something to exercise it with.
Right, until these add functionality we can actually use, e.g.,
drivers that call these new functions, I think we're in a holding
pattern.
Bjorn
On Mon, May 18, 2020 at 09:35:23AM -0700, Sean V Kelley wrote:
> With these helpers, a device driver can enable/disable access to
> CXL.mem and CXL.cache. Note that the device driver is responsible for
> managing the memory area.
>
> Signed-off-by: Sean V Kelley <[email protected]>
> ---
> drivers/pci/cxl.c | 93 ++++++++++++++++++++++++++++++++++++++++++++---
> drivers/pci/pci.h | 8 ++++
> 2 files changed, 96 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index 4437ca69ad33..0d0a1b82ea98 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c
> @@ -24,6 +24,88 @@
> #define PCI_CXL_HDM_COUNT(reg) (((reg) & (3 << 4)) >> 4)
> #define PCI_CXL_VIRAL BIT(14)
>
> +#define PCI_CXL_CONFIG_LOCK BIT(0)
> +
> +static void pci_cxl_unlock(struct pci_dev *dev)
> +{
> + int pos = dev->cxl_cap;
> + u16 lock;
> +
> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
> + lock &= ~PCI_CXL_CONFIG_LOCK;
> + pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
> +}
> +
> +static void pci_cxl_lock(struct pci_dev *dev)
> +{
> + int pos = dev->cxl_cap;
> + u16 lock;
> +
> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
> + lock |= PCI_CXL_CONFIG_LOCK;
> + pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
> +}
Maybe a tiny comment about what PCI_CXL_CONFIG_LOCK does? I guess
these functions don't enforce mutual exclusion (as I first expected a
"lock" function to do); they're simply telling the device to accept
CTRL changes or something?
> +static int pci_cxl_enable_disable_feature(struct pci_dev *dev, int enable,
> + u16 feature)
> +{
> + int pos = dev->cxl_cap;
> + int ret;
> + u16 reg;
> +
> + if (!dev->cxl_cap)
> + return -EINVAL;
"pos"
"pos" is the typical name for things like this, but sometimes I think
the code would be more descriptive if we used things like "cxl",
"aer", etc.
> +
> + /* Only for PCIe */
> + if (!pci_is_pcie(dev))
> + return -EINVAL;
Probably not necessary here. We already checked it in pci_cxi_init().
> + /* Only for Device 0 Function 0, Root Complex Integrated Endpoints */
> + if (dev->devfn != 0 || (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_END))
> + return -EINVAL;
> +
> + pci_cxl_unlock(dev);
> + ret = pci_read_config_word(dev, pos + PCI_CXL_CTRL, ®);
> + if (ret)
> + goto lock;
> +
> + if (enable)
> + reg |= feature;
> + else
> + reg &= ~feature;
> +
> + ret = pci_write_config_word(dev, pos + PCI_CXL_CTRL, reg);
> +
> +lock:
> + pci_cxl_lock(dev);
> +
> + return ret;
> +}
> +
> +int pci_cxl_mem_enable(struct pci_dev *dev)
> +{
> + return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_MEM);
> +}
> +EXPORT_SYMBOL_GPL(pci_cxl_mem_enable);
> +
> +void pci_cxl_mem_disable(struct pci_dev *dev)
> +{
> + pci_cxl_enable_disable_feature(dev, false, PCI_CXL_MEM);
> +}
> +EXPORT_SYMBOL_GPL(pci_cxl_mem_disable);
> +
> +int pci_cxl_cache_enable(struct pci_dev *dev)
> +{
> + return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_CACHE);
> +}
> +EXPORT_SYMBOL_GPL(pci_cxl_cache_enable);
> +
> +void pci_cxl_cache_disable(struct pci_dev *dev)
> +{
> + pci_cxl_enable_disable_feature(dev, false, PCI_CXL_CACHE);
> +}
> +EXPORT_SYMBOL_GPL(pci_cxl_cache_disable);
> +
> /*
> * pci_find_cxl_capability - Identify and return offset to Vendor-Specific
> * capabilities.
> @@ -73,11 +155,6 @@ void pci_cxl_init(struct pci_dev *dev)
> dev->cxl_cap = pos;
>
> pci_read_config_word(dev, pos + PCI_CXL_CAP, &cap);
> - pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
> - pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
> - pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
> - pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
> - pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
>
> pci_info(dev, "CXL: Cache%c IO%c Mem%c Viral%c HDMCount %d\n",
> FLAG(cap, PCI_CXL_CACHE),
> @@ -86,6 +163,12 @@ void pci_cxl_init(struct pci_dev *dev)
> FLAG(cap, PCI_CXL_VIRAL),
> PCI_CXL_HDM_COUNT(cap));
>
> + pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
> + pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
> + pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
> + pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
This looks like it's just moving these reads around and isn't actually
related to this patch. Put them where you want them in the first
place so the move doesn't clutter this patch.
> pci_info(dev, "CXL: cap ctrl status ctrl2 status2 lock\n");
> pci_info(dev, "CXL: %04x %04x %04x %04x %04x %04x\n",
> cap, ctrl, status, ctrl2, status2, lock);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d9905e2dee95..6336e16565ac 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -472,8 +472,16 @@ static inline void pci_restore_ats_state(struct pci_dev *dev) { }
> #ifdef CONFIG_PCI_CXL
> /* Compute eXpress Link */
> void pci_cxl_init(struct pci_dev *dev);
> +int pci_cxl_mem_enable(struct pci_dev *dev);
> +void pci_cxl_mem_disable(struct pci_dev *dev);
> +int pci_cxl_cache_enable(struct pci_dev *dev);
> +void pci_cxl_cache_disable(struct pci_dev *dev);
> #else
> static inline void pci_cxl_init(struct pci_dev *dev) { }
> +static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
> +static inline void pci_cxl_mem_disable(struct pci_dev *dev) {}
> +static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
> +static inline void pci_cxl_cache_disable(struct pci_dev *dev) {}
The stubs in this file aren't completely consistent, but you can be at
least locally consistent by including a space between the "{ }".
> #endif
>
> #ifdef CONFIG_PCI_PRI
> --
> 2.26.2
>
Hi Sean,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pci/next]
[also build test WARNING on v5.7-rc6 next-20200518]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sean-V-Kelley/PCI-Add-basic-Compute-eXpress-Link-DVSEC-decode/20200519-004029
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arc-defconfig (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
In file included from drivers/pci/of.c:16:
drivers/pci/pci.h: In function 'pci_cxl_mem_enable':
>> drivers/pci/pci.h:481:45: warning: no return statement in function returning non-void [-Wreturn-type]
481 | static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
| ^~~~~~~
drivers/pci/pci.h: In function 'pci_cxl_cache_enable':
drivers/pci/pci.h:483:47: warning: no return statement in function returning non-void [-Wreturn-type]
483 | static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
| ^~~~~~~
vim +481 drivers/pci/pci.h
471
472 #ifdef CONFIG_PCI_CXL
473 /* Compute eXpress Link */
474 void pci_cxl_init(struct pci_dev *dev);
475 int pci_cxl_mem_enable(struct pci_dev *dev);
476 void pci_cxl_mem_disable(struct pci_dev *dev);
477 int pci_cxl_cache_enable(struct pci_dev *dev);
478 void pci_cxl_cache_disable(struct pci_dev *dev);
479 #else
480 static inline void pci_cxl_init(struct pci_dev *dev) { }
> 481 static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
482 static inline void pci_cxl_mem_disable(struct pci_dev *dev) {}
483 static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
484 static inline void pci_cxl_cache_disable(struct pci_dev *dev) {}
485 #endif
486
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Sean,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pci/next]
[also build test WARNING on v5.7-rc6 next-20200518]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sean-V-Kelley/PCI-Add-basic-Compute-eXpress-Link-DVSEC-decode/20200519-004029
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
In file included from drivers/pci/hotplug/pci_hotplug_core.c:32:
drivers/pci/hotplug/../pci.h: In function 'pci_cxl_mem_enable':
>> drivers/pci/hotplug/../pci.h:481:45: warning: no return statement in function returning non-void [-Wreturn-type]
481 | static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
| ^~~~~~~
drivers/pci/hotplug/../pci.h: In function 'pci_cxl_cache_enable':
drivers/pci/hotplug/../pci.h:483:47: warning: no return statement in function returning non-void [-Wreturn-type]
483 | static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
| ^~~~~~~
vim +481 drivers/pci/hotplug/../pci.h
471
472 #ifdef CONFIG_PCI_CXL
473 /* Compute eXpress Link */
474 void pci_cxl_init(struct pci_dev *dev);
475 int pci_cxl_mem_enable(struct pci_dev *dev);
476 void pci_cxl_mem_disable(struct pci_dev *dev);
477 int pci_cxl_cache_enable(struct pci_dev *dev);
478 void pci_cxl_cache_disable(struct pci_dev *dev);
479 #else
480 static inline void pci_cxl_init(struct pci_dev *dev) { }
> 481 static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
482 static inline void pci_cxl_mem_disable(struct pci_dev *dev) {}
483 static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
484 static inline void pci_cxl_cache_disable(struct pci_dev *dev) {}
485 #endif
486
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 18 May 2020, at 9:55, Bjorn Helgaas wrote:
> On Mon, May 18, 2020 at 09:35:23AM -0700, Sean V Kelley wrote:
>> With these helpers, a device driver can enable/disable access to
>> CXL.mem and CXL.cache. Note that the device driver is responsible for
>> managing the memory area.
>>
>> Signed-off-by: Sean V Kelley <[email protected]>
>> ---
>> drivers/pci/cxl.c | 93
>> ++++++++++++++++++++++++++++++++++++++++++++---
>> drivers/pci/pci.h | 8 ++++
>> 2 files changed, 96 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
>> index 4437ca69ad33..0d0a1b82ea98 100644
>> --- a/drivers/pci/cxl.c
>> +++ b/drivers/pci/cxl.c
>> @@ -24,6 +24,88 @@
>> #define PCI_CXL_HDM_COUNT(reg) (((reg) & (3 << 4)) >> 4)
>> #define PCI_CXL_VIRAL BIT(14)
>>
>> +#define PCI_CXL_CONFIG_LOCK BIT(0)
>> +
>> +static void pci_cxl_unlock(struct pci_dev *dev)
>> +{
>> + int pos = dev->cxl_cap;
>> + u16 lock;
>> +
>> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
>> + lock &= ~PCI_CXL_CONFIG_LOCK;
>> + pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
>> +}
>> +
>> +static void pci_cxl_lock(struct pci_dev *dev)
>> +{
>> + int pos = dev->cxl_cap;
>> + u16 lock;
>> +
>> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
>> + lock |= PCI_CXL_CONFIG_LOCK;
>> + pci_write_config_word(dev, pos + PCI_CXL_LOCK, lock);
>> +}
>
> Maybe a tiny comment about what PCI_CXL_CONFIG_LOCK does? I guess
> these functions don't enforce mutual exclusion (as I first expected a
> "lock" function to do); they're simply telling the device to accept
> CTRL changes or something?
Agree will add. DVSEC Lock is intended to signify whether the CTRL bits
may be changed.
It locks certain registers and makes them RO. This lock is designed to
prevent future
changes to configuration. Not to be confused with enforcing mutual
exclusion.
>
>> +static int pci_cxl_enable_disable_feature(struct pci_dev *dev, int
>> enable,
>> + u16 feature)
>> +{
>> + int pos = dev->cxl_cap;
>> + int ret;
>> + u16 reg;
>> +
>> + if (!dev->cxl_cap)
>> + return -EINVAL;
>
> "pos"
>
> "pos" is the typical name for things like this, but sometimes I think
> the code would be more descriptive if we used things like "cxl",
> "aer", etc.
Will do, makes sense.
>
>> +
>> + /* Only for PCIe */
>> + if (!pci_is_pcie(dev))
>> + return -EINVAL;
>
> Probably not necessary here. We already checked it in pci_cxi_init().
You are correct. Will remove.
>
>> + /* Only for Device 0 Function 0, Root Complex Integrated Endpoints
>> */
>> + if (dev->devfn != 0 || (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_END))
>> + return -EINVAL;
>> +
>> + pci_cxl_unlock(dev);
>> + ret = pci_read_config_word(dev, pos + PCI_CXL_CTRL, ®);
>> + if (ret)
>> + goto lock;
>> +
>> + if (enable)
>> + reg |= feature;
>> + else
>> + reg &= ~feature;
>> +
>> + ret = pci_write_config_word(dev, pos + PCI_CXL_CTRL, reg);
>> +
>> +lock:
>> + pci_cxl_lock(dev);
>> +
>> + return ret;
>> +}
>> +
>> +int pci_cxl_mem_enable(struct pci_dev *dev)
>> +{
>> + return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_MEM);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_cxl_mem_enable);
>> +
>> +void pci_cxl_mem_disable(struct pci_dev *dev)
>> +{
>> + pci_cxl_enable_disable_feature(dev, false, PCI_CXL_MEM);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_cxl_mem_disable);
>> +
>> +int pci_cxl_cache_enable(struct pci_dev *dev)
>> +{
>> + return pci_cxl_enable_disable_feature(dev, true, PCI_CXL_CACHE);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_cxl_cache_enable);
>> +
>> +void pci_cxl_cache_disable(struct pci_dev *dev)
>> +{
>> + pci_cxl_enable_disable_feature(dev, false, PCI_CXL_CACHE);
>> +}
>> +EXPORT_SYMBOL_GPL(pci_cxl_cache_disable);
>> +
>> /*
>> * pci_find_cxl_capability - Identify and return offset to
>> Vendor-Specific
>> * capabilities.
>> @@ -73,11 +155,6 @@ void pci_cxl_init(struct pci_dev *dev)
>> dev->cxl_cap = pos;
>>
>> pci_read_config_word(dev, pos + PCI_CXL_CAP, &cap);
>> - pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
>> - pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
>> - pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
>> - pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
>> - pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
>>
>> pci_info(dev, "CXL: Cache%c IO%c Mem%c Viral%c HDMCount %d\n",
>> FLAG(cap, PCI_CXL_CACHE),
>> @@ -86,6 +163,12 @@ void pci_cxl_init(struct pci_dev *dev)
>> FLAG(cap, PCI_CXL_VIRAL),
>> PCI_CXL_HDM_COUNT(cap));
>>
>> + pci_read_config_word(dev, pos + PCI_CXL_CTRL, &ctrl);
>> + pci_read_config_word(dev, pos + PCI_CXL_STS, &status);
>> + pci_read_config_word(dev, pos + PCI_CXL_CTRL2, &ctrl2);
>> + pci_read_config_word(dev, pos + PCI_CXL_STS2, &status2);
>> + pci_read_config_word(dev, pos + PCI_CXL_LOCK, &lock);
>
> This looks like it's just moving these reads around and isn't actually
> related to this patch. Put them where you want them in the first
> place so the move doesn't clutter this patch.
Looks that way. I will clean it up.
>
>> pci_info(dev, "CXL: cap ctrl status ctrl2 status2 lock\n");
>> pci_info(dev, "CXL: %04x %04x %04x %04x %04x %04x\n",
>> cap, ctrl, status, ctrl2, status2, lock);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index d9905e2dee95..6336e16565ac 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -472,8 +472,16 @@ static inline void pci_restore_ats_state(struct
>> pci_dev *dev) { }
>> #ifdef CONFIG_PCI_CXL
>> /* Compute eXpress Link */
>> void pci_cxl_init(struct pci_dev *dev);
>> +int pci_cxl_mem_enable(struct pci_dev *dev);
>> +void pci_cxl_mem_disable(struct pci_dev *dev);
>> +int pci_cxl_cache_enable(struct pci_dev *dev);
>> +void pci_cxl_cache_disable(struct pci_dev *dev);
>> #else
>> static inline void pci_cxl_init(struct pci_dev *dev) { }
>> +static inline int pci_cxl_mem_enable(struct pci_dev *dev) {}
>> +static inline void pci_cxl_mem_disable(struct pci_dev *dev) {}
>> +static inline int pci_cxl_cache_enable(struct pci_dev *dev) {}
>> +static inline void pci_cxl_cache_disable(struct pci_dev *dev) {}
>
> The stubs in this file aren't completely consistent, but you can be at
> least locally consistent by including a space between the "{ }".
Will do.
Thanks,
Sean
>
>> #endif
>>
>> #ifdef CONFIG_PCI_PRI
>> --
>> 2.26.2
>>
On Mon, May 18, 2020 at 09:35:23AM -0700, Sean V Kelley wrote:
> With these helpers, a device driver can enable/disable access to
> CXL.mem and CXL.cache. Note that the device driver is responsible for
> managing the memory area.
Who is going to call these? Please don't submit new APIs without users.