[RFC PATCH 00/13] RFC: add Type2 device support

[RFC PATCH 00/13] RFC: add Type2 device support

[alucerop]

From: Alejandro Lucero <alejandro.lucero-palau@amd.com>

I need to start this RFC explaining not what the patchset does, but what we
expected to obtain and currently is under doubt: to configure a CXL memory
range advertised by our CXL network device and to use it "privately".

The main reason behind that privacy, but not the only one, is to avoid any
arbitrary use by any user-space "client" with enough privileges for using a
public interface to map the device memory and use it as regular memory. The
reason is obvious: the device expects writes to such a memory in a specific
format.

The doubt comes from the fact that after implementing the functionality exposed
in this patchset, we realized the current expectation seems to be a BIOS/UEFI
configuring HDM decoders or HPA ranges and passing memory ranges to the kernel,
and with the kernel having a default action on that memory range based on the
flag EFI_MEMORY_SP being set or not. If it is not set, the memory range will
be part of the kernel memory management, what we do not want for sure. If the
flag is set, a DAX device will be created which allows an user-space client to
map such a memory through the DAX device API, what we also prefer to avoid. I
know this is likely going to face opposition, but we see this RFC as the
opportunity for discussing the matter and, if it turns out to be the case, to be
guided towards the proper solution accepted by the maintainers/community. This
patchset does not tackle this default kernel behaviour although we already have
some ideas and workarounds for the short term. We'll be happy to discuss this
openly.

So, this patchset assumes a BIOS/UEFI not programming the HDM decoder or HPA
ranges for a CXL Type2 device. Although maybe a weird assumption as explained
above, a Type2 device added after boot, that is hotplugged, will likely need the
changes added here. Exporting some of the CXL core for vendor drivers is also
required for avoiding code duplication when reading DVSEC or decoder registers.
Finally if there is no such HPA range assigned, whatever the reason, this patchset
offers some way of obtaining one and/or finding out what is the problem behind the
lack of such HPA range.

Current CXL kernel code is focused on supporting Type3 CXL devices, aka memory
expanders. Type2 CXL devices, aka device accelerators, share some functionalities
but require some special handling.

First of all, Type2 are by definition specific to vendor designs and a specific
vendor driver is expected to work with the CXL specifics. This implies the CXL
setup needs to be done by such a driver instead of by a generic CXL PCI driver
as for memory expanders. Most of such setup needs to use current CXL core code
and therefore needs to be accessible to those vendor drivers. This is
accomplished with the first patch and with extensions to the exported functions
in subsequent patches.

Keeping with kernel rules of having a client using any new functionality added,
a CXL type2 driver is increasingly added. This is not a driver supporting a real
device but an emulated one in QEMU, with the QEMU patch following this patchset.

The reason for adding such a Type2 driver instead of changes to a current kernel
driver or a new driver is threefold:

1) the expected kernel driver to use the functionality added is a netdev one.
   Current internal CXL support is a codesign effort, therefore software and
   hardware evolving in lockstep. Adding changes to a netdev driver requires the
   full functionality and doing things following the netdev standard which is
   not the best option for this development stage.

2) Waiting for completing the development will delay the required Type2 support,
   and most of the required changes are unrelated to specific CXL usage by any
   vendor driver.

3) Type2 support will need some testing infrastructure, unit tests for ensuring
   Type2 devices are working, and module tests for ensuring CXL core changes do
   not affect Type2 support.

I hope these reasons are convincing enough.

I have decided to follow a gradual approach for adding such a driver using the
exported CXL functions and structs. I think it is easier to review the driver
when the new funcionality is added than to add the driver at the end, but not a
big deal if my approach is not liked.

The patches are based on a patchset sent by Dan Williams [1] which was just
partially integrated, most related to making things ready for Type2 but none
related to specific Type2 support. Those patches based on Dan´s work have Dan´s
signing, so Dan, tell me if you do not want me to add you.

Type2 implies, I think, only the related driver to manage the CXL specifics.
This means no user space intervention and therefore no sysfs files. This makes
easy to avoid the current problem of most of the sysfs related code expecting
Type3 devices. If I´m wrong in this regard, such a code will need further
changes.

A final note about CXL.cache is needed. This patchset does not cover it at all,
although the emulated Type2 device advertises it. From the kernel point of view
supporting CXL.cache will imply to be sure the CXL path supports what the Type2
device needs. A device accelerator will likely be connected to a Root Switch,
but other configurations can not be discarded. Therefore the kernel will need to
check not just HPA, DPA, interleave and granularity, but also the available
CXL.cache support and resources in each switch in the CXL path to the Type2
device. I expect to contribute to this support in the following months, and
it would be good to discuss about it when possible.

Alejandro.

[1] https://lore.kernel.org/linux-cxl/98b1f61a-e6c2-71d4-c368-50d958501b0c@intel.com/T/

Alejandro Lucero (13):
  cxl: move header files for absolute references
  cxl: add type2 device basic support
  cxl: export core function for type2 devices
  cxl: allow devices without mailbox capability
  cxl: fix check about pmem resource
  cxl: support type2 memdev creation
  cxl: add functions for exclusive access to endpoint port topology
  cxl: add cxl_get_hpa_freespace
  cxl: add cxl_request_dpa
  cxl: make region type based on endpoint type
  cxl: allow automatic region creation by type2 drivers
  cxl: preclude device memory to be used for dax
  cxl: test type2 private mapping

 drivers/cxl/acpi.c                      |   4 +-
 drivers/cxl/core/cdat.c                 |   9 +-
 drivers/cxl/core/core.h                 |   1 -
 drivers/cxl/core/hdm.c                  | 159 +++++++--
 drivers/cxl/core/mbox.c                 |   6 +-
 drivers/cxl/core/memdev.c               |  75 +++-
 drivers/cxl/core/pci.c                  |   6 +-
 drivers/cxl/core/pmem.c                 |   4 +-
 drivers/cxl/core/pmu.c                  |   4 +-
 drivers/cxl/core/port.c                 |   6 +-
 drivers/cxl/core/region.c               | 446 ++++++++++++++++++++----
 drivers/cxl/core/regs.c                 |   9 +-
 drivers/cxl/core/suspend.c              |   2 +-
 drivers/cxl/core/trace.c                |   2 +-
 drivers/cxl/core/trace.h                |   4 +-
 drivers/cxl/mem.c                       |  23 +-
 drivers/cxl/pci.c                       |   9 +-
 drivers/cxl/pmem.c                      |   4 +-
 drivers/cxl/port.c                      |   4 +-
 drivers/cxl/security.c                  |   4 +-
 drivers/dax/cxl.c                       |   2 +-
 drivers/perf/cxl_pmu.c                  |   4 +-
 {drivers/cxl => include/linux}/cxl.h    |   5 +
 {drivers/cxl => include/linux}/cxlmem.h |  22 +-
 {drivers/cxl => include/linux}/cxlpci.h |   2 +
 tools/testing/cxl/Kbuild                |   1 +
 tools/testing/cxl/cxl_core_exports.c    |   2 +-
 tools/testing/cxl/mock_acpi.c           |   2 +-
 tools/testing/cxl/test/cxl.c            |   2 +-
 tools/testing/cxl/test/mem.c            |   2 +-
 tools/testing/cxl/test/mock.c           |   4 +-
 tools/testing/cxl/test/mock.h           |   2 +-
 tools/testing/cxl/type2/Kbuild          |   7 +
 tools/testing/cxl/type2/pci_type2.c     | 201 +++++++++++
 34 files changed, 886 insertions(+), 153 deletions(-)
 rename {drivers/cxl => include/linux}/cxl.h (99%)
 rename {drivers/cxl => include/linux}/cxlmem.h (97%)
 rename {drivers/cxl => include/linux}/cxlpci.h (97%)
 create mode 100644 tools/testing/cxl/type2/Kbuild
 create mode 100644 tools/testing/cxl/type2/pci_type2.c

-- 
2.17.1

[RFC PATCH 01/13] cxl: move header files for absolute references

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

CXL Type 2 devices imply specific vendor drivers binding to those
devices instead of generic ones offered by CXL core like the PCI driver.
Those drivers need to use CXL core functions and structs for
initialization, create memdevs and create CXL regions.

This patch avoids referencing those files based on relative paths from
inside the kernel sources tree.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/acpi.c                      | 4 ++--
 drivers/cxl/core/cdat.c                 | 6 +++---
 drivers/cxl/core/hdm.c                  | 2 +-
 drivers/cxl/core/mbox.c                 | 6 +++---
 drivers/cxl/core/memdev.c               | 2 +-
 drivers/cxl/core/pci.c                  | 6 +++---
 drivers/cxl/core/pmem.c                 | 4 ++--
 drivers/cxl/core/pmu.c                  | 4 ++--
 drivers/cxl/core/port.c                 | 6 +++---
 drivers/cxl/core/region.c               | 4 ++--
 drivers/cxl/core/regs.c                 | 4 ++--
 drivers/cxl/core/suspend.c              | 2 +-
 drivers/cxl/core/trace.c                | 2 +-
 drivers/cxl/core/trace.h                | 4 ++--
 drivers/cxl/mem.c                       | 4 ++--
 drivers/cxl/pci.c                       | 6 +++---
 drivers/cxl/pmem.c                      | 4 ++--
 drivers/cxl/port.c                      | 4 ++--
 drivers/cxl/security.c                  | 4 ++--
 drivers/dax/cxl.c                       | 2 +-
 drivers/perf/cxl_pmu.c                  | 4 ++--
 {drivers/cxl => include/linux}/cxl.h    | 0
 {drivers/cxl => include/linux}/cxlmem.h | 2 +-
 {drivers/cxl => include/linux}/cxlpci.h | 0
 tools/testing/cxl/cxl_core_exports.c    | 2 +-
 tools/testing/cxl/mock_acpi.c           | 2 +-
 tools/testing/cxl/test/cxl.c            | 2 +-
 tools/testing/cxl/test/mem.c            | 2 +-
 tools/testing/cxl/test/mock.c           | 4 ++--
 tools/testing/cxl/test/mock.h           | 2 +-
 30 files changed, 50 insertions(+), 50 deletions(-)
 rename {drivers/cxl => include/linux}/cxl.h (100%)
 rename {drivers/cxl => include/linux}/cxlmem.h (99%)
 rename {drivers/cxl => include/linux}/cxlpci.h (100%)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cb8c155a2c9b..f023898382bc 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -8,8 +8,8 @@
 #include <linux/pci.h>
 #include <linux/node.h>
 #include <asm/div64.h>
-#include "cxlpci.h"
-#include "cxl.h"
+#include <linux/cxlpci.h>
+#include <linux/cxl.h>
 
 #define CXL_RCRB_SIZE	SZ_8K
 
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index bb83867d9fec..97ff1dfd63d6 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -5,10 +5,10 @@
 #include <linux/fw_table.h>
 #include <linux/node.h>
 #include <linux/overflow.h>
-#include "cxlpci.h"
-#include "cxlmem.h"
+#include <linux/cxlpci.h>
+#include <linux/cxlmem.h>
 #include "core.h"
-#include "cxl.h"
+#include <linux/cxl.h>
 #include "core.h"
 
 struct dsmas_entry {
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 7d97790b893d..47d9faf5897f 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -4,7 +4,7 @@
 #include <linux/device.h>
 #include <linux/delay.h>
 
-#include "cxlmem.h"
+#include <linux/cxlmem.h>
 #include "core.h"
 
 /**
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index f0f54aeccc87..d312b82f7f36 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -5,9 +5,9 @@
 #include <linux/ktime.h>
 #include <linux/mutex.h>
 #include <asm/unaligned.h>
-#include <cxlpci.h>
-#include <cxlmem.h>
-#include <cxl.h>
+#include <linux/cxlpci.h>
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 
 #include "core.h"
 #include "trace.h"
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index d4e259f3a7e9..07cd0b8b026f 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -7,7 +7,7 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <linux/pci.h>
-#include <cxlmem.h>
+#include <linux/cxlmem.h>
 #include "trace.h"
 #include "core.h"
 
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0df09bd79408..a494a03b4a83 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -7,9 +7,9 @@
 #include <linux/pci.h>
 #include <linux/pci-doe.h>
 #include <linux/aer.h>
-#include <cxlpci.h>
-#include <cxlmem.h>
-#include <cxl.h>
+#include <linux/cxlpci.h>
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 #include "core.h"
 #include "trace.h"
 
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index e69625a8d6a1..d1a5f8d9cf91 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -3,8 +3,8 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
-#include <cxlmem.h>
-#include <cxl.h>
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 #include "core.h"
 
 /**
diff --git a/drivers/cxl/core/pmu.c b/drivers/cxl/core/pmu.c
index 5d8e06b0ba6e..aa02ea582184 100644
--- a/drivers/cxl/core/pmu.c
+++ b/drivers/cxl/core/pmu.c
@@ -4,9 +4,9 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
-#include <cxlmem.h>
+#include <linux/cxlmem.h>
 #include <pmu.h>
-#include <cxl.h>
+#include <linux/cxl.h>
 #include "core.h"
 
 static void cxl_pmu_release(struct device *dev)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 762783bb091a..fdd76306260d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -11,9 +11,9 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <linux/node.h>
-#include <cxlmem.h>
-#include <cxlpci.h>
-#include <cxl.h>
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
+#include <linux/cxl.h>
 #include "core.h"
 
 /**
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..70e86a7c241d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -9,8 +9,8 @@
 #include <linux/uuid.h>
 #include <linux/sort.h>
 #include <linux/idr.h>
-#include <cxlmem.h>
-#include <cxl.h>
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 #include "core.h"
 
 /**
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 3c42f984eeaf..fd165e718cf2 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -4,8 +4,8 @@
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/pci.h>
-#include <cxlmem.h>
-#include <cxlpci.h>
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
 #include <pmu.h>
 
 #include "core.h"
diff --git a/drivers/cxl/core/suspend.c b/drivers/cxl/core/suspend.c
index a5984d96ea1d..117b3398ee56 100644
--- a/drivers/cxl/core/suspend.c
+++ b/drivers/cxl/core/suspend.c
@@ -2,7 +2,7 @@
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 #include <linux/atomic.h>
 #include <linux/export.h>
-#include "cxlmem.h"
+#include <linux/cxlmem.h>
 
 static atomic_t mem_active;
 
diff --git a/drivers/cxl/core/trace.c b/drivers/cxl/core/trace.c
index d0403dc3c8ab..1e6ba0de237a 100644
--- a/drivers/cxl/core/trace.c
+++ b/drivers/cxl/core/trace.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 
-#include <cxl.h>
+#include <linux/cxl.h>
 #include "core.h"
 
 #define CREATE_TRACE_POINTS
diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index e5f13260fc52..e3b1156a01ad 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -10,8 +10,8 @@
 #include <linux/pci.h>
 #include <asm-generic/unaligned.h>
 
-#include <cxl.h>
-#include <cxlmem.h>
+#include <linux/cxl.h>
+#include <linux/cxlmem.h>
 #include "core.h"
 
 #define CXL_RAS_UC_CACHE_DATA_PARITY	BIT(0)
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 0c79d9ce877c..6dc2bf1e2b1a 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -5,8 +5,8 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 
-#include "cxlmem.h"
-#include "cxlpci.h"
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
 
 /**
  * DOC: cxl mem
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..ccde33ac9c1c 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -11,9 +11,9 @@
 #include <linux/pci.h>
 #include <linux/aer.h>
 #include <linux/io.h>
-#include "cxlmem.h"
-#include "cxlpci.h"
-#include "cxl.h"
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
+#include <linux/cxl.h>
 #include "pmu.h"
 
 /**
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 7cb8994f8809..deadc5ffc2c2 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -8,8 +8,8 @@
 #include <linux/async.h>
 #include <linux/slab.h>
 #include <linux/nd.h>
-#include "cxlmem.h"
-#include "cxl.h"
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 
 extern const struct nvdimm_security_ops *cxl_security_ops;
 
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 97c21566677a..928ebff774ce 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -4,8 +4,8 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 
-#include "cxlmem.h"
-#include "cxlpci.h"
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
 
 /**
  * DOC: cxl port
diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
index 21856a3f408e..129b36928a05 100644
--- a/drivers/cxl/security.c
+++ b/drivers/cxl/security.c
@@ -6,8 +6,8 @@
 #include <linux/async.h>
 #include <linux/slab.h>
 #include <linux/memregion.h>
-#include "cxlmem.h"
-#include "cxl.h"
+#include <linux/cxlmem.h>
+#include <linux/cxl.h>
 
 static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
 						 enum nvdimm_passphrase_type ptype)
diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
index c696837ab23c..89a5c8eb666e 100644
--- a/drivers/dax/cxl.c
+++ b/drivers/dax/cxl.c
@@ -3,7 +3,7 @@
 #include <linux/module.h>
 #include <linux/dax.h>
 
-#include "../cxl/cxl.h"
+#include <linux/cxl.h>
 #include "bus.h"
 
 static int cxl_dax_region_probe(struct device *dev)
diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index 308c9969642e..9b93ba215bdb 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -20,8 +20,8 @@
 #include <linux/bug.h>
 #include <linux/pci.h>
 
-#include "../cxl/cxlpci.h"
-#include "../cxl/cxl.h"
+#include <linux/cxlpci.h>
+#include <linux/cxl.h>
 #include "../cxl/pmu.h"
 
 #define CXL_PMU_CAP_REG			0x0
diff --git a/drivers/cxl/cxl.h b/include/linux/cxl.h
similarity index 100%
rename from drivers/cxl/cxl.h
rename to include/linux/cxl.h
diff --git a/drivers/cxl/cxlmem.h b/include/linux/cxlmem.h
similarity index 99%
rename from drivers/cxl/cxlmem.h
rename to include/linux/cxlmem.h
index 36cee9c30ceb..0d26a45a4af2 100644
--- a/drivers/cxl/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -8,7 +8,7 @@
 #include <linux/rcuwait.h>
 #include <linux/cxl-event.h>
 #include <linux/node.h>
-#include "cxl.h"
+#include <linux/cxl.h>
 
 /* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */
 #define CXLMDEV_STATUS_OFFSET 0x0
diff --git a/drivers/cxl/cxlpci.h b/include/linux/cxlpci.h
similarity index 100%
rename from drivers/cxl/cxlpci.h
rename to include/linux/cxlpci.h
diff --git a/tools/testing/cxl/cxl_core_exports.c b/tools/testing/cxl/cxl_core_exports.c
index 077e6883921d..737b49ed3e46 100644
--- a/tools/testing/cxl/cxl_core_exports.c
+++ b/tools/testing/cxl/cxl_core_exports.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 
-#include "cxl.h"
+#include <linux/cxl.h>
 
 /* Exporting of cxl_core symbols that are only used by cxl_test */
 EXPORT_SYMBOL_NS_GPL(cxl_num_decoders_committed, CXL);
diff --git a/tools/testing/cxl/mock_acpi.c b/tools/testing/cxl/mock_acpi.c
index 55813de26d46..4e440a9c0cb2 100644
--- a/tools/testing/cxl/mock_acpi.c
+++ b/tools/testing/cxl/mock_acpi.c
@@ -4,7 +4,7 @@
 #include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/acpi.h>
-#include <cxl.h>
+#include <linux/cxl.h>
 #include "test/mock.h"
 
 struct acpi_device *to_cxl_host_bridge(struct device *host, struct device *dev)
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 61c69297e797..848c42c2c158 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -8,7 +8,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/mm.h>
-#include <cxlmem.h>
+#include <linux/cxlmem.h>
 
 #include "../watermark.h"
 #include "mock.h"
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 35ee41e435ab..dcddb6affc0d 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -9,7 +9,7 @@
 #include <linux/bits.h>
 #include <asm/unaligned.h>
 #include <crypto/sha2.h>
-#include <cxlmem.h>
+#include <linux/cxlmem.h>
 
 #include "trace.h"
 
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 6f737941dc0e..a1366a24677f 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -7,8 +7,8 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
-#include <cxlmem.h>
-#include <cxlpci.h>
+#include <linux/cxlmem.h>
+#include <linux/cxlpci.h>
 #include "mock.h"
 
 static LIST_HEAD(mock);
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index d1b0271d2822..bf7ec147ea80 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -2,7 +2,7 @@
 
 #include <linux/list.h>
 #include <linux/acpi.h>
-#include <cxl.h>
+#include <linux/cxl.h>
 
 struct cxl_mock_ops {
 	struct list_head list;
-- 
2.17.1

[RFC PATCH 02/13] cxl: add type2 device basic support

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Differientiating Type3, aka memory expanders, from Type2, aka device
accelerators, with a new function for initializing cxl_dev_state.

Adding a type2 driver for a CXL emulated device inside CXL kernel
testing infrastructure as a client for the functionality added.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c           | 15 ++++++
 include/linux/cxlmem.h              |  2 +
 tools/testing/cxl/Kbuild            |  1 +
 tools/testing/cxl/type2/Kbuild      |  7 +++
 tools/testing/cxl/type2/pci_type2.c | 80 +++++++++++++++++++++++++++++
 5 files changed, 105 insertions(+)
 create mode 100644 tools/testing/cxl/type2/Kbuild
 create mode 100644 tools/testing/cxl/type2/pci_type2.c

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 07cd0b8b026f..0336b3f14f4a 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work)
 
 static struct lock_class_key cxl_memdev_key;
 
+struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
+{
+	struct cxl_dev_state *cxlds;
+
+	cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
+	if (!cxlds)
+		return ERR_PTR(-ENOMEM);
+
+	cxlds->dev = dev;
+	cxlds->type = CXL_DEVTYPE_DEVMEM;
+
+	return cxlds;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
+
 static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 					   const struct file_operations *fops)
 {
diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
index 0d26a45a4af2..e8d12b543db1 100644
--- a/include/linux/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -859,4 +859,6 @@ struct cxl_hdm {
 struct seq_file;
 struct dentry *cxl_debugfs_create_dir(const char *dir);
 void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
+
+struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
 #endif /* __CXL_MEM_H__ */
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 030b388800f0..a285719c4db6 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
 
 obj-m += test/
+obj-m += type2/
diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild
new file mode 100644
index 000000000000..a96ad4d64924
--- /dev/null
+++ b/tools/testing/cxl/type2/Kbuild
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-m += pci_type2.o
+
+cxl_pci_type2-y := cxl_pci_type2.o
+
+KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
new file mode 100644
index 000000000000..863ce7dc28ef
--- /dev/null
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -0,0 +1,80 @@
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/cxl.h>
+#include <linux/cxlpci.h>
+#include <linux/cxlmem.h>
+
+struct cxl_dev_state *cxlds;
+
+#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
+
+static int type2_pci_probe(struct pci_dev *pci_dev,
+			   const struct pci_device_id *entry)
+
+{
+	u16 dvsec;
+
+	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
+
+	if (!dvsec) {
+		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
+		return 0;
+	} else {
+		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
+	}
+
+	cxlds = cxl_accel_state_create(&pci_dev->dev);
+	if (IS_ERR(cxlds))
+		return PTR_ERR(cxlds);
+
+	pci_info(pci_dev, "Initializing cxlds...");
+	cxlds->cxl_dvsec = dvsec;
+	cxlds->serial = pci_dev->dev.id;
+
+	/* Should not this be based on DVSEC range size registers */
+	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
+	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
+
+	return 0;
+}
+
+static void type2_pci_remove(struct pci_dev *pci_dev)
+{
+
+}
+
+/* PCI device ID table */
+static const struct pci_device_id type2_pci_table[] = {
+	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
+	{0}                     /* end of list */
+};
+
+static struct pci_driver type2_pci_driver = {
+	.name           = KBUILD_MODNAME,
+	.id_table       = type2_pci_table,
+	.probe          = type2_pci_probe,
+	.remove         = type2_pci_remove,
+};
+
+static int __init type2_cxl_init(void)
+{
+	int rc;
+
+	rc = pci_register_driver(&type2_pci_driver);
+
+	return rc;
+}
+
+static void __exit type2_cxl_exit(void)
+{
+	pci_unregister_driver(&type2_pci_driver);
+}
+
+module_init(type2_cxl_init);
+module_exit(type2_cxl_exit);
+
+MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>");
+MODULE_DESCRIPTION("CXL Type2 device support, driver test");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(CXL);
+MODULE_DEVICE_TABLE(pci, type2_pci_table);
-- 
2.17.1

[RFC PATCH 03/13] cxl: export core function for type2 devices

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

CXL initialization by type2 devices requires to use current CXL kernel
infrastructure only available to such core code. Type2 devices are by
definition owned by specific vendor drivers which need to use part of
that infrastructure for initialization.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/pci.c                   |  3 ++-
 include/linux/cxlpci.h              |  2 ++
 tools/testing/cxl/type2/pci_type2.c | 31 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index ccde33ac9c1c..497276302017 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -500,7 +500,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
 	return 0;
 }
 
-static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
+int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 			      struct cxl_register_map *map)
 {
 	int rc;
@@ -520,6 +520,7 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 
 	return cxl_setup_regs(map);
 }
+EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL);
 
 static int cxl_pci_ras_unmask(struct pci_dev *pdev)
 {
diff --git a/include/linux/cxlpci.h b/include/linux/cxlpci.h
index 93992a1c8eec..28fa4861a4f9 100644
--- a/include/linux/cxlpci.h
+++ b/include/linux/cxlpci.h
@@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
 void cxl_cor_error_detected(struct pci_dev *pdev);
 pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
 				    pci_channel_state_t state);
+int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
+		       struct cxl_register_map *map);
 #endif /* __CXL_PCI_H__ */
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index 863ce7dc28ef..b12f13e676fb 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -12,7 +12,9 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 			   const struct pci_device_id *entry)
 
 {
+	struct cxl_register_map map;
 	u16 dvsec;
+	int rc;
 
 	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
 
@@ -35,6 +37,35 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
 	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
 
+	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_MEMDEV, &map);
+	if (rc)
+		return rc;
+
+	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
+	if (rc)
+		return rc;
+
+	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
+				&cxlds->reg_map);
+	if (rc)
+		dev_warn(&pci_dev->dev, "No component registers (%d)\n", rc);
+
+	rc = cxl_map_component_regs(&cxlds->reg_map, &cxlds->regs.component,
+				    BIT(CXL_CM_CAP_CAP_ID_RAS));
+	if (rc)
+		dev_dbg(&pci_dev->dev, "Failed to map RAS capability.\n");
+
+	pci_info(pci_dev, "requesting resource...");
+	rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
+	if (rc)
+		return rc;
+
+	rc = cxl_await_media_ready(cxlds);
+	if (rc == 0)
+		cxlds->media_ready = true;
+	else
+		dev_warn(&pci_dev->dev, "Media not active (%d)\n", rc);
+
 	return 0;
 }
 
-- 
2.17.1

[RFC PATCH 04/13] cxl: allow devices without mailbox capability

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

A device not implementing the CXL memory-device class code, aka Type2
devices, has mailbox capability as optional. If the device registers
mapping does not show such capability, do not treat that as an error.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/regs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index fd165e718cf2..5434a7c899fd 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -437,11 +437,10 @@ static int cxl_probe_regs(struct cxl_register_map *map)
 	case CXL_REGLOC_RBI_MEMDEV:
 		dev_map = &map->device_map;
 		cxl_probe_device_regs(host, base, dev_map);
-		if (!dev_map->status.valid || !dev_map->mbox.valid ||
+		if (!dev_map->status.valid ||
 		    !dev_map->memdev.valid) {
-			dev_err(host, "registers not found: %s%s%s\n",
+			dev_err(host, "registers not found: %s%s\n",
 				!dev_map->status.valid ? "status " : "",
-				!dev_map->mbox.valid ? "mbox " : "",
 				!dev_map->memdev.valid ? "memdev " : "");
 			return -ENXIO;
 		}
-- 
2.17.1

[RFC PATCH 05/13] cxl: fix check about pmem resource

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Current check is using resource_size which counts on a range bigger than
0. For a resource with start and end being 0, resource_size returns 1
and implying a false positive. Use the end not being zero as the new check.

Note: If I´m not missing anything here, this should be extended to the
whole linux kernel where resource_size is being used in conditionals,
and where the likely right fix is to modify resource_size itself
checking for the range not being 0.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/hdm.c    | 4 ++--
 drivers/cxl/core/memdev.c | 8 ++++----
 drivers/cxl/mem.c         | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 47d9faf5897f..c5f70741d70a 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -432,12 +432,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
 	 * Only allow modes that are supported by the current partition
 	 * configuration
 	 */
-	if (mode == CXL_DECODER_PMEM && !resource_size(&cxlds->pmem_res)) {
+	if (mode == CXL_DECODER_PMEM && !cxlds->pmem_res.end) {
 		dev_dbg(dev, "no available pmem capacity\n");
 		rc = -ENXIO;
 		goto out;
 	}
-	if (mode == CXL_DECODER_RAM && !resource_size(&cxlds->ram_res)) {
+	if (mode == CXL_DECODER_RAM && !cxlds->ram_res.end) {
 		dev_dbg(dev, "no available ram capacity\n");
 		rc = -ENXIO;
 		goto out;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 0336b3f14f4a..b61d57d0d4f4 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -197,14 +197,14 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
 	int rc = 0;
 
 	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
-	if (resource_size(&cxlds->pmem_res)) {
+	if (cxlds->pmem_res.end) {
 		offset = cxlds->pmem_res.start;
 		length = resource_size(&cxlds->pmem_res);
 		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
 		if (rc)
 			return rc;
 	}
-	if (resource_size(&cxlds->ram_res)) {
+	if (cxlds->ram_res.end) {
 		offset = cxlds->ram_res.start;
 		length = resource_size(&cxlds->ram_res);
 		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
@@ -266,7 +266,7 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
 		return 0;
 
 	cxled = to_cxl_endpoint_decoder(dev);
-	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
+	if (!cxled->dpa_res || !cxled->dpa_res->end)
 		return 0;
 
 	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
@@ -302,7 +302,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
-	if (!resource_size(&cxlds->dpa_res)) {
+	if (!cxlds->dpa_res.end) {
 		dev_dbg(cxlds->dev, "device has no dpa resource\n");
 		return -EINVAL;
 	}
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 6dc2bf1e2b1a..a168343d2d4d 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -174,7 +174,7 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
+	if (cxlds->pmem_res.end && IS_ENABLED(CONFIG_CXL_PMEM)) {
 		rc = devm_cxl_add_nvdimm(cxlmd);
 		if (rc == -ENODEV)
 			dev_info(dev, "PMEM disabled by platform\n");
-- 
2.17.1

[RFC PATCH 06/13] cxl: support type2 memdev creation

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Adding memdev creation to the cxl type2 test driver.

Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
creating a memdev leading to problems when obtaining cxl_memdev_state
references from a CXL_DEVTYPE_DEVMEM type. This last device type is
managed by a specific vendor driver and does not need same sysfs files
since not userspace intervention is expected. This patch checks for the
right device type in those functions using cxl_memdev_state.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/cdat.c             |  3 +++
 drivers/cxl/core/memdev.c           |  9 +++++++++
 drivers/cxl/mem.c                   | 17 +++++++++++------
 tools/testing/cxl/type2/pci_type2.c |  6 ++++++
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 97ff1dfd63d6..292efd7e68ad 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -558,6 +558,9 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 	};
 	struct cxl_dpa_perf *perf;
 
+	if (!mds)
+		return;
+
 	switch (cxlr->mode) {
 	case CXL_DECODER_RAM:
 		perf = &mds->ram_perf;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index b61d57d0d4f4..27063cd4ea73 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -511,6 +511,9 @@ static umode_t cxl_ram_visible(struct kobject *kobj, struct attribute *a, int n)
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 
+	if (!mds)
+		return 0;
+
 	if (a == &dev_attr_ram_qos_class.attr)
 		if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID)
 			return 0;
@@ -530,6 +533,9 @@ static umode_t cxl_pmem_visible(struct kobject *kobj, struct attribute *a, int n
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 
+	if (!mds)
+		return 0;
+
 	if (a == &dev_attr_pmem_qos_class.attr)
 		if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID)
 			return 0;
@@ -550,6 +556,9 @@ static umode_t cxl_memdev_security_visible(struct kobject *kobj,
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 
+	if (!mds)
+		return 0;
+
 	if (a == &dev_attr_security_sanitize.attr &&
 	    !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds))
 		return 0;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index a168343d2d4d..da63ce486e1a 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -131,12 +131,14 @@ static int cxl_mem_probe(struct device *dev)
 	dentry = cxl_debugfs_create_dir(dev_name(dev));
 	debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
 
-	if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
-		debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
-				    &cxl_poison_inject_fops);
-	if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
-		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
-				    &cxl_poison_clear_fops);
+	if (mds) {
+		if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
+			debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
+					    &cxl_poison_inject_fops);
+		if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
+			debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
+					    &cxl_poison_clear_fops);
+	}
 
 	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
 	if (rc)
@@ -221,6 +223,9 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
 
+	if (!mds)
+		return 0;
+
 	if (a == &dev_attr_trigger_poison_list.attr)
 		if (!test_bit(CXL_POISON_ENABLED_LIST,
 			      mds->poison.enabled_cmds))
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index b12f13e676fb..f157139b712f 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -5,6 +5,7 @@
 #include <linux/cxlmem.h>
 
 struct cxl_dev_state *cxlds;
+struct cxl_memdev *cxlmd;
 
 #define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
 
@@ -66,6 +67,11 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 	else
 		dev_warn(&pci_dev->dev, "Media not active (%d)\n", rc);
 
+	pci_info(pci_dev, "cxl adding memdev...");
+	cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxlds);
+	if (IS_ERR(cxlmd))
+		return PTR_ERR(cxlmd);
+
 	return 0;
 }
 
-- 
2.17.1

[RFC PATCH 07/13] cxl: add functions for exclusive access to endpoint port topology

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Prevent concurrent access to endpoint port topology.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c           | 41 +++++++++++++++++++++++++++++
 include/linux/cxlmem.h              |  4 +++
 tools/testing/cxl/type2/pci_type2.c |  9 +++++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 27063cd4ea73..16e356ef5b6d 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -1124,6 +1124,47 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
 
+/*
+ * Try to get a locked reference on a memdev's CXL port topology
+ * connection. Be careful to observe when cxl_mem_probe() has deposited
+ * a probe deferral awaiting the arrival of the CXL root driver
+*/
+struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)
+{
+	struct cxl_port *endpoint;
+	int rc = -ENXIO;
+
+	device_lock(&cxlmd->dev);
+	endpoint = cxlmd->endpoint;
+	if (!endpoint)
+		goto err;
+
+	if (IS_ERR(endpoint)) {
+		rc = PTR_ERR(endpoint);
+		goto err;
+	}
+
+	device_lock(&endpoint->dev);
+	if (!endpoint->dev.driver)
+		goto err_endpoint;
+
+	return endpoint;
+
+err_endpoint:
+	device_unlock(&endpoint->dev);
+err:
+	device_unlock(&cxlmd->dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_NS(cxl_acquire_endpoint, CXL);
+
+void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
+{
+	device_unlock(&endpoint->dev);
+	device_unlock(&cxlmd->dev);
+}
+EXPORT_SYMBOL_NS(cxl_release_endpoint, CXL);
+
 static void sanitize_teardown_notifier(void *data)
 {
 	struct cxl_memdev_state *mds = data;
diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
index e8d12b543db1..11fe8367b046 100644
--- a/include/linux/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -88,6 +88,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 
 struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
 				       struct cxl_dev_state *cxlds);
+
+struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd);
+void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
+
 int devm_cxl_sanitize_setup_notifier(struct device *host,
 				     struct cxl_memdev *cxlmd);
 struct cxl_memdev_state;
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index f157139b712f..948cc95c5780 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -6,6 +6,7 @@
 
 struct cxl_dev_state *cxlds;
 struct cxl_memdev *cxlmd;
+struct cxl_port *endpoint;
 
 #define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
 
@@ -72,6 +73,14 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
+	endpoint = cxl_acquire_endpoint(cxlmd);
+	if (IS_ERR(endpoint)) {
+		dev_dbg(&pci_dev->dev, "cxl_acquire_endpoint failed\n");
+		return PTR_ERR(endpoint);
+	}
+
+	cxl_release_endpoint(cxlmd, endpoint);
+
 	return 0;
 }
 
-- 
2.17.1

[RFC PATCH 08/13] cxl: add cxl_get_hpa_freespace

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Based on the requirements from the endpoint and the topology such an
endpoint is attached to, this function informs about maximum host
physical address space possible to request. This is not a reservation
but only information which could change at the point the request based
on this information is made.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c           | 163 ++++++++++++++++++++++++++++
 include/linux/cxl.h                 |   5 +
 include/linux/cxlmem.h              |   5 +
 tools/testing/cxl/type2/pci_type2.c |  23 +++-
 4 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 70e86a7c241d..2731fd4243a1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -702,6 +702,169 @@ static int free_hpa(struct cxl_region *cxlr)
 	return 0;
 }
 
+
+struct cxlrd_max_context {
+	struct device * const *host_bridges;
+	int interleave_ways;
+	unsigned long flags;
+	resource_size_t max_hpa;
+	struct cxl_root_decoder *cxlrd;
+};
+
+static int find_max_hpa(struct device *dev, void *data)
+{
+	struct cxlrd_max_context *ctx = data;
+	struct cxl_switch_decoder *cxlsd;
+	struct cxl_root_decoder *cxlrd;
+	struct resource *res, *prev;
+	struct cxl_decoder *cxld;
+	resource_size_t max;
+	int found;
+
+	if (!is_root_decoder(dev))
+		return 0;
+
+	cxlrd = to_cxl_root_decoder(dev);
+	cxld = &cxlrd->cxlsd.cxld;
+	if ((cxld->flags & ctx->flags) != ctx->flags) {
+		dev_dbg(dev, "find_max_hpa, flags not matching: %08lx vs %08lx\n",
+			      cxld->flags, ctx->flags);
+		return 0;
+	}
+
+	/* A Host bridge could have more interleave ways than an
+	 * endpoint, couldn´t it?
+	 *
+	 * What does interleave ways mean here in terms of the requestor?
+	 * Why the FFMWS has 0 interleave ways but root port has 1?
+	 */
+	if (cxld->interleave_ways != ctx->interleave_ways) {
+		dev_dbg(dev, "find_max_hpa, interleave_ways  not matching\n");
+		return 0;
+	}
+
+	cxlsd = &cxlrd->cxlsd;
+
+	guard(rwsem_read)(&cxl_region_rwsem);
+	found = 0;
+	for (int i = 0; i < ctx->interleave_ways; i++)
+		for (int j = 0; j < ctx->interleave_ways; j++)
+			if (ctx->host_bridges[i] ==
+					cxlsd->target[j]->dport_dev) {
+				found++;
+				break;
+			}
+
+	if (found != ctx->interleave_ways) {
+		dev_dbg(dev, "find_max_hpa, no interleave_ways found\n");
+		return 0;
+	}
+
+	/*
+	 * Walk the root decoder resource range relying on cxl_region_rwsem to
+	 * preclude sibling arrival/departure and find the largest free space
+	 * gap.
+	 */
+	lockdep_assert_held_read(&cxl_region_rwsem);
+	max = 0;
+	res = cxlrd->res->child;
+	if (!res)
+		max = resource_size(cxlrd->res);
+	else
+		max = 0;
+
+	for (prev = NULL; res; prev = res, res = res->sibling) {
+		struct resource *next = res->sibling;
+		resource_size_t free = 0;
+
+		if (!prev && res->start > cxlrd->res->start) {
+			free = res->start - cxlrd->res->start;
+			max = max(free, max);
+		}
+		if (prev && res->start > prev->end + 1) {
+			free = res->start - prev->end + 1;
+			max = max(free, max);
+		}
+		if (next && res->end + 1 < next->start) {
+			free = next->start - res->end + 1;
+			max = max(free, max);
+		}
+		if (!next && res->end + 1 < cxlrd->res->end + 1) {
+			free = cxlrd->res->end + 1 - res->end + 1;
+			max = max(free, max);
+		}
+	}
+
+	if (max > ctx->max_hpa) {
+		if (ctx->cxlrd)
+			put_device(CXLRD_DEV(ctx->cxlrd));
+		get_device(CXLRD_DEV(cxlrd));
+		ctx->cxlrd = cxlrd;
+		ctx->max_hpa = max;
+		dev_info(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", &max);
+	}
+	return 0;
+}
+
+/**
+ * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints
+ * @endpoint: an endpoint that is mapped by the returned decoder
+ * @host_bridges: array of host-bridges that the decoder must interleave
+ * @interleave_ways: number of entries in @host_bridges
+ * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B]
+ * @max: output parameter of bytes available in the returned decoder
+ *
+ * The return tuple of a 'struct cxl_root_decoder' and 'bytes available (@max)'
+ * is a point in time snapshot. If by the time the caller goes to use this root
+ * decoder's capacity the capacity is reduced then caller needs to loop and
+ * retry.
+ *
+ * The returned root decoder has an elevated reference count that needs to be
+ * put with put_device(cxlrd_dev(cxlrd)). Locking context is with
+ * cxl_{acquire,release}_endpoint(), that ensures removal of the root decoder
+ * does not race.
+ */
+struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
+					       struct device *const *host_bridges,
+					       int interleave_ways,
+					       unsigned long flags,
+					       resource_size_t *max)
+{
+
+	struct cxlrd_max_context ctx = {
+		.host_bridges = host_bridges,
+		.interleave_ways = interleave_ways,
+		.flags = flags,
+	};
+	struct cxl_port *root_port;
+	struct cxl_root *root;
+
+	if (!is_cxl_endpoint(endpoint)) {
+		dev_dbg(&endpoint->dev, "hpa requestor is not an endpointr\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	root = find_cxl_root(endpoint);
+	if (!root) {
+		dev_dbg(&endpoint->dev, "endpoint can not be related to a root port\n");
+		return ERR_PTR(-ENXIO);
+	}
+
+	root_port = &root->port;
+	down_read(&cxl_region_rwsem);
+	device_for_each_child(&root_port->dev, &ctx, find_max_hpa);
+	up_read(&cxl_region_rwsem);
+	put_device(&root_port->dev);
+
+	if (!ctx.cxlrd)
+		return ERR_PTR(-ENOMEM);
+
+	*max = ctx.max_hpa;
+	return ctx.cxlrd;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_hpa_freespace, CXL);
+
+
 static ssize_t size_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t len)
 {
diff --git a/include/linux/cxl.h b/include/linux/cxl.h
index 036d17db68e0..1b2377062693 100644
--- a/include/linux/cxl.h
+++ b/include/linux/cxl.h
@@ -766,6 +766,11 @@ struct cxl_decoder *to_cxl_decoder(struct device *dev);
 struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
 struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev);
 struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev);
+
+#define CXLED_DEV(cxled)  &cxled->cxld.dev
+
+#define CXLRD_DEV(cxlrd) &cxlrd->cxlsd.cxld.dev
+
 bool is_root_decoder(struct device *dev);
 bool is_switch_decoder(struct device *dev);
 bool is_endpoint_decoder(struct device *dev);
diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
index 11fe8367b046..342ccd5486d3 100644
--- a/include/linux/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -865,4 +865,9 @@ struct dentry *cxl_debugfs_create_dir(const char *dir);
 void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
 
 struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
+struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
+					   struct device *const *host_bridges,
+					   int interleave_ways,
+					   unsigned long flags,
+					   resource_size_t *max);
 #endif /* __CXL_MEM_H__ */
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index 948cc95c5780..deb5eeae501b 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -4,6 +4,7 @@
 #include <linux/cxlpci.h>
 #include <linux/cxlmem.h>
 
+struct cxl_root_decoder *cxlrd;
 struct cxl_dev_state *cxlds;
 struct cxl_memdev *cxlmd;
 struct cxl_port *endpoint;
@@ -15,6 +16,7 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 
 {
 	struct cxl_register_map map;
+	resource_size_t max = 0;
 	u16 dvsec;
 	int rc;
 
@@ -79,9 +81,28 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 		return PTR_ERR(endpoint);
 	}
 
+	pci_info(pci_dev, "cxl hpa_freespace...");
+	cxlrd = cxl_get_hpa_freespace(endpoint, &endpoint->host_bridge, 1,
+				      CXL_DECODER_F_RAM | CXL_DECODER_F_TYPE2,
+				      &max);
+
+	if (IS_ERR(cxlrd)) {
+		dev_dbg(&pci_dev->dev, "cxl_get_hpa_freespace failed\n");
+		rc = PTR_ERR(cxlrd);
+		goto out;
+	}
+
+	if (max < CXL_TYPE2_MEM_SIZE) {
+		dev_dbg(&pci_dev->dev, "%s: no enough free HPA space %llu < %u\n",
+				       __func__, max, CXL_TYPE2_MEM_SIZE);
+		rc = -ENOMEM;
+		goto out;
+	}
+
+out:
 	cxl_release_endpoint(cxlmd, endpoint);
 
-	return 0;
+	return rc;
 }
 
 static void type2_pci_remove(struct pci_dev *pci_dev)
-- 
2.17.1

[RFC PATCH 09/13] cxl: add cxl_request_dpa

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Search and reserve DPA given input constraints.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/core.h             |   1 -
 drivers/cxl/core/hdm.c              | 153 +++++++++++++++++++++++-----
 include/linux/cxlmem.h              |   5 +
 tools/testing/cxl/type2/pci_type2.c |  12 ++-
 4 files changed, 145 insertions(+), 26 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index bc5a95665aa0..c0a2e2c1ccb3 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -61,7 +61,6 @@ struct dentry *cxl_debugfs_create_dir(const char *dir);
 int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
 		     enum cxl_decoder_mode mode);
 int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size);
-int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
 resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled);
 resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled);
 
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index c5f70741d70a..6459b6ecde88 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -404,6 +404,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
 	up_write(&cxl_dpa_rwsem);
 	return rc;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_dpa_free, CXL);
 
 int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
 		     enum cxl_decoder_mode mode)
@@ -451,30 +452,17 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
 	return rc;
 }
 
-int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
+static resource_size_t cxl_dpa_freespace(struct cxl_endpoint_decoder *cxled,
+					 resource_size_t *start_out,
+					 resource_size_t *skip_out)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	resource_size_t free_ram_start, free_pmem_start;
-	struct cxl_port *port = cxled_to_port(cxled);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct device *dev = &cxled->cxld.dev;
 	resource_size_t start, avail, skip;
 	struct resource *p, *last;
-	int rc;
-
-	down_write(&cxl_dpa_rwsem);
-	if (cxled->cxld.region) {
-		dev_dbg(dev, "decoder attached to %s\n",
-			dev_name(&cxled->cxld.region->dev));
-		rc = -EBUSY;
-		goto out;
-	}
 
-	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) {
-		dev_dbg(dev, "decoder enabled\n");
-		rc = -EBUSY;
-		goto out;
-	}
+	lockdep_assert_held(&cxl_dpa_rwsem);
 
 	for (p = cxlds->ram_res.child, last = NULL; p; p = p->sibling)
 		last = p;
@@ -496,7 +484,6 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
 		skip = 0;
 	} else if (cxled->mode == CXL_DECODER_PMEM) {
 		resource_size_t skip_start, skip_end;
-
 		start = free_pmem_start;
 		avail = cxlds->pmem_res.end - start + 1;
 		skip_start = free_ram_start;
@@ -506,21 +493,50 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
 		 * already handled the skip.
 		 */
 		if (cxlds->pmem_res.child &&
-		    skip_start == cxlds->pmem_res.child->start)
+				skip_start == cxlds->pmem_res.child->start)
 			skip_end = skip_start - 1;
 		else
 			skip_end = start - 1;
 		skip = skip_end - skip_start + 1;
 	} else {
-		dev_dbg(dev, "mode not set\n");
-		rc = -EINVAL;
+		avail = 0;
+	}
+
+	if (!avail)
+		return 0;
+	if (start_out)
+		*start_out = start;
+	if (skip_out)
+		*skip_out = skip;
+	return avail;
+}
+
+int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
+{
+	struct cxl_port *port = cxled_to_port(cxled);
+	struct device *dev = &cxled->cxld.dev;
+	resource_size_t start, avail, skip;
+	int rc;
+
+	down_write(&cxl_dpa_rwsem);
+	if (cxled->cxld.region) {
+		dev_dbg(dev, "EBUSY, decoder attached to %s\n",
+			     dev_name(&cxled->cxld.region->dev));
+		rc = -EBUSY;
 		goto out;
 	}
 
+	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) {
+		dev_dbg(dev, "EBUSY, decoder enabled\n");
+		rc = -EBUSY;
+		goto out;
+	}
+
+	avail = cxl_dpa_freespace(cxled, &start, &skip);
 	if (size > avail) {
 		dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size,
-			cxled->mode == CXL_DECODER_RAM ? "ram" : "pmem",
-			&avail);
+			     cxled->mode == CXL_DECODER_RAM ? "ram" : "pmem",
+			     &avail);
 		rc = -ENOSPC;
 		goto out;
 	}
@@ -532,9 +548,98 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
 	if (rc)
 		return rc;
 
-	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
+        return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
 }
 
+static int find_free_decoder(struct device *dev, void *data)
+{
+	struct cxl_endpoint_decoder *cxled;
+	struct cxl_port *port;
+
+	if (!is_endpoint_decoder(dev))
+		return 0;
+
+	cxled = to_cxl_endpoint_decoder(dev);
+	port = cxled_to_port(cxled);
+
+	if (cxled->cxld.id != port->hdm_end + 1) {
+		return 0;
+	}
+	return 1;
+}
+
+/**
+ * cxl_request_dpa - search and reserve DPA given input constraints
+ * @endpoint: an endpoint port with available decoders
+ * @mode: DPA operation mode (ram vs pmem)
+ * @min: the minimum amount of capacity the call needs
+ * @max: extra capacity to allocate after min is satisfied
+ *
+ * Given that a region needs to allocate from limited HPA capacity it
+ * may be the case that a device has more mappable DPA capacity than
+ * available HPA. So, the expectation is that @min is a driver known
+ * value for how much capacity is needed, and @max is based the limit of
+ * how much HPA space is available for a new region.
+ *
+ * Returns a pinned cxl_decoder with at least @min bytes of capacity
+ * reserved, or an error pointer. The caller is also expected to own the
+ * lifetime of the memdev registration associated with the endpoint to
+ * pin the decoder registered as well.
+ */
+struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
+					     enum cxl_decoder_mode mode,
+					     resource_size_t min,
+					     resource_size_t max)
+{
+	struct cxl_endpoint_decoder *cxled;
+	struct device *cxled_dev;
+	resource_size_t alloc;
+	int rc;
+
+	if (!IS_ALIGNED(min | max, SZ_256M))
+		return ERR_PTR(-EINVAL);
+
+	down_read(&cxl_dpa_rwsem);
+
+	cxled_dev = device_find_child(&endpoint->dev, NULL, find_free_decoder);
+	if (!cxled_dev)
+		cxled = ERR_PTR(-ENXIO);
+	else
+		cxled = to_cxl_endpoint_decoder(cxled_dev);
+
+	up_read(&cxl_dpa_rwsem);
+
+	if (IS_ERR(cxled)) {
+               return cxled;
+	}
+
+	rc = cxl_dpa_set_mode(cxled, mode);
+	if (rc)
+		goto err;
+
+	down_read(&cxl_dpa_rwsem);
+	alloc = cxl_dpa_freespace(cxled, NULL, NULL);
+	up_read(&cxl_dpa_rwsem);
+
+	if (max)
+		alloc = min(max, alloc);
+	if (alloc < min) {
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	rc = cxl_dpa_alloc(cxled, alloc);
+	if (rc)
+		goto err;
+
+	return cxled;
+err:
+	put_device(cxled_dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, CXL);
+
+
 static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
 {
 	u16 eig;
diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
index 342ccd5486d3..caf1cd86421c 100644
--- a/include/linux/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -870,4 +870,9 @@ struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
 					   int interleave_ways,
 					   unsigned long flags,
 					   resource_size_t *max);
+struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
+					     enum cxl_decoder_mode mode,
+					     resource_size_t min,
+					     resource_size_t max);
+int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
 #endif /* __CXL_MEM_H__ */
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index deb5eeae501b..6499d709f54d 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -4,6 +4,7 @@
 #include <linux/cxlpci.h>
 #include <linux/cxlmem.h>
 
+struct cxl_endpoint_decoder *cxled;
 struct cxl_root_decoder *cxlrd;
 struct cxl_dev_state *cxlds;
 struct cxl_memdev *cxlmd;
@@ -99,6 +100,15 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 		goto out;
 	}
 
+	pci_info(pci_dev, "cxl request_dpa...");
+	cxled = cxl_request_dpa(endpoint, CXL_DECODER_RAM, CXL_TYPE2_MEM_SIZE,
+				CXL_TYPE2_MEM_SIZE);
+	if (IS_ERR(cxled)) {
+		dev_dbg(&pci_dev->dev, "cxl_request_dpa error\n");
+		rc = PTR_ERR(cxled);
+		goto out;
+	}
+
 out:
 	cxl_release_endpoint(cxlmd, endpoint);
 
@@ -107,7 +117,7 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 
 static void type2_pci_remove(struct pci_dev *pci_dev)
 {
-
+	cxl_dpa_free(cxled);
 }
 
 /* PCI device ID table */
-- 
2.17.1

[RFC PATCH 10/13] cxl: make region type based on endpoint type

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Current code is expecting Type3 or CXL_DECODER_HOSTONLYMEM devices only.
Suport for Type2 implies region type needs to be based on the endpoint
type instead.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/region.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 2731fd4243a1..8228b7e96d8d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2574,7 +2574,8 @@ static ssize_t create_ram_region_show(struct device *dev,
 }
 
 static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
-					  enum cxl_decoder_mode mode, int id)
+					  enum cxl_decoder_mode mode, int id,
+					  enum cxl_decoder_type target_type)
 {
 	int rc;
 
@@ -2587,7 +2588,7 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EBUSY);
 	}
 
-	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM);
+	return devm_cxl_add_region(cxlrd, id, mode, target_type);
 }
 
 static ssize_t create_pmem_region_store(struct device *dev,
@@ -2602,7 +2603,8 @@ static ssize_t create_pmem_region_store(struct device *dev,
 	if (rc != 1)
 		return -EINVAL;
 
-	cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id);
+	cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id,
+			       CXL_DECODER_HOSTONLYMEM);
 	if (IS_ERR(cxlr))
 		return PTR_ERR(cxlr);
 
@@ -2622,7 +2624,8 @@ static ssize_t create_ram_region_store(struct device *dev,
 	if (rc != 1)
 		return -EINVAL;
 
-	cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id);
+	cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id,
+			       CXL_DECODER_HOSTONLYMEM);
 	if (IS_ERR(cxlr))
 		return PTR_ERR(cxlr);
 
@@ -3146,7 +3149,8 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	do {
 		cxlr = __create_region(cxlrd, cxled->mode,
-				       atomic_read(&cxlrd->region_id));
+				       atomic_read(&cxlrd->region_id),
+				       cxled->cxld.target_type);
 	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
 
 	if (IS_ERR(cxlr)) {
-- 
2.17.1

[RFC PATCH 11/13] cxl: allow automatic region creation by type2 drivers

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Creating a CXL region requires userspace intervention through the cxl
sysfs files. Type2 support should allow accelerator drivers to create
such cxl region from kernel code.

Adding that functionality and integrating it with current support for
memory expanders.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c           | 262 ++++++++++++++++++++++------
 include/linux/cxlmem.h              |   4 +
 tools/testing/cxl/type2/pci_type2.c |  18 ++
 3 files changed, 228 insertions(+), 56 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8228b7e96d8d..014684ff4343 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -479,22 +479,14 @@ static ssize_t interleave_ways_show(struct device *dev,
 
 static const struct attribute_group *get_cxl_region_target_group(void);
 
-static ssize_t interleave_ways_store(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t len)
+static int set_interleave_ways(struct cxl_region *cxlr, int val)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
-	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
-	unsigned int val, save;
-	int rc;
+	int save, rc;
 	u8 iw;
 
-	rc = kstrtouint(buf, 0, &val);
-	if (rc)
-		return rc;
-
 	rc = ways_to_eiw(val, &iw);
 	if (rc)
 		return rc;
@@ -509,25 +501,42 @@ static ssize_t interleave_ways_store(struct device *dev,
 		return -EINVAL;
 	}
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		rc = -EBUSY;
-		goto out;
-	}
+	lockdep_assert_held_write(&cxl_region_rwsem);
+	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
+		return -EBUSY;
 
 	save = p->interleave_ways;
 	p->interleave_ways = val;
 	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
 	if (rc)
 		p->interleave_ways = save;
-out:
+
+	return rc;
+}
+
+static ssize_t interleave_ways_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	unsigned int val;
+	int rc;
+
+	rc = kstrtouint(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	rc = down_write_killable(&cxl_region_rwsem);
+	if (rc)
+		return rc;
+
+	rc = set_interleave_ways(cxlr, val);
 	up_write(&cxl_region_rwsem);
 	if (rc)
 		return rc;
 	return len;
 }
+
 static DEVICE_ATTR_RW(interleave_ways);
 
 static ssize_t interleave_granularity_show(struct device *dev,
@@ -547,21 +556,14 @@ static ssize_t interleave_granularity_show(struct device *dev,
 	return rc;
 }
 
-static ssize_t interleave_granularity_store(struct device *dev,
-					    struct device_attribute *attr,
-					    const char *buf, size_t len)
+static int set_interleave_granularity(struct cxl_region *cxlr, int val)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
-	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
-	int rc, val;
+	int rc;
 	u16 ig;
 
-	rc = kstrtoint(buf, 0, &val);
-	if (rc)
-		return rc;
-
 	rc = granularity_to_eig(val, &ig);
 	if (rc)
 		return rc;
@@ -577,21 +579,36 @@ static ssize_t interleave_granularity_store(struct device *dev,
 	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
 		return -EINVAL;
 
+	lockdep_assert_held_write(&cxl_region_rwsem);
+	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
+		return -EBUSY;
+
+	p->interleave_granularity = val;
+	return 0;
+}
+
+static ssize_t interleave_granularity_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	int rc, val;
+
+	rc = kstrtoint(buf, 0, &val);
+	if (rc)
+		return rc;
+
 	rc = down_write_killable(&cxl_region_rwsem);
 	if (rc)
 		return rc;
-	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		rc = -EBUSY;
-		goto out;
-	}
 
-	p->interleave_granularity = val;
-out:
+	rc = set_interleave_granularity(cxlr, val);
 	up_write(&cxl_region_rwsem);
 	if (rc)
 		return rc;
 	return len;
 }
+
 static DEVICE_ATTR_RW(interleave_granularity);
 
 static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
@@ -2666,6 +2683,14 @@ cxl_find_region_by_name(struct cxl_root_decoder *cxlrd, const char *name)
 	return to_cxl_region(region_dev);
 }
 
+static void drop_region(struct cxl_region *cxlr)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_port *port = cxlrd_to_port(cxlrd);
+
+	devm_release_action(port->uport_dev, unregister_region, cxlr);
+}
+
 static ssize_t delete_region_store(struct device *dev,
 				   struct device_attribute *attr,
 				   const char *buf, size_t len)
@@ -3135,17 +3160,18 @@ static int match_region_by_range(struct device *dev, void *data)
 	return rc;
 }
 
-/* Establish an empty region covering the given HPA range */
-static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
-					   struct cxl_endpoint_decoder *cxled)
+static void construct_region_end(void)
+{
+	up_write(&cxl_region_rwsem);
+}
+
+static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd,
+						 struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *port = cxlrd_to_port(cxlrd);
-	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
 	struct cxl_region *cxlr;
-	struct resource *res;
-	int rc;
+	int err = 0;
 
 	do {
 		cxlr = __create_region(cxlrd, cxled->mode,
@@ -3154,8 +3180,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
 
 	if (IS_ERR(cxlr)) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s failed assign region: %ld\n",
+		dev_err(cxlmd->dev.parent,"%s:%s: %s failed assign region: %ld\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			__func__, PTR_ERR(cxlr));
 		return cxlr;
@@ -3165,23 +3190,47 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	p = &cxlr->params;
 	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s autodiscovery interrupted\n",
+			"%s:%s: %s region setup interrupted\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			__func__);
-		rc = -EBUSY;
-		goto err;
+		err = -EBUSY;
+	}
+
+	if (err) {
+		construct_region_end();
+		drop_region(cxlr);
+		return ERR_PTR(err);
 	}
+	return cxlr;
+}
+
+
+/* Establish an empty region covering the given HPA range */
+static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
+					   struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct range *hpa = &cxled->cxld.hpa_range;
+	struct cxl_region_params *p;
+	struct cxl_region *cxlr;
+	struct resource *res;
+	int rc;
+
+	cxlr = construct_region_begin(cxlrd, cxled);
+	if (IS_ERR(cxlr))
+		return cxlr;
 
 	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
 
 	res = kmalloc(sizeof(*res), GFP_KERNEL);
 	if (!res) {
 		rc = -ENOMEM;
-		goto err;
+		goto out;
 	}
 
 	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
 				    dev_name(&cxlr->dev));
+
 	rc = insert_resource(cxlrd->res, res);
 	if (rc) {
 		/*
@@ -3194,6 +3243,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 			 __func__, dev_name(&cxlr->dev));
 	}
 
+	p = &cxlr->params;
 	p->res = res;
 	p->interleave_ways = cxled->cxld.interleave_ways;
 	p->interleave_granularity = cxled->cxld.interleave_granularity;
@@ -3201,24 +3251,124 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
 	if (rc)
-		goto err;
+		goto out;
 
 	dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
-		dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
-		dev_name(&cxlr->dev), p->res, p->interleave_ways,
-		p->interleave_granularity);
+				   dev_name(&cxlmd->dev),
+				   dev_name(&cxled->cxld.dev), __func__,
+				   dev_name(&cxlr->dev), p->res,
+				   p->interleave_ways,
+				   p->interleave_granularity);
 
 	/* ...to match put_device() in cxl_add_to_region() */
 	get_device(&cxlr->dev);
 	up_write(&cxl_region_rwsem);
+out:
+	construct_region_end();
+	if (rc) {
+		drop_region(cxlr);
+		return ERR_PTR(rc);
+	}
+	return cxlr;
+}
+
+static struct cxl_region *
+__construct_new_region(struct cxl_root_decoder *cxlrd,
+		       struct cxl_endpoint_decoder **cxled, int ways)
+{
+	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
+	struct cxl_region_params *p;
+	resource_size_t size = 0;
+	struct cxl_region *cxlr;
+	int rc, i;
+
+	/* If interleaving is not supported, why does ways need to be at least 1? */
+	if (ways < 1)
+		return ERR_PTR(-EINVAL);
+
+	cxlr = construct_region_begin(cxlrd, cxled[0]);
+	if (IS_ERR(cxlr))
+		return cxlr;
+
+	rc = set_interleave_ways(cxlr, ways);
+	if (rc)
+		goto out;
+
+	rc = set_interleave_granularity(cxlr, cxld->interleave_granularity);
+	if (rc)
+		goto out;
+
+	down_read(&cxl_dpa_rwsem);
+	for (i = 0; i < ways; i++) {
+		if (!cxled[i]->dpa_res)
+			break;
+		size += resource_size(cxled[i]->dpa_res);
+	}
+	up_read(&cxl_dpa_rwsem);
+
+	if (i < ways)
+		goto out;
+
+	rc = alloc_hpa(cxlr, size);
+	if (rc)
+		goto out;
+
+	down_read(&cxl_dpa_rwsem);
+	for (i = 0; i < ways; i++) {
+		rc = cxl_region_attach(cxlr, cxled[i], i);
+		if (rc)
+			break;
+	}
+	up_read(&cxl_dpa_rwsem);
+
+	if (rc)
+		goto out;
+
+	rc = cxl_region_decode_commit(cxlr);
+	if (rc)
+		goto out;
 
+	p = &cxlr->params;
+	p->state = CXL_CONFIG_COMMIT;
+out:
+	construct_region_end();
+	if (rc) {
+		drop_region(cxlr);
+		return ERR_PTR(rc);
+	}
 	return cxlr;
+}
 
-err:
-	up_write(&cxl_region_rwsem);
-	devm_release_action(port->uport_dev, unregister_region, cxlr);
-	return ERR_PTR(rc);
+/**
+ * cxl_create_region - Establish a region given an array of endpoint decoders
+ * @cxlrd: root decoder to allocate HPA
+ * @cxled: array of endpoint decoders with reserved DPA capacity
+ * @ways: size of @cxled array
+ *
+ * Returns a fully formed region in the commit state and attached to the
+ * cxl_region driver.
+ */
+struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
+				     struct cxl_endpoint_decoder **cxled,
+				     int ways)
+{
+	struct cxl_region *cxlr;
+
+	mutex_lock(&cxlrd->range_lock);
+	cxlr = __construct_new_region(cxlrd, cxled, ways);
+	mutex_unlock(&cxlrd->range_lock);
+
+	if (IS_ERR(cxlr))
+		return cxlr;
+
+	if (device_attach(&cxlr->dev) <= 0) {
+		dev_err(&cxlr->dev, "failed to create region\n");
+		drop_region(cxlr);
+		return ERR_PTR(-ENODEV);
+	}
+	return cxlr;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_create_region, CXL);
 
 int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 {
diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
index caf1cd86421c..fc963c2c2dc4 100644
--- a/include/linux/cxlmem.h
+++ b/include/linux/cxlmem.h
@@ -875,4 +875,8 @@ struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
 					     resource_size_t min,
 					     resource_size_t max);
 int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
+struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
+				     struct cxl_endpoint_decoder **cxled,
+				     int ways);
+
 #endif /* __CXL_MEM_H__ */
diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index 6499d709f54d..0e7f17c0c920 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -4,8 +4,10 @@
 #include <linux/cxlpci.h>
 #include <linux/cxlmem.h>
 
+struct cxl_region_params *region_params;
 struct cxl_endpoint_decoder *cxled;
 struct cxl_root_decoder *cxlrd;
+struct cxl_region *efx_region;
 struct cxl_dev_state *cxlds;
 struct cxl_memdev *cxlmd;
 struct cxl_port *endpoint;
@@ -109,6 +111,22 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 		goto out;
 	}
 
+	pci_info(pci_dev, "cxl create_region...");
+	efx_region = cxl_create_region(cxlrd, &cxled, 1);
+	if (!efx_region) {
+		rc = PTR_ERR(cxled);
+		goto out_dpa;
+	}
+
+	region_params = &efx_region->params;
+	pci_info(pci_dev, "CXL region: start=%llx, end=%llx\n", region_params->res->start,
+			  region_params->res->end);
+
+	cxl_release_endpoint(cxlmd, endpoint);
+	return 0;
+
+out_dpa:
+	cxl_dpa_free(cxled);
 out:
 	cxl_release_endpoint(cxlmd, endpoint);
 
-- 
2.17.1

[RFC PATCH 12/13] cxl: preclude device memory to be used for dax

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

By definition a type2 cxl device will use the host managed memory for
specific functionality, therefore it should not be available to other
uses.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/region.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 014684ff4343..0716c2b8d456 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3485,6 +3485,9 @@ static int cxl_region_probe(struct device *dev)
 	case CXL_DECODER_PMEM:
 		return devm_cxl_add_pmem_region(cxlr);
 	case CXL_DECODER_RAM:
+		if (cxlr->type != CXL_DECODER_HOSTONLYMEM)
+			return 0;
+
 		/*
 		 * The region can not be manged by CXL if any portion of
 		 * it is already online as 'System RAM'
-- 
2.17.1

[RFC PATCH 13/13] cxl: test type2 private mapping

[alucerop]

From: Alejandro Lucero <alucerop@amd.com>

Based on the cxl region allocated, map it internally and do
a write and read to a random offset.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 tools/testing/cxl/type2/pci_type2.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
index 0e7f17c0c920..4bd71e444ab4 100644
--- a/tools/testing/cxl/type2/pci_type2.c
+++ b/tools/testing/cxl/type2/pci_type2.c
@@ -4,6 +4,7 @@
 #include <linux/cxlpci.h>
 #include <linux/cxlmem.h>
 
+void __iomem *ctpio_cxl = NULL, *ctpio_target = NULL;
 struct cxl_region_params *region_params;
 struct cxl_endpoint_decoder *cxled;
 struct cxl_root_decoder *cxlrd;
@@ -20,6 +21,8 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 {
 	struct cxl_register_map map;
 	resource_size_t max = 0;
+	u32 data_read;
+	u32 offset;
 	u16 dvsec;
 	int rc;
 
@@ -122,6 +125,28 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 	pci_info(pci_dev, "CXL region: start=%llx, end=%llx\n", region_params->res->start,
 			  region_params->res->end);
 
+	ctpio_cxl = ioremap(region_params->res->start, region_params->res->end -
+			    region_params->res->start);
+	if (!ctpio_cxl) {
+		printk("%s: ioremap failed\n", __func__);
+	} else {
+		printk("%s: ioremap OK. ctpio_cxl=%p\n", __func__, ctpio_cxl);
+	}
+
+	get_random_bytes(&offset, sizeof(offset));
+
+	offset &= (CXL_TYPE2_MEM_SIZE - 1);
+	offset &= ~0xf;
+	ctpio_target = ctpio_cxl + offset;
+	printk("%s: ctpio_target=%p\n", __func__, ctpio_target);
+
+	*(uint32_t *)ctpio_target = 0xdeadbeef;
+
+	data_read = 0;
+	data_read = *(uint32_t *)ctpio_target;
+
+	printk("%s: ctpio_target=%p read back, %08x\n", __func__, ctpio_target, data_read);
+
 	cxl_release_endpoint(cxlmd, endpoint);
 	return 0;
 
@@ -135,6 +160,7 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
 
 static void type2_pci_remove(struct pci_dev *pci_dev)
 {
+	iounmap(ctpio_cxl);
 	cxl_dpa_free(cxled);
 }
 
-- 
2.17.1

RE: [RFC PATCH 00/13] RFC: add Type2 device support

[Dan Williams]

alucerop@ wrote:
> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
> 
> I need to start this RFC explaining not what the patchset does, but what we
> expected to obtain and currently is under doubt: to configure a CXL memory
> range advertised by our CXL network device and to use it "privately".
> 
> The main reason behind that privacy, but not the only one, is to avoid any
> arbitrary use by any user-space "client" with enough privileges for using a
> public interface to map the device memory and use it as regular memory. The
> reason is obvious: the device expects writes to such a memory in a specific
> format.
> 
> The doubt comes from the fact that after implementing the functionality exposed
> in this patchset, we realized the current expectation seems to be a BIOS/UEFI
> configuring HDM decoders or HPA ranges and passing memory ranges to the kernel,
> and with the kernel having a default action on that memory range based on the
> flag EFI_MEMORY_SP being set or not.

Lets clear up this confusion first.

My expectation for CXL accelerator provided memory is that it is marked "EFI
Reserved" in the memory map (or CDAT), not "EFI Conventional + Special Purpose
Attribute". Recall that the EFI_MEMORY_SP attribute is just a *hint* that OS may
*not* want to consider a memory range as part of the general purpose memory
pool. CXL Accelerator Memory is far from that definition. If a driver needs to
be loaded to effectively use a memory range that is not a range that can be
transparently converted to "EFI Conventional Memory".

> If it is not set, the memory range will be part of the kernel memory
> management, what we do not want for sure. If the flag is set, a DAX device
> will be created which allows an user-space client to map such a memory through
> the DAX device API, what we also prefer to avoid. I know this is likely going
> to face opposition, but we see this RFC as the opportunity for discussing the
> matter and, if it turns out to be the case, to be guided towards the proper
> solution accepted by the maintainers/community. This patchset does not tackle
> this default kernel behaviour although we already have some ideas and
> workarounds for the short term. We'll be happy to discuss this openly.

It may have been subtle, but even in my initial RFC [1]. I was explicitly
skipping exposing accelerator memory via device-dax:

@@ -3085,6 +3183,15 @@ static int cxl_region_probe(struct device *dev)
 					p->res->start, p->res->end, cxlr,
 					is_system_ram) > 0)
 			return 0;
+
+		/*
+		 * HDM-D[B] (device-memory) regions have accelerator
+		 * specific usage, skip device-dax registration.
+		 */
+		if (cxlr->type == CXL_DECODER_DEVMEM)
+			return 0;
+
+		/* HDM-H routes to device-dax */
 		return devm_cxl_add_dax_region(cxlr);
 	default:
 		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",

[1] https://lore.kernel.org/r/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com

> So, this patchset assumes a BIOS/UEFI not programming the HDM decoder or HPA
> ranges for a CXL Type2 device.

At least for Linux it should not care whether BIOS maps it or not. The
expectation is that whichever agent maps it, BIOS or Linux CXL core, that agent
honors the memory type specified in the device CDAT. I.e. the "Device Scoped EFI
Memory Type Structure (DSEMTS)" in the accelerator CDAT should pass "2" in the
"EFI Memory Type and Attribute" field, where "2" indicates "EFI Reserved" memory
type for any HPA that maps this device's DPA.

> above, a Type2 device added after boot, that is hotplugged, will likely need the
> changes added here. Exporting some of the CXL core for vendor drivers is also
> required for avoiding code duplication when reading DVSEC or decoder registers.
> Finally if there is no such HPA range assigned, whatever the reason, this patchset
> offers some way of obtaining one and/or finding out what is the problem behind the
> lack of such HPA range.

Yes, that was the whole point of the "Device Memory Setup" thread [2] to show a
plausible way to reuse the common core infrastructure for accelerators with
CXL.mem capability.

[2] https://lore.kernel.org/all/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/

It would help me if you can summarize what you did and did not adopt from that
proposal, i.e. where it helped and where it missed the mark for your use case.

[..]
> Keeping with kernel rules of having a client using any new functionality added,
> a CXL type2 driver is increasingly added. This is not a driver supporting a real
> device but an emulated one in QEMU, with the QEMU patch following this patchset.

Lets start with the deltas compared to [2], the observation there is that much
of the CXL core is reusable for this type-2 use case, and even improves the
organization of the core. The consumer of the refactoring just ends up being a
few new helpers around the core.

> The reason for adding such a Type2 driver instead of changes to a current kernel
> driver or a new driver is threefold:
> 
> 1) the expected kernel driver to use the functionality added is a netdev one.
>    Current internal CXL support is a codesign effort, therefore software and
>    hardware evolving in lockstep. Adding changes to a netdev driver requires the
>    full functionality and doing things following the netdev standard which is
>    not the best option for this development stage.

Not sure what "netdev standard" means, and not sure it matters for a discussion
that is constrained to how the CXL core should evolve to accommodate
accelerator-local memory.

> 2) Waiting for completing the development will delay the required Type2 support,
>    and most of the required changes are unrelated to specific CXL usage by any
>    vendor driver.

Again, [2] already path cleared the idea that early plumbing of the CXL core for
type-2 is in scope. Just need to make sure this effort is focused on general CXL
specification use cases.

> 
> 3) Type2 support will need some testing infrastructure, unit tests for ensuring
>    Type2 devices are working, and module tests for ensuring CXL core changes do
>    not affect Type2 support.
> 
> I hope these reasons are convincing enough.

I am happy to have general specification discussions about core functionality
that all CXL accelerators need and plumbing those ABIs to be exercised via
something like cxl_test.

I expect that cxl_test only covers coarse grained inter-module ABIs and that the
fine details will need to wait until the accelerator specifics are closer to
being disclosed and the real driver patches can start flowing. In other words, I
am not keen to see QEMU used as a way to proxy features into the kernel without
disclosing what the real world consumer actually needs.

Now, that said, there may also be opportunity for targeted extensions of the
QEMU CXL memory-expander device, like DPA capacity that is mapped as "EFI
Reserved" device-memory, but lets talk about the specifics here.

Also recall that the current CXL driver does not yet support parsing CXL PMEM
region labels. Support for that ends up looking quite similar to an accelerator
asking for a specific DPA range to be dynamically mapped by the CXL core. So I
think we can get quite far along the path to enabling type-2-CXL.mem just by
flushing out some of the generic type-3-CXL.mem use cases.

> I have decided to follow a gradual approach for adding such a driver using the
> exported CXL functions and structs. I think it is easier to review the driver
> when the new funcionality is added than to add the driver at the end, but not a
> big deal if my approach is not liked.
> 
> The patches are based on a patchset sent by Dan Williams [1] which was just
> partially integrated, most related to making things ready for Type2 but none
> related to specific Type2 support. Those patches based on Dan´s work have Dan´s
> signing, so Dan, tell me if you do not want me to add you.

It is fine to reuse those patches, but please do include:

Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Link: <lore-link for copied code>

...so I can try to remember if I still think the source patch was a good idea or
not. Again, a summary analysis of what did and did not work for you from that
posting would help me get a running start at reviewing this.

> Type2 implies, I think, only the related driver to manage the CXL specifics.
> This means no user space intervention and therefore no sysfs files. This makes
> easy to avoid the current problem of most of the sysfs related code expecting
> Type3 devices. If I´m wrong in this regard, such a code will need further
> changes.

Not sure what you mean here, I am hoping that sysfs ABI for enumerating objects
like memdevs, decoders, and regions "just works" and is hidden from the
accelerator vendor driver.

> A final note about CXL.cache is needed. This patchset does not cover it at all,
> although the emulated Type2 device advertises it. From the kernel point of view
> supporting CXL.cache will imply to be sure the CXL path supports what the Type2
> device needs. A device accelerator will likely be connected to a Root Switch,
> but other configurations can not be discarded. Therefore the kernel will need to
> check not just HPA, DPA, interleave and granularity, but also the available
> CXL.cache support and resources in each switch in the CXL path to the Type2
> device. I expect to contribute to this support in the following months, and
> it would be good to discuss about it when possible.

Yup, especially CXL 3.x Cache ID management. Lets talk about some ways to plumb
that, because it is also a general CXL specification capability. Another topic
is what is needed for CXL Protocol and Component error handling, I expect some
of that to be CXL core common and some of that to be accelertor driver
augmented.

If you think we will still be talking about this driver enabling in September
I would invite you to submit this as a topic for the CXL uConf.

https://lpc.events/event/18/contributions/1673/

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Jonathan Cameron]

On Thu, 16 May 2024 09:11:51 +0100
<alucerop@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Differientiating Type3, aka memory expanders, from Type2, aka device
> accelerators, with a new function for initializing cxl_dev_state.
> 
> Adding a type2 driver for a CXL emulated device inside CXL kernel
> testing infrastructure as a client for the functionality added.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

If you are going to keep Dan's sign-off it needs to be clear why.
Either put the author to Dan and swap these two, or use a
Co-developed tag.

A few drive my trivial comments.

> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> new file mode 100644
> index 000000000000..863ce7dc28ef
> --- /dev/null
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -0,0 +1,80 @@
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/cxl.h>
> +#include <linux/cxlpci.h>
> +#include <linux/cxlmem.h>
> +
> +struct cxl_dev_state *cxlds;
> +
> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
> +
> +static int type2_pci_probe(struct pci_dev *pci_dev,
> +			   const struct pci_device_id *entry)
> +
> +{
> +	u16 dvsec;
> +
> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
Add a line break somewhere in there as no real reason it needs to be all in eonline.
> +

Trivial: Drop this blank line to keep error check associated with the call.

> +	if (!dvsec) {
> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
> +		return 0;

Returned, so no point in the else.

> +	} else {
> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
> +	}
> +
> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
> +	if (IS_ERR(cxlds))
> +		return PTR_ERR(cxlds);
> +
> +	pci_info(pci_dev, "Initializing cxlds...");
> +	cxlds->cxl_dvsec = dvsec;
> +	cxlds->serial = pci_dev->dev.id;
> +
> +	/* Should not this be based on DVSEC range size registers */
> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
> +
> +	return 0;
> +}
> +
> +static void type2_pci_remove(struct pci_dev *pci_dev)
> +{
> +
> +}
> +
> +/* PCI device ID table */
> +static const struct pci_device_id type2_pci_table[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
> +	{0}                     /* end of list */
{} is more common.
> +};
> +
> +static struct pci_driver type2_pci_driver = {
> +	.name           = KBUILD_MODNAME,
> +	.id_table       = type2_pci_table,
> +	.probe          = type2_pci_probe,
> +	.remove         = type2_pci_remove,

Add remove only when it does something.

> +};
> +
> +static int __init type2_cxl_init(void)
> +{
> +	int rc;
> +
> +	rc = pci_register_driver(&type2_pci_driver);
> +
> +	return rc;
> +}
> +
> +static void __exit type2_cxl_exit(void)
> +{
> +	pci_unregister_driver(&type2_pci_driver);

Unless you are adding more in here later in the series there is a macro
to cover all this. module_pci_driver()


> +}
> +
> +module_init(type2_cxl_init);
> +module_exit(type2_cxl_exit);
> +
> +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>");
> +MODULE_DESCRIPTION("CXL Type2 device support, driver test");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(CXL);
> +MODULE_DEVICE_TABLE(pci, type2_pci_table);

Re: [RFC PATCH 04/13] cxl: allow devices without mailbox capability

[Jonathan Cameron]

On Thu, 16 May 2024 09:11:53 +0100
<alucerop@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> A device not implementing the CXL memory-device class code, aka Type2
> devices, has mailbox capability as optional. If the device registers
> mapping does not show such capability, do not treat that as an error.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
If you are removing this check from the shared code, it needs to be
in the more specific code, or you need to state why that isn't
a problem (i.e. it is caught later anyway)

Jonathan

> ---
>  drivers/cxl/core/regs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index fd165e718cf2..5434a7c899fd 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -437,11 +437,10 @@ static int cxl_probe_regs(struct cxl_register_map *map)
>  	case CXL_REGLOC_RBI_MEMDEV:
>  		dev_map = &map->device_map;
>  		cxl_probe_device_regs(host, base, dev_map);
> -		if (!dev_map->status.valid || !dev_map->mbox.valid ||
> +		if (!dev_map->status.valid ||
>  		    !dev_map->memdev.valid) {
> -			dev_err(host, "registers not found: %s%s%s\n",
> +			dev_err(host, "registers not found: %s%s\n",
>  				!dev_map->status.valid ? "status " : "",
> -				!dev_map->mbox.valid ? "mbox " : "",
>  				!dev_map->memdev.valid ? "memdev " : "");
>  			return -ENXIO;
>  		}

Re: [RFC PATCH 05/13] cxl: fix check about pmem resource

[Jonathan Cameron]

On Thu, 16 May 2024 09:11:54 +0100
<alucerop@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Current check is using resource_size which counts on a range bigger than
> 0. For a resource with start and end being 0, resource_size returns 1
> and implying a false positive. Use the end not being zero as the new check.
> 
> Note: If I´m not missing anything here, this should be extended to the
> whole linux kernel where resource_size is being used in conditionals,
> and where the likely right fix is to modify resource_size itself
> checking for the range not being 0.

The start and end being 0 is a valid resource of length 1 so that
should not need fixing.

These should have been initialized with DEFINE_RES_MEM()
/ DEFINE_RES_NAMED()
That will happily set the .end to -1 resulting in a wrap around
so that you get all bits set.

Sometimes this gives slightly confusing range prints but otherwise
I think it is fine.

> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/hdm.c    | 4 ++--
>  drivers/cxl/core/memdev.c | 8 ++++----
>  drivers/cxl/mem.c         | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 47d9faf5897f..c5f70741d70a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -432,12 +432,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>  	 * Only allow modes that are supported by the current partition
>  	 * configuration
>  	 */
> -	if (mode == CXL_DECODER_PMEM && !resource_size(&cxlds->pmem_res)) {
> +	if (mode == CXL_DECODER_PMEM && !cxlds->pmem_res.end) {
>  		dev_dbg(dev, "no available pmem capacity\n");
>  		rc = -ENXIO;
>  		goto out;
>  	}
> -	if (mode == CXL_DECODER_RAM && !resource_size(&cxlds->ram_res)) {
> +	if (mode == CXL_DECODER_RAM && !cxlds->ram_res.end) {
>  		dev_dbg(dev, "no available ram capacity\n");
>  		rc = -ENXIO;
>  		goto out;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 0336b3f14f4a..b61d57d0d4f4 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -197,14 +197,14 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
>  	int rc = 0;
>  
>  	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
> -	if (resource_size(&cxlds->pmem_res)) {
> +	if (cxlds->pmem_res.end) {
>  		offset = cxlds->pmem_res.start;
>  		length = resource_size(&cxlds->pmem_res);
>  		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
>  		if (rc)
>  			return rc;
>  	}
> -	if (resource_size(&cxlds->ram_res)) {
> +	if (cxlds->ram_res.end) {
>  		offset = cxlds->ram_res.start;
>  		length = resource_size(&cxlds->ram_res);
>  		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> @@ -266,7 +266,7 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>  		return 0;
>  
>  	cxled = to_cxl_endpoint_decoder(dev);
> -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
> +	if (!cxled->dpa_res || !cxled->dpa_res->end)
>  		return 0;
>  
>  	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
> @@ -302,7 +302,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
>  	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>  		return 0;
>  
> -	if (!resource_size(&cxlds->dpa_res)) {
> +	if (!cxlds->dpa_res.end) {
>  		dev_dbg(cxlds->dev, "device has no dpa resource\n");
>  		return -EINVAL;
>  	}
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 6dc2bf1e2b1a..a168343d2d4d 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -174,7 +174,7 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
> +	if (cxlds->pmem_res.end && IS_ENABLED(CONFIG_CXL_PMEM)) {
>  		rc = devm_cxl_add_nvdimm(cxlmd);
>  		if (rc == -ENODEV)
>  			dev_info(dev, "PMEM disabled by platform\n");

Re: [RFC PATCH 00/13] RFC: add Type2 device support

[Alejandro Lucero Palau]

On 5/17/24 01:08, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com>
>>
>> I need to start this RFC explaining not what the patchset does, but what we
>> expected to obtain and currently is under doubt: to configure a CXL memory
>> range advertised by our CXL network device and to use it "privately".
>>
>> The main reason behind that privacy, but not the only one, is to avoid any
>> arbitrary use by any user-space "client" with enough privileges for using a
>> public interface to map the device memory and use it as regular memory. The
>> reason is obvious: the device expects writes to such a memory in a specific
>> format.
>>
>> The doubt comes from the fact that after implementing the functionality exposed
>> in this patchset, we realized the current expectation seems to be a BIOS/UEFI
>> configuring HDM decoders or HPA ranges and passing memory ranges to the kernel,
>> and with the kernel having a default action on that memory range based on the
>> flag EFI_MEMORY_SP being set or not.
> Lets clear up this confusion first.
>
> My expectation for CXL accelerator provided memory is that it is marked "EFI
> Reserved" in the memory map (or CDAT), not "EFI Conventional + Special Purpose
> Attribute". Recall that the EFI_MEMORY_SP attribute is just a *hint* that OS may
> *not* want to consider a memory range as part of the general purpose memory
> pool. CXL Accelerator Memory is far from that definition. If a driver needs to
> be loaded to effectively use a memory range that is not a range that can be
> transparently converted to "EFI Conventional Memory".

Well, that "EFI Reserved" is what we want. However, AFAIK, it is 
EFI_MEMORY_SP the only one being discussed with our BIOS/UEFI contacts.

Because how to assign those flags seems to be based on BIOS 
implementation, we count on some BIOS versions doing the wrong thing for 
our purposes, so I have been working on a workaround implying udev 
events and executing a modified daxctl for switching a dax device mode 
or even removing/destroying it.

Anyway, that "EFI Reserved" flag gives up hope.

>> If it is not set, the memory range will be part of the kernel memory
>> management, what we do not want for sure. If the flag is set, a DAX device
>> will be created which allows an user-space client to map such a memory through
>> the DAX device API, what we also prefer to avoid. I know this is likely going
>> to face opposition, but we see this RFC as the opportunity for discussing the
>> matter and, if it turns out to be the case, to be guided towards the proper
>> solution accepted by the maintainers/community. This patchset does not tackle
>> this default kernel behaviour although we already have some ideas and
>> workarounds for the short term. We'll be happy to discuss this openly.
> It may have been subtle, but even in my initial RFC [1]. I was explicitly
> skipping exposing accelerator memory via device-dax:


I did not see that. So, happy to know we are in the same page here.


> @@ -3085,6 +3183,15 @@ static int cxl_region_probe(struct device *dev)
>   					p->res->start, p->res->end, cxlr,
>   					is_system_ram) > 0)
>   			return 0;
> +
> +		/*
> +		 * HDM-D[B] (device-memory) regions have accelerator
> +		 * specific usage, skip device-dax registration.
> +		 */
> +		if (cxlr->type == CXL_DECODER_DEVMEM)
> +			return 0;
> +
> +		/* HDM-H routes to device-dax */
>   		return devm_cxl_add_dax_region(cxlr);
>   	default:
>   		dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
>
> [1] https://lore.kernel.org/r/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com
>
>> So, this patchset assumes a BIOS/UEFI not programming the HDM decoder or HPA
>> ranges for a CXL Type2 device.
> At least for Linux it should not care whether BIOS maps it or not. The
> expectation is that whichever agent maps it, BIOS or Linux CXL core, that agent
> honors the memory type specified in the device CDAT. I.e. the "Device Scoped EFI
> Memory Type Structure (DSEMTS)" in the accelerator CDAT should pass "2" in the
> "EFI Memory Type and Attribute" field, where "2" indicates "EFI Reserved" memory
> type for any HPA that maps this device's DPA.
>
>> above, a Type2 device added after boot, that is hotplugged, will likely need the
>> changes added here. Exporting some of the CXL core for vendor drivers is also
>> required for avoiding code duplication when reading DVSEC or decoder registers.
>> Finally if there is no such HPA range assigned, whatever the reason, this patchset
>> offers some way of obtaining one and/or finding out what is the problem behind the
>> lack of such HPA range.
> Yes, that was the whole point of the "Device Memory Setup" thread [2] to show a
> plausible way to reuse the common core infrastructure for accelerators with
> CXL.mem capability.
>
> [2] https://lore.kernel.org/all/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/
>
> It would help me if you can summarize what you did and did not adopt from that
> proposal, i.e. where it helped and where it missed the mark for your use case.


I basically took your patchset referenced in the RFC, specifically the 
19th one, and tried to work with it from a pcie/CXL device driver. I did 
debug all the problems precluding the CXL invoked function to succeed 
and applying those changes from previous patches required and not 
committed yet. Patches 7, 8, 9 and 11 are based on your patchset, with 
just some minor change since I did not see a reason for changing them 
after studying them. The other patches are those fixes for having a 
Type2 finally able to create a dax region and map it.

> [..]
>> Keeping with kernel rules of having a client using any new functionality added,
>> a CXL type2 driver is increasingly added. This is not a driver supporting a real
>> device but an emulated one in QEMU, with the QEMU patch following this patchset.
> Lets start with the deltas compared to [2], the observation there is that much
> of the CXL core is reusable for this type-2 use case, and even improves the
> organization of the core. The consumer of the refactoring just ends up being a
> few new helpers around the core.
>
>> The reason for adding such a Type2 driver instead of changes to a current kernel
>> driver or a new driver is threefold:
>>
>> 1) the expected kernel driver to use the functionality added is a netdev one.
>>     Current internal CXL support is a codesign effort, therefore software and
>>     hardware evolving in lockstep. Adding changes to a netdev driver requires the
>>     full functionality and doing things following the netdev standard which is
>>     not the best option for this development stage.
> Not sure what "netdev standard" means, and not sure it matters for a discussion
> that is constrained to how the CXL core should evolve to accommodate
> accelerator-local memory.

You know I could not add to a netdev driver just those changes for CXL 
initialization, then mapping a CXL region and doing nothing else. Such a 
mapping needs to be linked to something inside the driver what is just 
under development and testing.

>> 2) Waiting for completing the development will delay the required Type2 support,
>>     and most of the required changes are unrelated to specific CXL usage by any
>>     vendor driver.
> Again, [2] already path cleared the idea that early plumbing of the CXL core for
> type-2 is in scope. Just need to make sure this effort is focused on general CXL
> specification use cases.
>
>> 3) Type2 support will need some testing infrastructure, unit tests for ensuring
>>     Type2 devices are working, and module tests for ensuring CXL core changes do
>>     not affect Type2 support.
>>
>> I hope these reasons are convincing enough.
> I am happy to have general specification discussions about core functionality
> that all CXL accelerators need and plumbing those ABIs to be exercised via
> something like cxl_test.
>
> I expect that cxl_test only covers coarse grained inter-module ABIs and that the
> fine details will need to wait until the accelerator specifics are closer to
> being disclosed and the real driver patches can start flowing. In other words, I
> am not keen to see QEMU used as a way to proxy features into the kernel without
> disclosing what the real world consumer actually needs.

Sure. FWIW, this will end up changing a internal driver mapping HW 
buffers which are now based on PCI BAR offsets by same mapping based on 
CXL region mapping.

This involves, PFs and VFs, and because CXL is only advertised by PF0, 
this all brings more complexity to those changes than could be expected.

I think taking the CXL core to the support needed, hopefully based on 
this RFC, should be started as soon as possible instead of waiting for 
our driver/device to be ready.

>
> Now, that said, there may also be opportunity for targeted extensions of the
> QEMU CXL memory-expander device, like DPA capacity that is mapped as "EFI
> Reserved" device-memory, but lets talk about the specifics here.
Yes. I'm actually using qemu hmem and kernel efi_fake_mem for some 
testing about those workarounds, so happy to help with new qemu support 
for the "EFI Reserved" case as well.
> Also recall that the current CXL driver does not yet support parsing CXL PMEM
> region labels. Support for that ends up looking quite similar to an accelerator
> asking for a specific DPA range to be dynamically mapped by the CXL core. So I
> think we can get quite far along the path to enabling type-2-CXL.mem just by
> flushing out some of the generic type-3-CXL.mem use cases.
OK. I'll take a look to those pmem needs.
>> I have decided to follow a gradual approach for adding such a driver using the
>> exported CXL functions and structs. I think it is easier to review the driver
>> when the new funcionality is added than to add the driver at the end, but not a
>> big deal if my approach is not liked.
>>
>> The patches are based on a patchset sent by Dan Williams [1] which was just
>> partially integrated, most related to making things ready for Type2 but none
>> related to specific Type2 support. Those patches based on Dan´s work have Dan´s
>> signing, so Dan, tell me if you do not want me to add you.
> It is fine to reuse those patches, but please do include:
>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Link: <lore-link for copied code>

Ok. I'll do the changes for next versions.


> ...so I can try to remember if I still think the source patch was a good idea or
> not. Again, a summary analysis of what did and did not work for you from that
> posting would help me get a running start at reviewing this.
>
>> Type2 implies, I think, only the related driver to manage the CXL specifics.
>> This means no user space intervention and therefore no sysfs files. This makes
>> easy to avoid the current problem of most of the sysfs related code expecting
>> Type3 devices. If I´m wrong in this regard, such a code will need further
>> changes.
> Not sure what you mean here, I am hoping that sysfs ABI for enumerating objects
> like memdevs, decoders, and regions "just works" and is hidden from the
> accelerator vendor driver.

If a Type2 is fully private, should be any sysfs files about it?

AFAIK, those files are mainly for user space able to create memories 
joining them, or for changing the dax mode. What do you think a Type2 
needs through sysfs apart from info? While writing this I realize I have 
not thought about power management ...

>
>> A final note about CXL.cache is needed. This patchset does not cover it at all,
>> although the emulated Type2 device advertises it. From the kernel point of view
>> supporting CXL.cache will imply to be sure the CXL path supports what the Type2
>> device needs. A device accelerator will likely be connected to a Root Switch,
>> but other configurations can not be discarded. Therefore the kernel will need to
>> check not just HPA, DPA, interleave and granularity, but also the available
>> CXL.cache support and resources in each switch in the CXL path to the Type2
>> device. I expect to contribute to this support in the following months, and
>> it would be good to discuss about it when possible.
> Yup, especially CXL 3.x Cache ID management. Lets talk about some ways to plumb
> that, because it is also a general CXL specification capability. Another topic
> is what is needed for CXL Protocol and Component error handling, I expect some
> of that to be CXL core common and some of that to be accelertor driver
> augmented.
>
> If you think we will still be talking about this driver enabling in September
> I would invite you to submit this as a topic for the CXL uConf.

That would be great!

If this RFC work is integrated by then with the CXL core, that would be 
fantastic, but the CXL.cache support will still be pending.

>
> https://lpc.events/event/18/contributions/1673/

Re: [RFC PATCH 05/13] cxl: fix check about pmem resource

[Alejandro Lucero Palau]

On 5/17/24 15:40, Jonathan Cameron wrote:
> On Thu, 16 May 2024 09:11:54 +0100
> <alucerop@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Current check is using resource_size which counts on a range bigger than
>> 0. For a resource with start and end being 0, resource_size returns 1
>> and implying a false positive. Use the end not being zero as the new check.
>>
>> Note: If I´m not missing anything here, this should be extended to the
>> whole linux kernel where resource_size is being used in conditionals,
>> and where the likely right fix is to modify resource_size itself
>> checking for the range not being 0.
> The start and end being 0 is a valid resource of length 1 so that
> should not need fixing.


Uhmmm ... I have problems understanding this but I guess there's a good 
reason for it.


> These should have been initialized with DEFINE_RES_MEM()
> / DEFINE_RES_NAMED()
> That will happily set the .end to -1 resulting in a wrap around
> so that you get all bits set.


That makes sense, but I wonder if we should have some function for doing 
default initialization of those fields like this which some users, like 
an accelerator with no pmem, will likely leave untouched.

Maybe inside cxl_accel_state_create in patch2 something like this:

     cxlds->dpa_res = DEFINE_RES(0, -1);

     cxlds->ram_res = DEFINE_RES(0, -1);

     cxlds->pmem_res = DEFINE_RES(0, -1);


>
> Sometimes this gives slightly confusing range prints but otherwise
> I think it is fine.
>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/hdm.c    | 4 ++--
>>   drivers/cxl/core/memdev.c | 8 ++++----
>>   drivers/cxl/mem.c         | 2 +-
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 47d9faf5897f..c5f70741d70a 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -432,12 +432,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>>   	 * Only allow modes that are supported by the current partition
>>   	 * configuration
>>   	 */
>> -	if (mode == CXL_DECODER_PMEM && !resource_size(&cxlds->pmem_res)) {
>> +	if (mode == CXL_DECODER_PMEM && !cxlds->pmem_res.end) {
>>   		dev_dbg(dev, "no available pmem capacity\n");
>>   		rc = -ENXIO;
>>   		goto out;
>>   	}
>> -	if (mode == CXL_DECODER_RAM && !resource_size(&cxlds->ram_res)) {
>> +	if (mode == CXL_DECODER_RAM && !cxlds->ram_res.end) {
>>   		dev_dbg(dev, "no available ram capacity\n");
>>   		rc = -ENXIO;
>>   		goto out;
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 0336b3f14f4a..b61d57d0d4f4 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -197,14 +197,14 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
>>   	int rc = 0;
>>   
>>   	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
>> -	if (resource_size(&cxlds->pmem_res)) {
>> +	if (cxlds->pmem_res.end) {
>>   		offset = cxlds->pmem_res.start;
>>   		length = resource_size(&cxlds->pmem_res);
>>   		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
>>   		if (rc)
>>   			return rc;
>>   	}
>> -	if (resource_size(&cxlds->ram_res)) {
>> +	if (cxlds->ram_res.end) {
>>   		offset = cxlds->ram_res.start;
>>   		length = resource_size(&cxlds->ram_res);
>>   		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
>> @@ -266,7 +266,7 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>>   		return 0;
>>   
>>   	cxled = to_cxl_endpoint_decoder(dev);
>> -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
>> +	if (!cxled->dpa_res || !cxled->dpa_res->end)
>>   		return 0;
>>   
>>   	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
>> @@ -302,7 +302,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
>>   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>   		return 0;
>>   
>> -	if (!resource_size(&cxlds->dpa_res)) {
>> +	if (!cxlds->dpa_res.end) {
>>   		dev_dbg(cxlds->dev, "device has no dpa resource\n");
>>   		return -EINVAL;
>>   	}
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 6dc2bf1e2b1a..a168343d2d4d 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -174,7 +174,7 @@ static int cxl_mem_probe(struct device *dev)
>>   	if (rc)
>>   		return rc;
>>   
>> -	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
>> +	if (cxlds->pmem_res.end && IS_ENABLED(CONFIG_CXL_PMEM)) {
>>   		rc = devm_cxl_add_nvdimm(cxlmd);
>>   		if (rc == -ENODEV)
>>   			dev_info(dev, "PMEM disabled by platform\n");

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Alejandro Lucero Palau]

On 5/17/24 15:30, Jonathan Cameron wrote:
> On Thu, 16 May 2024 09:11:51 +0100
> <alucerop@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Differientiating Type3, aka memory expanders, from Type2, aka device
>> accelerators, with a new function for initializing cxl_dev_state.
>>
>> Adding a type2 driver for a CXL emulated device inside CXL kernel
>> testing infrastructure as a client for the functionality added.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> If you are going to keep Dan's sign-off it needs to be clear why.
> Either put the author to Dan and swap these two, or use a
> Co-developed tag.


Yes, Dan commented on this an he prefers the co-developed flag, so I'll 
change to that.


> A few drive my trivial comments.
>
>> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
>> new file mode 100644
>> index 000000000000..863ce7dc28ef
>> --- /dev/null
>> +++ b/tools/testing/cxl/type2/pci_type2.c
>> @@ -0,0 +1,80 @@
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/cxl.h>
>> +#include <linux/cxlpci.h>
>> +#include <linux/cxlmem.h>
>> +
>> +struct cxl_dev_state *cxlds;
>> +
>> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
>> +
>> +static int type2_pci_probe(struct pci_dev *pci_dev,
>> +			   const struct pci_device_id *entry)
>> +
>> +{
>> +	u16 dvsec;
>> +
>> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> Add a line break somewhere in there as no real reason it needs to be all in eonline.


Sure.


>> +
> Trivial: Drop this blank line to keep error check associated with the call.


OK


>> +	if (!dvsec) {
>> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
>> +		return 0;
> Returned, so no point in the else.


Right.


>> +	} else {
>> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
>> +	}
>> +
>> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
>> +	if (IS_ERR(cxlds))
>> +		return PTR_ERR(cxlds);
>> +
>> +	pci_info(pci_dev, "Initializing cxlds...");
>> +	cxlds->cxl_dvsec = dvsec;
>> +	cxlds->serial = pci_dev->dev.id;
>> +
>> +	/* Should not this be based on DVSEC range size registers */
>> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
>> +
>> +	return 0;
>> +}
>> +
>> +static void type2_pci_remove(struct pci_dev *pci_dev)
>> +{
>> +
>> +}
>> +
>> +/* PCI device ID table */
>> +static const struct pci_device_id type2_pci_table[] = {
>> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
>> +	{0}                     /* end of list */
> {} is more common.


OK


>> +};
>> +
>> +static struct pci_driver type2_pci_driver = {
>> +	.name           = KBUILD_MODNAME,
>> +	.id_table       = type2_pci_table,
>> +	.probe          = type2_pci_probe,
>> +	.remove         = type2_pci_remove,
> Add remove only when it does something.


OK


>> +};
>> +
>> +static int __init type2_cxl_init(void)
>> +{
>> +	int rc;
>> +
>> +	rc = pci_register_driver(&type2_pci_driver);
>> +
>> +	return rc;
>> +}
>> +
>> +static void __exit type2_cxl_exit(void)
>> +{
>> +	pci_unregister_driver(&type2_pci_driver);
> Unless you are adding more in here later in the series there is a macro
> to cover all this. module_pci_driver()
>

I did not know about it. I'll use it.


Thanks


>> +}
>> +
>> +module_init(type2_cxl_init);
>> +module_exit(type2_cxl_exit);
>> +
>> +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>");
>> +MODULE_DESCRIPTION("CXL Type2 device support, driver test");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS(CXL);
>> +MODULE_DEVICE_TABLE(pci, type2_pci_table);

Re: [RFC PATCH 04/13] cxl: allow devices without mailbox capability

[Alejandro Lucero Palau]

On 5/17/24 15:33, Jonathan Cameron wrote:
> On Thu, 16 May 2024 09:11:53 +0100
> <alucerop@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> A device not implementing the CXL memory-device class code, aka Type2
>> devices, has mailbox capability as optional. If the device registers
>> mapping does not show such capability, do not treat that as an error.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> If you are removing this check from the shared code, it needs to be
> in the more specific code, or you need to state why that isn't
> a problem (i.e. it is caught later anyway)


Yes, I realize this is solving the Type2 problem but impacting on 
current behavior for Type3.

I'll change it for doing it without such impact.

Thanks.


>
> Jonathan
>
>> ---
>>   drivers/cxl/core/regs.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index fd165e718cf2..5434a7c899fd 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -437,11 +437,10 @@ static int cxl_probe_regs(struct cxl_register_map *map)
>>   	case CXL_REGLOC_RBI_MEMDEV:
>>   		dev_map = &map->device_map;
>>   		cxl_probe_device_regs(host, base, dev_map);
>> -		if (!dev_map->status.valid || !dev_map->mbox.valid ||
>> +		if (!dev_map->status.valid ||
>>   		    !dev_map->memdev.valid) {
>> -			dev_err(host, "registers not found: %s%s%s\n",
>> +			dev_err(host, "registers not found: %s%s\n",
>>   				!dev_map->status.valid ? "status " : "",
>> -				!dev_map->mbox.valid ? "mbox " : "",
>>   				!dev_map->memdev.valid ? "memdev " : "");
>>   			return -ENXIO;
>>   		}

Re: [RFC PATCH 00/13] RFC: add Type2 device support

[Dan Williams]

Alejandro Lucero Palau wrote:
> On 5/17/24 01:08, Dan Williams wrote:
[..]
> > My expectation for CXL accelerator provided memory is that it is marked "EFI
> > Reserved" in the memory map (or CDAT), not "EFI Conventional + Special Purpose
> > Attribute". Recall that the EFI_MEMORY_SP attribute is just a *hint* that OS may
> > *not* want to consider a memory range as part of the general purpose memory
> > pool. CXL Accelerator Memory is far from that definition. If a driver needs to
> > be loaded to effectively use a memory range that is not a range that can be
> > transparently converted to "EFI Conventional Memory".
> 
> Well, that "EFI Reserved" is what we want. However, AFAIK, it is 
> EFI_MEMORY_SP the only one being discussed with our BIOS/UEFI contacts.

Well, tell your BIOS contacts that they need to honor what the device
puts in the CDAT for the memory-type, not just make it up on their own.

From the Linux side we can scream loudly with
WARN_TAINT_FIRMWARE_WORKAROUND every time we see a BIOS that has failed
to match EFI memory-type with whatever the device puts in the CDAT,
because that's clearly a broken BIOS.

> Because how to assign those flags seems to be based on BIOS 
> implementation, we count on some BIOS versions doing the wrong thing for 
> our purposes, so I have been working on a workaround implying udev 
> events and executing a modified daxctl for switching a dax device mode 
> or even removing/destroying it.

Yikes, no, lets not do that. Another thing we can do from the kernel
side, in case the CDAT is missing, is to never attach device-dax to
address ranges within CXL windows with the "Device Coherent Window
Restriction".

[..]
> > It would help me if you can summarize what you did and did not adopt from that
> > proposal, i.e. where it helped and where it missed the mark for your use case.
> 
> I basically took your patchset referenced in the RFC, specifically the 
> 19th one, and tried to work with it from a pcie/CXL device driver. I did 
> debug all the problems precluding the CXL invoked function to succeed 
> and applying those changes from previous patches required and not 
> committed yet. Patches 7, 8, 9 and 11 are based on your patchset, with 
> just some minor change since I did not see a reason for changing them 
> after studying them. The other patches are those fixes for having a 
> Type2 finally able to create a dax region and map it.

Ok, let's not add dax region's for this case, but the rest sounds
straightforward, I'll take a look.

[..]
> >> The reason for adding such a Type2 driver instead of changes to a current kernel
> >> driver or a new driver is threefold:
> >>
> >> 1) the expected kernel driver to use the functionality added is a netdev one.
> >>     Current internal CXL support is a codesign effort, therefore software and
> >>     hardware evolving in lockstep. Adding changes to a netdev driver requires the
> >>     full functionality and doing things following the netdev standard which is
> >>     not the best option for this development stage.
> > Not sure what "netdev standard" means, and not sure it matters for a discussion
> > that is constrained to how the CXL core should evolve to accommodate
> > accelerator-local memory.
> 
> You know I could not add to a netdev driver just those changes for CXL 
> initialization, then mapping a CXL region and doing nothing else. Such a 
> mapping needs to be linked to something inside the driver what is just 
> under development and testing.

Ok, you're just talking about pre-plumbing the CXL bits before getting
started on the netdev side. Makes sense to me.

> >> 2) Waiting for completing the development will delay the required Type2 support,
> >>     and most of the required changes are unrelated to specific CXL usage by any
> >>     vendor driver.
> > Again, [2] already path cleared the idea that early plumbing of the CXL core for
> > type-2 is in scope. Just need to make sure this effort is focused on general CXL
> > specification use cases.
> >
> >> 3) Type2 support will need some testing infrastructure, unit tests for ensuring
> >>     Type2 devices are working, and module tests for ensuring CXL core changes do
> >>     not affect Type2 support.
> >>
> >> I hope these reasons are convincing enough.
> > I am happy to have general specification discussions about core functionality
> > that all CXL accelerators need and plumbing those ABIs to be exercised via
> > something like cxl_test.
> >
> > I expect that cxl_test only covers coarse grained inter-module ABIs and that the
> > fine details will need to wait until the accelerator specifics are closer to
> > being disclosed and the real driver patches can start flowing. In other words, I
> > am not keen to see QEMU used as a way to proxy features into the kernel without
> > disclosing what the real world consumer actually needs.
> 
> Sure. FWIW, this will end up changing a internal driver mapping HW 
> buffers which are now based on PCI BAR offsets by same mapping based on 
> CXL region mapping.
> 
> This involves, PFs and VFs, and because CXL is only advertised by PF0, 
> this all brings more complexity to those changes than could be expected.
> 
> I think taking the CXL core to the support needed, hopefully based on 
> this RFC, should be started as soon as possible instead of waiting for 
> our driver/device to be ready.

I agree, but continued transparency on what the CXL use case looks like
in the final implementation helps make sure the right upstream CXL
mechanism is being built.

For example, just from that insight above I am wondering if we need some
centralized interface for VFs to lookup / allocate CXL resources from
their PF0.

[..]
> >> Type2 implies, I think, only the related driver to manage the CXL specifics.
> >> This means no user space intervention and therefore no sysfs files. This makes
> >> easy to avoid the current problem of most of the sysfs related code expecting
> >> Type3 devices. If I´m wrong in this regard, such a code will need further
> >> changes.
> > Not sure what you mean here, I am hoping that sysfs ABI for enumerating objects
> > like memdevs, decoders, and regions "just works" and is hidden from the
> > accelerator vendor driver.
> 
> If a Type2 is fully private, should be any sysfs files about it?
> 
> AFAIK, those files are mainly for user space able to create memories 
> joining them, or for changing the dax mode. What do you think a Type2 
> needs through sysfs apart from info? While writing this I realize I have 
> not thought about power management ...

They are primarily for userspace to enumerate resources, and they
provide kernel object names for CXL error events. For example userspace
could generically map a CXL switch error to impacted endpoints without
needing to know or care whether it was a type-2 or type-3 endpoint.

[..]
> > If you think we will still be talking about this driver enabling in September
> > I would invite you to submit this as a topic for the CXL uConf.
> 
> That would be great!
> 
> If this RFC work is integrated by then with the CXL core, that would be 
> fantastic, but the CXL.cache support will still be pending.

Sounds good.

Re: [RFC PATCH 00/13] RFC: add Type2 device support

[Alejandro Lucero Palau]

On 5/21/24 05:56, Dan Williams wrote:
> Alejandro Lucero Palau wrote:
>> On 5/17/24 01:08, Dan Williams wrote:
> [..]
>>> My expectation for CXL accelerator provided memory is that it is marked "EFI
>>> Reserved" in the memory map (or CDAT), not "EFI Conventional + Special Purpose
>>> Attribute". Recall that the EFI_MEMORY_SP attribute is just a *hint* that OS may
>>> *not* want to consider a memory range as part of the general purpose memory
>>> pool. CXL Accelerator Memory is far from that definition. If a driver needs to
>>> be loaded to effectively use a memory range that is not a range that can be
>>> transparently converted to "EFI Conventional Memory".
>> Well, that "EFI Reserved" is what we want. However, AFAIK, it is
>> EFI_MEMORY_SP the only one being discussed with our BIOS/UEFI contacts.
> Well, tell your BIOS contacts that they need to honor what the device
> puts in the CDAT for the memory-type, not just make it up on their own.
>
>  From the Linux side we can scream loudly with
> WARN_TAINT_FIRMWARE_WORKAROUND every time we see a BIOS that has failed
> to match EFI memory-type with whatever the device puts in the CDAT,
> because that's clearly a broken BIOS.


Yes, we need to sync with these guys :-)

EFI_MEMORY_SP was our/my only focus and that is probably the problem and 
the confusion.

BTW, How can the kernel warn about this? I guess this requires the 
driver raising it. doesn't it?


>> Because how to assign those flags seems to be based on BIOS
>> implementation, we count on some BIOS versions doing the wrong thing for
>> our purposes, so I have been working on a workaround implying udev
>> events and executing a modified daxctl for switching a dax device mode
>> or even removing/destroying it.
> Yikes, no, lets not do that. Another thing we can do from the kernel
> side, in case the CDAT is missing, is to never attach device-dax to
> address ranges within CXL windows with the "Device Coherent Window
> Restriction".


I do not want to put my reputation on this workaround, but I see it as a 
first and quick fallback and not requiring kernel changes to CXL/DAX. My 
idea is to add some interface between drivers and DAX for doing this if 
necessary, once the driver tries to get the resource and it fails. Maybe 
with some kernel boot param like dax_allow_change=drivername or 
something similar. What else could we do for the case of the BIOS/kernel 
doing the wrong thing?


> [..]
>>> It would help me if you can summarize what you did and did not adopt from that
>>> proposal, i.e. where it helped and where it missed the mark for your use case.
>> I basically took your patchset referenced in the RFC, specifically the
>> 19th one, and tried to work with it from a pcie/CXL device driver. I did
>> debug all the problems precluding the CXL invoked function to succeed
>> and applying those changes from previous patches required and not
>> committed yet. Patches 7, 8, 9 and 11 are based on your patchset, with
>> just some minor change since I did not see a reason for changing them
>> after studying them. The other patches are those fixes for having a
>> Type2 finally able to create a dax region and map it.
> Ok, let's not add dax region's for this case, but the rest sounds
> straightforward, I'll take a look.
>
> [..]
>>>> The reason for adding such a Type2 driver instead of changes to a current kernel
>>>> driver or a new driver is threefold:
>>>>
>>>> 1) the expected kernel driver to use the functionality added is a netdev one.
>>>>      Current internal CXL support is a codesign effort, therefore software and
>>>>      hardware evolving in lockstep. Adding changes to a netdev driver requires the
>>>>      full functionality and doing things following the netdev standard which is
>>>>      not the best option for this development stage.
>>> Not sure what "netdev standard" means, and not sure it matters for a discussion
>>> that is constrained to how the CXL core should evolve to accommodate
>>> accelerator-local memory.
>> You know I could not add to a netdev driver just those changes for CXL
>> initialization, then mapping a CXL region and doing nothing else. Such a
>> mapping needs to be linked to something inside the driver what is just
>> under development and testing.
> Ok, you're just talking about pre-plumbing the CXL bits before getting
> started on the netdev side. Makes sense to me.
>
>>>> 2) Waiting for completing the development will delay the required Type2 support,
>>>>      and most of the required changes are unrelated to specific CXL usage by any
>>>>      vendor driver.
>>> Again, [2] already path cleared the idea that early plumbing of the CXL core for
>>> type-2 is in scope. Just need to make sure this effort is focused on general CXL
>>> specification use cases.
>>>
>>>> 3) Type2 support will need some testing infrastructure, unit tests for ensuring
>>>>      Type2 devices are working, and module tests for ensuring CXL core changes do
>>>>      not affect Type2 support.
>>>>
>>>> I hope these reasons are convincing enough.
>>> I am happy to have general specification discussions about core functionality
>>> that all CXL accelerators need and plumbing those ABIs to be exercised via
>>> something like cxl_test.
>>>
>>> I expect that cxl_test only covers coarse grained inter-module ABIs and that the
>>> fine details will need to wait until the accelerator specifics are closer to
>>> being disclosed and the real driver patches can start flowing. In other words, I
>>> am not keen to see QEMU used as a way to proxy features into the kernel without
>>> disclosing what the real world consumer actually needs.
>> Sure. FWIW, this will end up changing a internal driver mapping HW
>> buffers which are now based on PCI BAR offsets by same mapping based on
>> CXL region mapping.
>>
>> This involves, PFs and VFs, and because CXL is only advertised by PF0,
>> this all brings more complexity to those changes than could be expected.
>>
>> I think taking the CXL core to the support needed, hopefully based on
>> this RFC, should be started as soon as possible instead of waiting for
>> our driver/device to be ready.
> I agree, but continued transparency on what the CXL use case looks like
> in the final implementation helps make sure the right upstream CXL
> mechanism is being built.
>
> For example, just from that insight above I am wondering if we need some
> centralized interface for VFs to lookup / allocate CXL resources from
> their PF0.


Not in our case, but it could be needed (or convenient) for more complex 
devices regarding CXL ranges or decoders.


> [..]
>>>> Type2 implies, I think, only the related driver to manage the CXL specifics.
>>>> This means no user space intervention and therefore no sysfs files. This makes
>>>> easy to avoid the current problem of most of the sysfs related code expecting
>>>> Type3 devices. If I´m wrong in this regard, such a code will need further
>>>> changes.
>>> Not sure what you mean here, I am hoping that sysfs ABI for enumerating objects
>>> like memdevs, decoders, and regions "just works" and is hidden from the
>>> accelerator vendor driver.
>> If a Type2 is fully private, should be any sysfs files about it?
>>
>> AFAIK, those files are mainly for user space able to create memories
>> joining them, or for changing the dax mode. What do you think a Type2
>> needs through sysfs apart from info? While writing this I realize I have
>> not thought about power management ...
> They are primarily for userspace to enumerate resources, and they
> provide kernel object names for CXL error events. For example userspace
> could generically map a CXL switch error to impacted endpoints without
> needing to know or care whether it was a type-2 or type-3 endpoint.


That makes a lot of sense. Then I'm afraid I should work on this. I'll 
try to find out if it could be just modifying current patchset with 
minor changes.

Thanks


> [..]
>>> If you think we will still be talking about this driver enabling in September
>>> I would invite you to submit this as a topic for the CXL uConf.
>> That would be great!
>>
>> If this RFC work is integrated by then with the CXL core, that would be
>> fantastic, but the CXL.cache support will still be pending.
> Sounds good.
>

Re: [RFC PATCH 00/13] RFC: add Type2 device support

[Alejandro Lucero Palau]

ping

...

and some new comments below.


On 5/22/24 17:38, Alejandro Lucero Palau wrote:
>
> On 5/21/24 05:56, Dan Williams wrote:
>> Alejandro Lucero Palau wrote:
>>> On 5/17/24 01:08, Dan Williams wrote:
>> [..]
>>>> My expectation for CXL accelerator provided memory is that it is 
>>>> marked "EFI
>>>> Reserved" in the memory map (or CDAT), not "EFI Conventional + 
>>>> Special Purpose
>>>> Attribute". Recall that the EFI_MEMORY_SP attribute is just a 
>>>> *hint* that OS may
>>>> *not* want to consider a memory range as part of the general 
>>>> purpose memory
>>>> pool. CXL Accelerator Memory is far from that definition. If a 
>>>> driver needs to
>>>> be loaded to effectively use a memory range that is not a range 
>>>> that can be
>>>> transparently converted to "EFI Conventional Memory".
>>> Well, that "EFI Reserved" is what we want. However, AFAIK, it is
>>> EFI_MEMORY_SP the only one being discussed with our BIOS/UEFI contacts.
>> Well, tell your BIOS contacts that they need to honor what the device
>> puts in the CDAT for the memory-type, not just make it up on their own.
>>
>>  From the Linux side we can scream loudly with
>> WARN_TAINT_FIRMWARE_WORKAROUND every time we see a BIOS that has failed
>> to match EFI memory-type with whatever the device puts in the CDAT,
>> because that's clearly a broken BIOS.
>
>
> Yes, we need to sync with these guys :-)
>
> EFI_MEMORY_SP was our/my only focus and that is probably the problem 
> and the confusion.
>
> BTW, How can the kernel warn about this? I guess this requires the 
> driver raising it. doesn't it?
>
>
>>> Because how to assign those flags seems to be based on BIOS
>>> implementation, we count on some BIOS versions doing the wrong thing 
>>> for
>>> our purposes, so I have been working on a workaround implying udev
>>> events and executing a modified daxctl for switching a dax device mode
>>> or even removing/destroying it.
>> Yikes, no, lets not do that. Another thing we can do from the kernel
>> side, in case the CDAT is missing, is to never attach device-dax to
>> address ranges within CXL windows with the "Device Coherent Window
>> Restriction".
>
>
> I do not want to put my reputation on this workaround, but I see it as 
> a first and quick fallback and not requiring kernel changes to 
> CXL/DAX. My idea is to add some interface between drivers and DAX for 
> doing this if necessary, once the driver tries to get the resource and 
> it fails. Maybe with some kernel boot param like 
> dax_allow_change=drivername or something similar. What else could we 
> do for the case of the BIOS/kernel doing the wrong thing?
>
>
>> [..]
>>>> It would help me if you can summarize what you did and did not 
>>>> adopt from that
>>>> proposal, i.e. where it helped and where it missed the mark for 
>>>> your use case.
>>> I basically took your patchset referenced in the RFC, specifically the
>>> 19th one, and tried to work with it from a pcie/CXL device driver. I 
>>> did
>>> debug all the problems precluding the CXL invoked function to succeed
>>> and applying those changes from previous patches required and not
>>> committed yet. Patches 7, 8, 9 and 11 are based on your patchset, with
>>> just some minor change since I did not see a reason for changing them
>>> after studying them. The other patches are those fixes for having a
>>> Type2 finally able to create a dax region and map it.
>> Ok, let's not add dax region's for this case, but the rest sounds
>> straightforward, I'll take a look.
>>
>> [..]
>>>>> The reason for adding such a Type2 driver instead of changes to a 
>>>>> current kernel
>>>>> driver or a new driver is threefold:
>>>>>
>>>>> 1) the expected kernel driver to use the functionality added is a 
>>>>> netdev one.
>>>>>      Current internal CXL support is a codesign effort, therefore 
>>>>> software and
>>>>>      hardware evolving in lockstep. Adding changes to a netdev 
>>>>> driver requires the
>>>>>      full functionality and doing things following the netdev 
>>>>> standard which is
>>>>>      not the best option for this development stage.
>>>> Not sure what "netdev standard" means, and not sure it matters for 
>>>> a discussion
>>>> that is constrained to how the CXL core should evolve to accommodate
>>>> accelerator-local memory.
>>> You know I could not add to a netdev driver just those changes for CXL
>>> initialization, then mapping a CXL region and doing nothing else. 
>>> Such a
>>> mapping needs to be linked to something inside the driver what is just
>>> under development and testing.
>> Ok, you're just talking about pre-plumbing the CXL bits before getting
>> started on the netdev side. Makes sense to me.
>>
>>>>> 2) Waiting for completing the development will delay the required 
>>>>> Type2 support,
>>>>>      and most of the required changes are unrelated to specific 
>>>>> CXL usage by any
>>>>>      vendor driver.
>>>> Again, [2] already path cleared the idea that early plumbing of the 
>>>> CXL core for
>>>> type-2 is in scope. Just need to make sure this effort is focused 
>>>> on general CXL
>>>> specification use cases.
>>>>
>>>>> 3) Type2 support will need some testing infrastructure, unit tests 
>>>>> for ensuring
>>>>>      Type2 devices are working, and module tests for ensuring CXL 
>>>>> core changes do
>>>>>      not affect Type2 support.
>>>>>
>>>>> I hope these reasons are convincing enough.
>>>> I am happy to have general specification discussions about core 
>>>> functionality
>>>> that all CXL accelerators need and plumbing those ABIs to be 
>>>> exercised via
>>>> something like cxl_test.
>>>>
>>>> I expect that cxl_test only covers coarse grained inter-module ABIs 
>>>> and that the
>>>> fine details will need to wait until the accelerator specifics are 
>>>> closer to
>>>> being disclosed and the real driver patches can start flowing. In 
>>>> other words, I
>>>> am not keen to see QEMU used as a way to proxy features into the 
>>>> kernel without
>>>> disclosing what the real world consumer actually needs.
>>> Sure. FWIW, this will end up changing a internal driver mapping HW
>>> buffers which are now based on PCI BAR offsets by same mapping based on
>>> CXL region mapping.
>>>
>>> This involves, PFs and VFs, and because CXL is only advertised by PF0,
>>> this all brings more complexity to those changes than could be 
>>> expected.
>>>
>>> I think taking the CXL core to the support needed, hopefully based on
>>> this RFC, should be started as soon as possible instead of waiting for
>>> our driver/device to be ready.
>> I agree, but continued transparency on what the CXL use case looks like
>> in the final implementation helps make sure the right upstream CXL
>> mechanism is being built.
>>
>> For example, just from that insight above I am wondering if we need some
>> centralized interface for VFs to lookup / allocate CXL resources from
>> their PF0.
>
>
> Not in our case, but it could be needed (or convenient) for more 
> complex devices regarding CXL ranges or decoders.
>
>
>> [..]
>>>>> Type2 implies, I think, only the related driver to manage the CXL 
>>>>> specifics.
>>>>> This means no user space intervention and therefore no sysfs 
>>>>> files. This makes
>>>>> easy to avoid the current problem of most of the sysfs related 
>>>>> code expecting
>>>>> Type3 devices. If I´m wrong in this regard, such a code will need 
>>>>> further
>>>>> changes.
>>>> Not sure what you mean here, I am hoping that sysfs ABI for 
>>>> enumerating objects
>>>> like memdevs, decoders, and regions "just works" and is hidden from 
>>>> the
>>>> accelerator vendor driver.
>>> If a Type2 is fully private, should be any sysfs files about it?
>>>
>>> AFAIK, those files are mainly for user space able to create memories
>>> joining them, or for changing the dax mode. What do you think a Type2
>>> needs through sysfs apart from info? While writing this I realize I 
>>> have
>>> not thought about power management ...
>> They are primarily for userspace to enumerate resources, and they
>> provide kernel object names for CXL error events. For example userspace
>> could generically map a CXL switch error to impacted endpoints without
>> needing to know or care whether it was a type-2 or type-3 endpoint.
>
>
> That makes a lot of sense. Then I'm afraid I should work on this. I'll 
> try to find out if it could be just modifying current patchset with 
> minor changes.
>
> Thanks
>
>

I've been looking at this, mainly alarmed by the error handling problem 
mentioned.

Firstly, my original comment in the cover letter is misleading. The 
patchset does not avoid kobjects to be created just some sysfs files 
related to kobjects attributes.

I think it is the right thing to do, although some of those attributes 
could in fact be shown, so I will fix that. However, I think some fields 
now in cxl_memdev_state should be inside clx_memdev, like 
ram_perf/pmem_perf, but moreover, I think we need a more flexible way of 
linking Type2 to those structs where the optional Type2 functionalities 
can be set or not. Not something to be faced in this patchset but by for 
some refactoring in the near future if my concern turns out to be a real 
one.


Secondly, there is another concern, maybe a more important one, and it 
is related to the error handling. I have to admit I did not think about 
this at all before Dan's comment, and I think it is something worth to 
discuss.

There are, at least, two error situations I'm concern with, although 
they (can) end up with the same action: a pcie/CXL slot reset. Assuming 
there could be cases where the system can recover from this situation, 
and noting that CXL.mem or CXL.cache could stall cpus and likely 
crashing the system, the concern is how the CXL device configuration 
like HDM should be done after the slot reset. While a FLR keeps that 
config, a slot reset does not. The current Type3 support inside the CXL 
pci driver does nothing when the device is detached (no release 
callback), what is happening when the system performs the slot reset, 
and cxl_error_resume is not trying anything. If I'm not wrong, it is in 
the resume callback where the device is probed again.

So here is my question: should not an endpoint slot reset be supported?


>> [..]
>>>> If you think we will still be talking about this driver enabling in 
>>>> September
>>>> I would invite you to submit this as a topic for the CXL uConf.
>>> That would be great!
>>>
>>> If this RFC work is integrated by then with the CXL core, that would be
>>> fantastic, but the CXL.cache support will still be pending.
>> Sounds good.
>>

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Dan Williams]

alucerop@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> CXL Type 2 devices imply specific vendor drivers binding to those
> devices instead of generic ones offered by CXL core like the PCI driver.
> Those drivers need to use CXL core functions and structs for
> initialization, create memdevs and create CXL regions.
> 
> This patch avoids referencing those files based on relative paths from
> inside the kernel sources tree.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/acpi.c                      | 4 ++--
>  drivers/cxl/core/cdat.c                 | 6 +++---
>  drivers/cxl/core/hdm.c                  | 2 +-
>  drivers/cxl/core/mbox.c                 | 6 +++---
>  drivers/cxl/core/memdev.c               | 2 +-
>  drivers/cxl/core/pci.c                  | 6 +++---
>  drivers/cxl/core/pmem.c                 | 4 ++--
>  drivers/cxl/core/pmu.c                  | 4 ++--
>  drivers/cxl/core/port.c                 | 6 +++---
>  drivers/cxl/core/region.c               | 4 ++--
>  drivers/cxl/core/regs.c                 | 4 ++--
>  drivers/cxl/core/suspend.c              | 2 +-
>  drivers/cxl/core/trace.c                | 2 +-
>  drivers/cxl/core/trace.h                | 4 ++--
>  drivers/cxl/mem.c                       | 4 ++--
>  drivers/cxl/pci.c                       | 6 +++---
>  drivers/cxl/pmem.c                      | 4 ++--
>  drivers/cxl/port.c                      | 4 ++--
>  drivers/cxl/security.c                  | 4 ++--
>  drivers/dax/cxl.c                       | 2 +-
>  drivers/perf/cxl_pmu.c                  | 4 ++--
>  {drivers/cxl => include/linux}/cxl.h    | 0
>  {drivers/cxl => include/linux}/cxlmem.h | 2 +-
>  {drivers/cxl => include/linux}/cxlpci.h | 0

So I don't like this approach, there are details that are private to the
CXL generic memory-expander use case, and some that are suitable for
CXL.mem and CXL.cache capabilities in other drivers. That distinct
subset should move to include/linux/. I.e. I want to see the incremental
conversion of what 3rd party drivers need compared to the generic
expander case, and consider when and where new shared infrastructure
needs to be refactored.

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Christoph Hellwig]

On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
> alucerop@ wrote:
> > From: Alejandro Lucero <alucerop@amd.com>
> > 
> > CXL Type 2 devices imply specific vendor drivers binding to those
> > devices instead of generic ones offered by CXL core like the PCI driver.

No, it absolutelt does not.  There is no such thing as a vendor driver
in Linux.

> So I don't like this approach, there are details that are private to the
> CXL generic memory-expander use case, and some that are suitable for
> CXL.mem and CXL.cache capabilities in other drivers. That distinct
> subset should move to include/linux/. I.e. I want to see the incremental
> conversion of what 3rd party drivers need compared to the generic
> expander case, and consider when and where new shared infrastructure
> needs to be refactored.

And I'd much rather keep all these drivers in drivers/cxl/ anyway so
that we can keep a tight control over them.

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Dan Williams]

alucerop@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Differientiating Type3, aka memory expanders, from Type2, aka device

s/Differientiating/Differentiating/

...actually to make this imperative tense don't use gerund phrases, so:

s/Differentiating/Differentiate/

This "imperative tense" preference is borrowed from the x86 tip tree
patch recommendations [1], which reminds me that CXL should create a
document like that to make the grammar expectations known.

[1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

> accelerators, with a new function for initializing cxl_dev_state.
> 
> Adding a type2 driver for a CXL emulated device inside CXL kernel

s/Adding/Add/

I will also note that ChatGPT does a decent job at converting patch
changelogs to imperative tense.

> testing infrastructure as a client for the functionality added.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

It would really help me out if the changelog mentions what you adopted
and what you modified. With a Link: to the patch where the code
originated.

I will still review the parts I wrote previously to see if I still agree
with them, but its taxing to come back to this patch cold and think "did
I write this routine, or is this new?". Can you repost with the
changelog commentary fixed up to reflect that?

> ---
>  drivers/cxl/core/memdev.c           | 15 ++++++
>  include/linux/cxlmem.h              |  2 +
>  tools/testing/cxl/Kbuild            |  1 +
>  tools/testing/cxl/type2/Kbuild      |  7 +++
>  tools/testing/cxl/type2/pci_type2.c | 80 +++++++++++++++++++++++++++++
>  5 files changed, 105 insertions(+)
>  create mode 100644 tools/testing/cxl/type2/Kbuild
>  create mode 100644 tools/testing/cxl/type2/pci_type2.c
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 07cd0b8b026f..0336b3f14f4a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work)
>  
>  static struct lock_class_key cxl_memdev_key;
>  
> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
> +{
> +	struct cxl_dev_state *cxlds;
> +
> +	cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> +	if (!cxlds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cxlds->dev = dev;
> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
> +
> +	return cxlds;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
> +
>  static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  					   const struct file_operations *fops)
>  {
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index 0d26a45a4af2..e8d12b543db1 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -859,4 +859,6 @@ struct cxl_hdm {
>  struct seq_file;
>  struct dentry *cxl_debugfs_create_dir(const char *dir);
>  void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
> +
> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>  #endif /* __CXL_MEM_H__ */
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 030b388800f0..a285719c4db6 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
>  KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>  
>  obj-m += test/
> +obj-m += type2/
> diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild
> new file mode 100644
> index 000000000000..a96ad4d64924
> --- /dev/null
> +++ b/tools/testing/cxl/type2/Kbuild
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-m += pci_type2.o
> +
> +cxl_pci_type2-y := cxl_pci_type2.o
> +
> +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> new file mode 100644
> index 000000000000..863ce7dc28ef
> --- /dev/null
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -0,0 +1,80 @@
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/cxl.h>
> +#include <linux/cxlpci.h>
> +#include <linux/cxlmem.h>
> +
> +struct cxl_dev_state *cxlds;
> +
> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
> +
> +static int type2_pci_probe(struct pci_dev *pci_dev,
> +			   const struct pci_device_id *entry)

So to date, tools/testing/cxl/ has been for cxl_test which skips all the
PCI register emulation and just runs based on mocking core-kernel and
core-cxl interfaces. I would like to explore how far the cxl_test
approach can go and leave the PCI integration to when a driver can
reference real PCI ids.

Otherwise, I feel like too much development effort can be diverted to
this "proxy" and increase the timeline to seeing the real thing.

> +
> +{
> +	u16 dvsec;
> +
> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> +
> +	if (!dvsec) {
> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
> +		return 0;
> +	} else {
> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
> +	}
> +
> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
> +	if (IS_ERR(cxlds))
> +		return PTR_ERR(cxlds);
> +
> +	pci_info(pci_dev, "Initializing cxlds...");
> +	cxlds->cxl_dvsec = dvsec;
> +	cxlds->serial = pci_dev->dev.id;
> +
> +	/* Should not this be based on DVSEC range size registers */
> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");

Especially at this stage of the driver there is nothing that require
QEMU emulation versus cxl_test mocking.

> +
> +	return 0;
> +}
> +
> +static void type2_pci_remove(struct pci_dev *pci_dev)
> +{
> +
> +}
> +
> +/* PCI device ID table */
> +static const struct pci_device_id type2_pci_table[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},

Real vendor-ids should have real device-ids, also that particular
device-id choice is not appropriate.

Re: [RFC PATCH 03/13] cxl: export core function for type2 devices

[Dan Williams]

alucerop@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> CXL initialization by type2 devices requires to use current CXL kernel
> infrastructure only available to such core code. Type2 devices are by
> definition owned by specific vendor drivers which need to use part of
> that infrastructure for initialization.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/pci.c                   |  3 ++-
>  include/linux/cxlpci.h              |  2 ++
>  tools/testing/cxl/type2/pci_type2.c | 31 +++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index ccde33ac9c1c..497276302017 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -500,7 +500,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> -static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  			      struct cxl_register_map *map)
>  {
>  	int rc;
> @@ -520,6 +520,7 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  
>  	return cxl_setup_regs(map);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL);

Any functionality in cxl_pci that you want to export to a 3rd party CXL
driver needs to move to drivers/cxl/core/pci.c

>  
>  static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>  {
> diff --git a/include/linux/cxlpci.h b/include/linux/cxlpci.h
> index 93992a1c8eec..28fa4861a4f9 100644
> --- a/include/linux/cxlpci.h
> +++ b/include/linux/cxlpci.h
> @@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
>  void cxl_cor_error_detected(struct pci_dev *pdev);
>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  				    pci_channel_state_t state);
> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> +		       struct cxl_register_map *map);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> index 863ce7dc28ef..b12f13e676fb 100644
> --- a/tools/testing/cxl/type2/pci_type2.c
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -12,7 +12,9 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>  			   const struct pci_device_id *entry)
>  
>  {
> +	struct cxl_register_map map;
>  	u16 dvsec;
> +	int rc;
>  
>  	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
>  
> @@ -35,6 +37,35 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>  	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>  	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
>  
> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxlds->reg_map);
> +	if (rc)
> +		dev_warn(&pci_dev->dev, "No component registers (%d)\n", rc);
> +
> +	rc = cxl_map_component_regs(&cxlds->reg_map, &cxlds->regs.component,
> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
> +	if (rc)
> +		dev_dbg(&pci_dev->dev, "Failed to map RAS capability.\n");
> +
> +	pci_info(pci_dev, "requesting resource...");

Setting aside whether this driver moves forward vs a cxl_test mock, if
you want the driver to be chatty use pci_dbg() or dev_dbg() not pci_info().

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Alejandro Lucero Palau]

On 6/12/24 05:27, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> CXL Type 2 devices imply specific vendor drivers binding to those
>> devices instead of generic ones offered by CXL core like the PCI driver.
>> Those drivers need to use CXL core functions and structs for
>> initialization, create memdevs and create CXL regions.
>>
>> This patch avoids referencing those files based on relative paths from
>> inside the kernel sources tree.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/acpi.c                      | 4 ++--
>>   drivers/cxl/core/cdat.c                 | 6 +++---
>>   drivers/cxl/core/hdm.c                  | 2 +-
>>   drivers/cxl/core/mbox.c                 | 6 +++---
>>   drivers/cxl/core/memdev.c               | 2 +-
>>   drivers/cxl/core/pci.c                  | 6 +++---
>>   drivers/cxl/core/pmem.c                 | 4 ++--
>>   drivers/cxl/core/pmu.c                  | 4 ++--
>>   drivers/cxl/core/port.c                 | 6 +++---
>>   drivers/cxl/core/region.c               | 4 ++--
>>   drivers/cxl/core/regs.c                 | 4 ++--
>>   drivers/cxl/core/suspend.c              | 2 +-
>>   drivers/cxl/core/trace.c                | 2 +-
>>   drivers/cxl/core/trace.h                | 4 ++--
>>   drivers/cxl/mem.c                       | 4 ++--
>>   drivers/cxl/pci.c                       | 6 +++---
>>   drivers/cxl/pmem.c                      | 4 ++--
>>   drivers/cxl/port.c                      | 4 ++--
>>   drivers/cxl/security.c                  | 4 ++--
>>   drivers/dax/cxl.c                       | 2 +-
>>   drivers/perf/cxl_pmu.c                  | 4 ++--
>>   {drivers/cxl => include/linux}/cxl.h    | 0
>>   {drivers/cxl => include/linux}/cxlmem.h | 2 +-
>>   {drivers/cxl => include/linux}/cxlpci.h | 0
> So I don't like this approach, there are details that are private to the
> CXL generic memory-expander use case, and some that are suitable for
> CXL.mem and CXL.cache capabilities in other drivers. That distinct
> subset should move to include/linux/. I.e. I want to see the incremental
> conversion of what 3rd party drivers need compared to the generic
> expander case, and consider when and where new shared infrastructure
> needs to be refactored.
>

OK. It makes sense.

I'll work on that if there is a v2.

Thanks

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Alejandro Lucero Palau]

On 6/12/24 05:30, Christoph Hellwig wrote:
> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
>> alucerop@ wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> CXL Type 2 devices imply specific vendor drivers binding to those
>>> devices instead of generic ones offered by CXL core like the PCI driver.
> No, it absolutelt does not.  There is no such thing as a vendor driver
> in Linux.


Well, yes, I see your point, and it is absolutely correct.

It was a bad way of explaining the need of a driver supporting a 
specific hardware.


>> So I don't like this approach, there are details that are private to the
>> CXL generic memory-expander use case, and some that are suitable for
>> CXL.mem and CXL.cache capabilities in other drivers. That distinct
>> subset should move to include/linux/. I.e. I want to see the incremental
>> conversion of what 3rd party drivers need compared to the generic
>> expander case, and consider when and where new shared infrastructure
>> needs to be refactored.
> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
> that we can keep a tight control over them.
>

My initial thought was drivers for ethernet, gpus, or any other kind of 
standard device binding to the CXL/PCIe.io and doing the CXL 
initialization. Your comment seems to suggest other approach, which I 
can only relate to using auxbus for the device CXL.mem and CXL.cache, 
and  then being handled by a generic driver inside the CXL core. Is this 
what you are suggesting?

If so, I'm not against it, although it would require to study the 
implications.

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Alejandro Lucero Palau]

On 6/12/24 05:43, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Differientiating Type3, aka memory expanders, from Type2, aka device
> s/Differientiating/Differentiating/
>
> ...actually to make this imperative tense don't use gerund phrases, so:
>
> s/Differentiating/Differentiate/
>
> This "imperative tense" preference is borrowed from the x86 tip tree
> patch recommendations [1], which reminds me that CXL should create a
> document like that to make the grammar expectations known.


OK.


> [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>
>> accelerators, with a new function for initializing cxl_dev_state.
>>
>> Adding a type2 driver for a CXL emulated device inside CXL kernel
> s/Adding/Add/
>
> I will also note that ChatGPT does a decent job at converting patch
> changelogs to imperative tense.


OK.


>> testing infrastructure as a client for the functionality added.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> It would really help me out if the changelog mentions what you adopted
> and what you modified. With a Link: to the patch where the code
> originated.


The patchset is mentioned/referenced in the cover letter.

I will add individual references as well for any patch needing it.


> I will still review the parts I wrote previously to see if I still agree
> with them, but its taxing to come back to this patch cold and think "did
> I write this routine, or is this new?". Can you repost with the
> changelog commentary fixed up to reflect that?


I'll do.


>> ---
>>   drivers/cxl/core/memdev.c           | 15 ++++++
>>   include/linux/cxlmem.h              |  2 +
>>   tools/testing/cxl/Kbuild            |  1 +
>>   tools/testing/cxl/type2/Kbuild      |  7 +++
>>   tools/testing/cxl/type2/pci_type2.c | 80 +++++++++++++++++++++++++++++
>>   5 files changed, 105 insertions(+)
>>   create mode 100644 tools/testing/cxl/type2/Kbuild
>>   create mode 100644 tools/testing/cxl/type2/pci_type2.c
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 07cd0b8b026f..0336b3f14f4a 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
>> +{
>> +	struct cxl_dev_state *cxlds;
>> +
>> +	cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
>> +	if (!cxlds)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	cxlds->dev = dev;
>> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
>> +
>> +	return cxlds;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
>> +
>>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   					   const struct file_operations *fops)
>>   {
>> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
>> index 0d26a45a4af2..e8d12b543db1 100644
>> --- a/include/linux/cxlmem.h
>> +++ b/include/linux/cxlmem.h
>> @@ -859,4 +859,6 @@ struct cxl_hdm {
>>   struct seq_file;
>>   struct dentry *cxl_debugfs_create_dir(const char *dir);
>>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
>> +
>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>>   #endif /* __CXL_MEM_H__ */
>> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
>> index 030b388800f0..a285719c4db6 100644
>> --- a/tools/testing/cxl/Kbuild
>> +++ b/tools/testing/cxl/Kbuild
>> @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
>>   KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>>   
>>   obj-m += test/
>> +obj-m += type2/
>> diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild
>> new file mode 100644
>> index 000000000000..a96ad4d64924
>> --- /dev/null
>> +++ b/tools/testing/cxl/type2/Kbuild
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-m += pci_type2.o
>> +
>> +cxl_pci_type2-y := cxl_pci_type2.o
>> +
>> +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
>> new file mode 100644
>> index 000000000000..863ce7dc28ef
>> --- /dev/null
>> +++ b/tools/testing/cxl/type2/pci_type2.c
>> @@ -0,0 +1,80 @@
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/cxl.h>
>> +#include <linux/cxlpci.h>
>> +#include <linux/cxlmem.h>
>> +
>> +struct cxl_dev_state *cxlds;
>> +
>> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
>> +
>> +static int type2_pci_probe(struct pci_dev *pci_dev,
>> +			   const struct pci_device_id *entry)
> So to date, tools/testing/cxl/ has been for cxl_test which skips all the
> PCI register emulation and just runs based on mocking core-kernel and
> core-cxl interfaces. I would like to explore how far the cxl_test
> approach can go and leave the PCI integration to when a driver can
> reference real PCI ids.
>
> Otherwise, I feel like too much development effort can be diverted to
> this "proxy" and increase the timeline to seeing the real thing.


Fair enough.

I think I could add the real driver in a new patchset version or maybe a 
completely new one.

 From my previous comment about potentially using something like 
auxbus,that would obviously mean a complete refactoring.


>> +
>> +{
>> +	u16 dvsec;
>> +
>> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
>> +
>> +	if (!dvsec) {
>> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
>> +		return 0;
>> +	} else {
>> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
>> +	}
>> +
>> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
>> +	if (IS_ERR(cxlds))
>> +		return PTR_ERR(cxlds);
>> +
>> +	pci_info(pci_dev, "Initializing cxlds...");
>> +	cxlds->cxl_dvsec = dvsec;
>> +	cxlds->serial = pci_dev->dev.id;
>> +
>> +	/* Should not this be based on DVSEC range size registers */
>> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
> Especially at this stage of the driver there is nothing that require
> QEMU emulation versus cxl_test mocking.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void type2_pci_remove(struct pci_dev *pci_dev)
>> +{
>> +
>> +}
>> +
>> +/* PCI device ID table */
>> +static const struct pci_device_id type2_pci_table[] = {
>> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
> Real vendor-ids should have real device-ids, also that particular
> device-id choice is not appropriate.

Re: [RFC PATCH 03/13] cxl: export core function for type2 devices

[Alejandro Lucero Palau]

On 6/12/24 05:50, Dan Williams wrote:
> alucerop@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> CXL initialization by type2 devices requires to use current CXL kernel
>> infrastructure only available to such core code. Type2 devices are by
>> definition owned by specific vendor drivers which need to use part of
>> that infrastructure for initialization.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/pci.c                   |  3 ++-
>>   include/linux/cxlpci.h              |  2 ++
>>   tools/testing/cxl/type2/pci_type2.c | 31 +++++++++++++++++++++++++++++
>>   3 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index ccde33ac9c1c..497276302017 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -500,7 +500,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>>   	return 0;
>>   }
>>   
>> -static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   			      struct cxl_register_map *map)
>>   {
>>   	int rc;
>> @@ -520,6 +520,7 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   
>>   	return cxl_setup_regs(map);
>>   }
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL);
> Any functionality in cxl_pci that you want to export to a 3rd party CXL
> driver needs to move to drivers/cxl/core/pci.c


OK.


>>   
>>   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   {
>> diff --git a/include/linux/cxlpci.h b/include/linux/cxlpci.h
>> index 93992a1c8eec..28fa4861a4f9 100644
>> --- a/include/linux/cxlpci.h
>> +++ b/include/linux/cxlpci.h
>> @@ -130,4 +130,6 @@ void read_cdat_data(struct cxl_port *port);
>>   void cxl_cor_error_detected(struct pci_dev *pdev);
>>   pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>>   				    pci_channel_state_t state);
>> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>> +		       struct cxl_register_map *map);
>>   #endif /* __CXL_PCI_H__ */
>> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
>> index 863ce7dc28ef..b12f13e676fb 100644
>> --- a/tools/testing/cxl/type2/pci_type2.c
>> +++ b/tools/testing/cxl/type2/pci_type2.c
>> @@ -12,7 +12,9 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>>   			   const struct pci_device_id *entry)
>>   
>>   {
>> +	struct cxl_register_map map;
>>   	u16 dvsec;
>> +	int rc;
>>   
>>   	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
>>   
>> @@ -35,6 +37,35 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>>   	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>>   	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
>>   
>> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_MEMDEV, &map);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map);
>> +	if (rc)
>> +		dev_warn(&pci_dev->dev, "No component registers (%d)\n", rc);
>> +
>> +	rc = cxl_map_component_regs(&cxlds->reg_map, &cxlds->regs.component,
>> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
>> +	if (rc)
>> +		dev_dbg(&pci_dev->dev, "Failed to map RAS capability.\n");
>> +
>> +	pci_info(pci_dev, "requesting resource...");
> Setting aside whether this driver moves forward vs a cxl_test mock, if
> you want the driver to be chatty use pci_dbg() or dev_dbg() not pci_info().
>

It seems this driver will be removed in future versions or probably in a 
new patchset.

Thanks

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Alejandro Lucero Palau]

Differentiate Type3, aka memory expanders, from Type2, aka device
accelerators, with a new function for initializing cxl_dev_state.

Add a type2 driver for a CXL emulated device inside CXL kernel
testing infrastructure as a client for the functionality added.

Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#mef63c11ff2ffc265b75785caeb2c2370953ccf44

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>

On 5/16/24 09:11, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Differientiating Type3, aka memory expanders, from Type2, aka device
> accelerators, with a new function for initializing cxl_dev_state.
>
> Adding a type2 driver for a CXL emulated device inside CXL kernel
> testing infrastructure as a client for the functionality added.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/cxl/core/memdev.c           | 15 ++++++
>   include/linux/cxlmem.h              |  2 +
>   tools/testing/cxl/Kbuild            |  1 +
>   tools/testing/cxl/type2/Kbuild      |  7 +++
>   tools/testing/cxl/type2/pci_type2.c | 80 +++++++++++++++++++++++++++++
>   5 files changed, 105 insertions(+)
>   create mode 100644 tools/testing/cxl/type2/Kbuild
>   create mode 100644 tools/testing/cxl/type2/pci_type2.c
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 07cd0b8b026f..0336b3f14f4a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work)
>   
>   static struct lock_class_key cxl_memdev_key;
>   
> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
> +{
> +	struct cxl_dev_state *cxlds;
> +
> +	cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> +	if (!cxlds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cxlds->dev = dev;
> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
> +
> +	return cxlds;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
> +
>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>   					   const struct file_operations *fops)
>   {
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index 0d26a45a4af2..e8d12b543db1 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -859,4 +859,6 @@ struct cxl_hdm {
>   struct seq_file;
>   struct dentry *cxl_debugfs_create_dir(const char *dir);
>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
> +
> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>   #endif /* __CXL_MEM_H__ */
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 030b388800f0..a285719c4db6 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
>   KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
>   
>   obj-m += test/
> +obj-m += type2/
> diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild
> new file mode 100644
> index 000000000000..a96ad4d64924
> --- /dev/null
> +++ b/tools/testing/cxl/type2/Kbuild
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-m += pci_type2.o
> +
> +cxl_pci_type2-y := cxl_pci_type2.o
> +
> +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> new file mode 100644
> index 000000000000..863ce7dc28ef
> --- /dev/null
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -0,0 +1,80 @@
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/cxl.h>
> +#include <linux/cxlpci.h>
> +#include <linux/cxlmem.h>
> +
> +struct cxl_dev_state *cxlds;
> +
> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
> +
> +static int type2_pci_probe(struct pci_dev *pci_dev,
> +			   const struct pci_device_id *entry)
> +
> +{
> +	u16 dvsec;
> +
> +	dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> +
> +	if (!dvsec) {
> +		pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor);
> +		return 0;
> +	} else {
> +		pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
> +	}
> +
> +	cxlds = cxl_accel_state_create(&pci_dev->dev);
> +	if (IS_ERR(cxlds))
> +		return PTR_ERR(cxlds);
> +
> +	pci_info(pci_dev, "Initializing cxlds...");
> +	cxlds->cxl_dvsec = dvsec;
> +	cxlds->serial = pci_dev->dev.id;
> +
> +	/* Should not this be based on DVSEC range size registers */
> +	cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram");
> +
> +	return 0;
> +}
> +
> +static void type2_pci_remove(struct pci_dev *pci_dev)
> +{
> +
> +}
> +
> +/* PCI device ID table */
> +static const struct pci_device_id type2_pci_table[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
> +	{0}                     /* end of list */
> +};
> +
> +static struct pci_driver type2_pci_driver = {
> +	.name           = KBUILD_MODNAME,
> +	.id_table       = type2_pci_table,
> +	.probe          = type2_pci_probe,
> +	.remove         = type2_pci_remove,
> +};
> +
> +static int __init type2_cxl_init(void)
> +{
> +	int rc;
> +
> +	rc = pci_register_driver(&type2_pci_driver);
> +
> +	return rc;
> +}
> +
> +static void __exit type2_cxl_exit(void)
> +{
> +	pci_unregister_driver(&type2_pci_driver);
> +}
> +
> +module_init(type2_cxl_init);
> +module_exit(type2_cxl_exit);
> +
> +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>");
> +MODULE_DESCRIPTION("CXL Type2 device support, driver test");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(CXL);
> +MODULE_DEVICE_TABLE(pci, type2_pci_table);

Re: [RFC PATCH 07/13] cxl: add functions for exclusive access to endpoint port topology

[Alejandro Lucero Palau]

From: Alejandro Lucero <alucerop@amd.com>

Prevent concurrent access to endpoint port topology.

Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m18497367d2ae38f88e94c06369eaa83fa23e92b2

Note: I realize original patch is explaining what the code does while my explanation is ... really poor. I will use original changelog in future versions.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>

On 5/16/24 09:11, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Prevent concurrent access to endpoint port topology.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/cxl/core/memdev.c           | 41 +++++++++++++++++++++++++++++
>   include/linux/cxlmem.h              |  4 +++
>   tools/testing/cxl/type2/pci_type2.c |  9 +++++++
>   3 files changed, 54 insertions(+)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 27063cd4ea73..16e356ef5b6d 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1124,6 +1124,47 @@ struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>   }
>   EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
>   
> +/*
> + * Try to get a locked reference on a memdev's CXL port topology
> + * connection. Be careful to observe when cxl_mem_probe() has deposited
> + * a probe deferral awaiting the arrival of the CXL root driver
> +*/
> +struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint;
> +	int rc = -ENXIO;
> +
> +	device_lock(&cxlmd->dev);
> +	endpoint = cxlmd->endpoint;
> +	if (!endpoint)
> +		goto err;
> +
> +	if (IS_ERR(endpoint)) {
> +		rc = PTR_ERR(endpoint);
> +		goto err;
> +	}
> +
> +	device_lock(&endpoint->dev);
> +	if (!endpoint->dev.driver)
> +		goto err_endpoint;
> +
> +	return endpoint;
> +
> +err_endpoint:
> +	device_unlock(&endpoint->dev);
> +err:
> +	device_unlock(&cxlmd->dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS(cxl_acquire_endpoint, CXL);
> +
> +void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
> +{
> +	device_unlock(&endpoint->dev);
> +	device_unlock(&cxlmd->dev);
> +}
> +EXPORT_SYMBOL_NS(cxl_release_endpoint, CXL);
> +
>   static void sanitize_teardown_notifier(void *data)
>   {
>   	struct cxl_memdev_state *mds = data;
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index e8d12b543db1..11fe8367b046 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -88,6 +88,10 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>   
>   struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>   				       struct cxl_dev_state *cxlds);
> +
> +struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd);
> +void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
> +
>   int devm_cxl_sanitize_setup_notifier(struct device *host,
>   				     struct cxl_memdev *cxlmd);
>   struct cxl_memdev_state;
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> index f157139b712f..948cc95c5780 100644
> --- a/tools/testing/cxl/type2/pci_type2.c
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -6,6 +6,7 @@
>   
>   struct cxl_dev_state *cxlds;
>   struct cxl_memdev *cxlmd;
> +struct cxl_port *endpoint;
>   
>   #define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
>   
> @@ -72,6 +73,14 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   	if (IS_ERR(cxlmd))
>   		return PTR_ERR(cxlmd);
>   
> +	endpoint = cxl_acquire_endpoint(cxlmd);
> +	if (IS_ERR(endpoint)) {
> +		dev_dbg(&pci_dev->dev, "cxl_acquire_endpoint failed\n");
> +		return PTR_ERR(endpoint);
> +	}
> +
> +	cxl_release_endpoint(cxlmd, endpoint);
> +
>   	return 0;
>   }
>   

Re: [RFC PATCH 08/13] cxl: add cxl_get_hpa_freespace

[Alejandro Lucero Palau]

From: Alejandro Lucero <alucerop@amd.com>

Based on the requirements from the endpoint and the topology such an
endpoint is attached to, this function informs about maximum host
physical address space possible to request. This is not a reservation
but only information which could change at the point the request based
on this information is made.

Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m6fbe775541da3cd477d65fa95c8acdc347345b4f

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>

On 5/16/24 09:11, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Based on the requirements from the endpoint and the topology such an
> endpoint is attached to, this function informs about maximum host
> physical address space possible to request. This is not a reservation
> but only information which could change at the point the request based
> on this information is made.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/cxl/core/region.c           | 163 ++++++++++++++++++++++++++++
>   include/linux/cxl.h                 |   5 +
>   include/linux/cxlmem.h              |   5 +
>   tools/testing/cxl/type2/pci_type2.c |  23 +++-
>   4 files changed, 195 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 70e86a7c241d..2731fd4243a1 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -702,6 +702,169 @@ static int free_hpa(struct cxl_region *cxlr)
>   	return 0;
>   }
>   
> +
> +struct cxlrd_max_context {
> +	struct device * const *host_bridges;
> +	int interleave_ways;
> +	unsigned long flags;
> +	resource_size_t max_hpa;
> +	struct cxl_root_decoder *cxlrd;
> +};
> +
> +static int find_max_hpa(struct device *dev, void *data)
> +{
> +	struct cxlrd_max_context *ctx = data;
> +	struct cxl_switch_decoder *cxlsd;
> +	struct cxl_root_decoder *cxlrd;
> +	struct resource *res, *prev;
> +	struct cxl_decoder *cxld;
> +	resource_size_t max;
> +	int found;
> +
> +	if (!is_root_decoder(dev))
> +		return 0;
> +
> +	cxlrd = to_cxl_root_decoder(dev);
> +	cxld = &cxlrd->cxlsd.cxld;
> +	if ((cxld->flags & ctx->flags) != ctx->flags) {
> +		dev_dbg(dev, "find_max_hpa, flags not matching: %08lx vs %08lx\n",
> +			      cxld->flags, ctx->flags);
> +		return 0;
> +	}
> +
> +	/* A Host bridge could have more interleave ways than an
> +	 * endpoint, couldn´t it?
> +	 *
> +	 * What does interleave ways mean here in terms of the requestor?
> +	 * Why the FFMWS has 0 interleave ways but root port has 1?
> +	 */
> +	if (cxld->interleave_ways != ctx->interleave_ways) {
> +		dev_dbg(dev, "find_max_hpa, interleave_ways  not matching\n");
> +		return 0;
> +	}
> +
> +	cxlsd = &cxlrd->cxlsd;
> +
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	found = 0;
> +	for (int i = 0; i < ctx->interleave_ways; i++)
> +		for (int j = 0; j < ctx->interleave_ways; j++)
> +			if (ctx->host_bridges[i] ==
> +					cxlsd->target[j]->dport_dev) {
> +				found++;
> +				break;
> +			}
> +
> +	if (found != ctx->interleave_ways) {
> +		dev_dbg(dev, "find_max_hpa, no interleave_ways found\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * Walk the root decoder resource range relying on cxl_region_rwsem to
> +	 * preclude sibling arrival/departure and find the largest free space
> +	 * gap.
> +	 */
> +	lockdep_assert_held_read(&cxl_region_rwsem);
> +	max = 0;
> +	res = cxlrd->res->child;
> +	if (!res)
> +		max = resource_size(cxlrd->res);
> +	else
> +		max = 0;
> +
> +	for (prev = NULL; res; prev = res, res = res->sibling) {
> +		struct resource *next = res->sibling;
> +		resource_size_t free = 0;
> +
> +		if (!prev && res->start > cxlrd->res->start) {
> +			free = res->start - cxlrd->res->start;
> +			max = max(free, max);
> +		}
> +		if (prev && res->start > prev->end + 1) {
> +			free = res->start - prev->end + 1;
> +			max = max(free, max);
> +		}
> +		if (next && res->end + 1 < next->start) {
> +			free = next->start - res->end + 1;
> +			max = max(free, max);
> +		}
> +		if (!next && res->end + 1 < cxlrd->res->end + 1) {
> +			free = cxlrd->res->end + 1 - res->end + 1;
> +			max = max(free, max);
> +		}
> +	}
> +
> +	if (max > ctx->max_hpa) {
> +		if (ctx->cxlrd)
> +			put_device(CXLRD_DEV(ctx->cxlrd));
> +		get_device(CXLRD_DEV(cxlrd));
> +		ctx->cxlrd = cxlrd;
> +		ctx->max_hpa = max;
> +		dev_info(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", &max);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints
> + * @endpoint: an endpoint that is mapped by the returned decoder
> + * @host_bridges: array of host-bridges that the decoder must interleave
> + * @interleave_ways: number of entries in @host_bridges
> + * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B]
> + * @max: output parameter of bytes available in the returned decoder
> + *
> + * The return tuple of a 'struct cxl_root_decoder' and 'bytes available (@max)'
> + * is a point in time snapshot. If by the time the caller goes to use this root
> + * decoder's capacity the capacity is reduced then caller needs to loop and
> + * retry.
> + *
> + * The returned root decoder has an elevated reference count that needs to be
> + * put with put_device(cxlrd_dev(cxlrd)). Locking context is with
> + * cxl_{acquire,release}_endpoint(), that ensures removal of the root decoder
> + * does not race.
> + */
> +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
> +					       struct device *const *host_bridges,
> +					       int interleave_ways,
> +					       unsigned long flags,
> +					       resource_size_t *max)
> +{
> +
> +	struct cxlrd_max_context ctx = {
> +		.host_bridges = host_bridges,
> +		.interleave_ways = interleave_ways,
> +		.flags = flags,
> +	};
> +	struct cxl_port *root_port;
> +	struct cxl_root *root;
> +
> +	if (!is_cxl_endpoint(endpoint)) {
> +		dev_dbg(&endpoint->dev, "hpa requestor is not an endpointr\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	root = find_cxl_root(endpoint);
> +	if (!root) {
> +		dev_dbg(&endpoint->dev, "endpoint can not be related to a root port\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	root_port = &root->port;
> +	down_read(&cxl_region_rwsem);
> +	device_for_each_child(&root_port->dev, &ctx, find_max_hpa);
> +	up_read(&cxl_region_rwsem);
> +	put_device(&root_port->dev);
> +
> +	if (!ctx.cxlrd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*max = ctx.max_hpa;
> +	return ctx.cxlrd;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_hpa_freespace, CXL);
> +
> +
>   static ssize_t size_store(struct device *dev, struct device_attribute *attr,
>   			  const char *buf, size_t len)
>   {
> diff --git a/include/linux/cxl.h b/include/linux/cxl.h
> index 036d17db68e0..1b2377062693 100644
> --- a/include/linux/cxl.h
> +++ b/include/linux/cxl.h
> @@ -766,6 +766,11 @@ struct cxl_decoder *to_cxl_decoder(struct device *dev);
>   struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
>   struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev);
>   struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev);
> +
> +#define CXLED_DEV(cxled)  &cxled->cxld.dev
> +
> +#define CXLRD_DEV(cxlrd) &cxlrd->cxlsd.cxld.dev
> +
>   bool is_root_decoder(struct device *dev);
>   bool is_switch_decoder(struct device *dev);
>   bool is_endpoint_decoder(struct device *dev);
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index 11fe8367b046..342ccd5486d3 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -865,4 +865,9 @@ struct dentry *cxl_debugfs_create_dir(const char *dir);
>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
>   
>   struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
> +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
> +					   struct device *const *host_bridges,
> +					   int interleave_ways,
> +					   unsigned long flags,
> +					   resource_size_t *max);
>   #endif /* __CXL_MEM_H__ */
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> index 948cc95c5780..deb5eeae501b 100644
> --- a/tools/testing/cxl/type2/pci_type2.c
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -4,6 +4,7 @@
>   #include <linux/cxlpci.h>
>   #include <linux/cxlmem.h>
>   
> +struct cxl_root_decoder *cxlrd;
>   struct cxl_dev_state *cxlds;
>   struct cxl_memdev *cxlmd;
>   struct cxl_port *endpoint;
> @@ -15,6 +16,7 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   
>   {
>   	struct cxl_register_map map;
> +	resource_size_t max = 0;
>   	u16 dvsec;
>   	int rc;
>   
> @@ -79,9 +81,28 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   		return PTR_ERR(endpoint);
>   	}
>   
> +	pci_info(pci_dev, "cxl hpa_freespace...");
> +	cxlrd = cxl_get_hpa_freespace(endpoint, &endpoint->host_bridge, 1,
> +				      CXL_DECODER_F_RAM | CXL_DECODER_F_TYPE2,
> +				      &max);
> +
> +	if (IS_ERR(cxlrd)) {
> +		dev_dbg(&pci_dev->dev, "cxl_get_hpa_freespace failed\n");
> +		rc = PTR_ERR(cxlrd);
> +		goto out;
> +	}
> +
> +	if (max < CXL_TYPE2_MEM_SIZE) {
> +		dev_dbg(&pci_dev->dev, "%s: no enough free HPA space %llu < %u\n",
> +				       __func__, max, CXL_TYPE2_MEM_SIZE);
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +out:
>   	cxl_release_endpoint(cxlmd, endpoint);
>   
> -	return 0;
> +	return rc;
>   }
>   
>   static void type2_pci_remove(struct pci_dev *pci_dev)

Re: [RFC PATCH 09/13] cxl: add cxl_request_dpa

[Alejandro Lucero Palau]

From: Alejandro Lucero <alucerop@amd.com>

Search and reserve DPA given input constraints.


Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m4271ee49a91615c8af54e3ab20679f8be3099393

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>



On 5/16/24 09:11, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Search and reserve DPA given input constraints.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/cxl/core/core.h             |   1 -
>   drivers/cxl/core/hdm.c              | 153 +++++++++++++++++++++++-----
>   include/linux/cxlmem.h              |   5 +
>   tools/testing/cxl/type2/pci_type2.c |  12 ++-
>   4 files changed, 145 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index bc5a95665aa0..c0a2e2c1ccb3 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -61,7 +61,6 @@ struct dentry *cxl_debugfs_create_dir(const char *dir);
>   int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>   		     enum cxl_decoder_mode mode);
>   int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size);
> -int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
>   resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled);
>   resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled);
>   
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index c5f70741d70a..6459b6ecde88 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -404,6 +404,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
>   	up_write(&cxl_dpa_rwsem);
>   	return rc;
>   }
> +EXPORT_SYMBOL_NS_GPL(cxl_dpa_free, CXL);
>   
>   int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>   		     enum cxl_decoder_mode mode)
> @@ -451,30 +452,17 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>   	return rc;
>   }
>   
> -int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
> +static resource_size_t cxl_dpa_freespace(struct cxl_endpoint_decoder *cxled,
> +					 resource_size_t *start_out,
> +					 resource_size_t *skip_out)
>   {
>   	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>   	resource_size_t free_ram_start, free_pmem_start;
> -	struct cxl_port *port = cxled_to_port(cxled);
>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct device *dev = &cxled->cxld.dev;
>   	resource_size_t start, avail, skip;
>   	struct resource *p, *last;
> -	int rc;
> -
> -	down_write(&cxl_dpa_rwsem);
> -	if (cxled->cxld.region) {
> -		dev_dbg(dev, "decoder attached to %s\n",
> -			dev_name(&cxled->cxld.region->dev));
> -		rc = -EBUSY;
> -		goto out;
> -	}
>   
> -	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) {
> -		dev_dbg(dev, "decoder enabled\n");
> -		rc = -EBUSY;
> -		goto out;
> -	}
> +	lockdep_assert_held(&cxl_dpa_rwsem);
>   
>   	for (p = cxlds->ram_res.child, last = NULL; p; p = p->sibling)
>   		last = p;
> @@ -496,7 +484,6 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>   		skip = 0;
>   	} else if (cxled->mode == CXL_DECODER_PMEM) {
>   		resource_size_t skip_start, skip_end;
> -
>   		start = free_pmem_start;
>   		avail = cxlds->pmem_res.end - start + 1;
>   		skip_start = free_ram_start;
> @@ -506,21 +493,50 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>   		 * already handled the skip.
>   		 */
>   		if (cxlds->pmem_res.child &&
> -		    skip_start == cxlds->pmem_res.child->start)
> +				skip_start == cxlds->pmem_res.child->start)
>   			skip_end = skip_start - 1;
>   		else
>   			skip_end = start - 1;
>   		skip = skip_end - skip_start + 1;
>   	} else {
> -		dev_dbg(dev, "mode not set\n");
> -		rc = -EINVAL;
> +		avail = 0;
> +	}
> +
> +	if (!avail)
> +		return 0;
> +	if (start_out)
> +		*start_out = start;
> +	if (skip_out)
> +		*skip_out = skip;
> +	return avail;
> +}
> +
> +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
> +{
> +	struct cxl_port *port = cxled_to_port(cxled);
> +	struct device *dev = &cxled->cxld.dev;
> +	resource_size_t start, avail, skip;
> +	int rc;
> +
> +	down_write(&cxl_dpa_rwsem);
> +	if (cxled->cxld.region) {
> +		dev_dbg(dev, "EBUSY, decoder attached to %s\n",
> +			     dev_name(&cxled->cxld.region->dev));
> +		rc = -EBUSY;
>   		goto out;
>   	}
>   
> +	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) {
> +		dev_dbg(dev, "EBUSY, decoder enabled\n");
> +		rc = -EBUSY;
> +		goto out;
> +	}
> +
> +	avail = cxl_dpa_freespace(cxled, &start, &skip);
>   	if (size > avail) {
>   		dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size,
> -			cxled->mode == CXL_DECODER_RAM ? "ram" : "pmem",
> -			&avail);
> +			     cxled->mode == CXL_DECODER_RAM ? "ram" : "pmem",
> +			     &avail);
>   		rc = -ENOSPC;
>   		goto out;
>   	}
> @@ -532,9 +548,98 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>   	if (rc)
>   		return rc;
>   
> -	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> +        return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
>   }
>   
> +static int find_free_decoder(struct device *dev, void *data)
> +{
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_port *port;
> +
> +	if (!is_endpoint_decoder(dev))
> +		return 0;
> +
> +	cxled = to_cxl_endpoint_decoder(dev);
> +	port = cxled_to_port(cxled);
> +
> +	if (cxled->cxld.id != port->hdm_end + 1) {
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +/**
> + * cxl_request_dpa - search and reserve DPA given input constraints
> + * @endpoint: an endpoint port with available decoders
> + * @mode: DPA operation mode (ram vs pmem)
> + * @min: the minimum amount of capacity the call needs
> + * @max: extra capacity to allocate after min is satisfied
> + *
> + * Given that a region needs to allocate from limited HPA capacity it
> + * may be the case that a device has more mappable DPA capacity than
> + * available HPA. So, the expectation is that @min is a driver known
> + * value for how much capacity is needed, and @max is based the limit of
> + * how much HPA space is available for a new region.
> + *
> + * Returns a pinned cxl_decoder with at least @min bytes of capacity
> + * reserved, or an error pointer. The caller is also expected to own the
> + * lifetime of the memdev registration associated with the endpoint to
> + * pin the decoder registered as well.
> + */
> +struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
> +					     enum cxl_decoder_mode mode,
> +					     resource_size_t min,
> +					     resource_size_t max)
> +{
> +	struct cxl_endpoint_decoder *cxled;
> +	struct device *cxled_dev;
> +	resource_size_t alloc;
> +	int rc;
> +
> +	if (!IS_ALIGNED(min | max, SZ_256M))
> +		return ERR_PTR(-EINVAL);
> +
> +	down_read(&cxl_dpa_rwsem);
> +
> +	cxled_dev = device_find_child(&endpoint->dev, NULL, find_free_decoder);
> +	if (!cxled_dev)
> +		cxled = ERR_PTR(-ENXIO);
> +	else
> +		cxled = to_cxl_endpoint_decoder(cxled_dev);
> +
> +	up_read(&cxl_dpa_rwsem);
> +
> +	if (IS_ERR(cxled)) {
> +               return cxled;
> +	}
> +
> +	rc = cxl_dpa_set_mode(cxled, mode);
> +	if (rc)
> +		goto err;
> +
> +	down_read(&cxl_dpa_rwsem);
> +	alloc = cxl_dpa_freespace(cxled, NULL, NULL);
> +	up_read(&cxl_dpa_rwsem);
> +
> +	if (max)
> +		alloc = min(max, alloc);
> +	if (alloc < min) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	rc = cxl_dpa_alloc(cxled, alloc);
> +	if (rc)
> +		goto err;
> +
> +	return cxled;
> +err:
> +	put_device(cxled_dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, CXL);
> +
> +
>   static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
>   {
>   	u16 eig;
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index 342ccd5486d3..caf1cd86421c 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -870,4 +870,9 @@ struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint,
>   					   int interleave_ways,
>   					   unsigned long flags,
>   					   resource_size_t *max);
> +struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
> +					     enum cxl_decoder_mode mode,
> +					     resource_size_t min,
> +					     resource_size_t max);
> +int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
>   #endif /* __CXL_MEM_H__ */
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> index deb5eeae501b..6499d709f54d 100644
> --- a/tools/testing/cxl/type2/pci_type2.c
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -4,6 +4,7 @@
>   #include <linux/cxlpci.h>
>   #include <linux/cxlmem.h>
>   
> +struct cxl_endpoint_decoder *cxled;
>   struct cxl_root_decoder *cxlrd;
>   struct cxl_dev_state *cxlds;
>   struct cxl_memdev *cxlmd;
> @@ -99,6 +100,15 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   		goto out;
>   	}
>   
> +	pci_info(pci_dev, "cxl request_dpa...");
> +	cxled = cxl_request_dpa(endpoint, CXL_DECODER_RAM, CXL_TYPE2_MEM_SIZE,
> +				CXL_TYPE2_MEM_SIZE);
> +	if (IS_ERR(cxled)) {
> +		dev_dbg(&pci_dev->dev, "cxl_request_dpa error\n");
> +		rc = PTR_ERR(cxled);
> +		goto out;
> +	}
> +
>   out:
>   	cxl_release_endpoint(cxlmd, endpoint);
>   
> @@ -107,7 +117,7 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   
>   static void type2_pci_remove(struct pci_dev *pci_dev)
>   {
> -
> +	cxl_dpa_free(cxled);
>   }
>   
>   /* PCI device ID table */

Re: [RFC PATCH 11/13] cxl: allow automatic region creation by type2 drivers

[Alejandro Lucero Palau]

From: Alejandro Lucero <alucerop@amd.com>

Creating a CXL region requires userspace intervention through the cxl
sysfs files. Type2 support should allow accelerator drivers to create
such cxl region from kernel code.

Add that functionality and integrate it with current support for
memory expanders.

Based on: https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m84598b534cc5664f5bb31521ba6e41c7bc213758

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>



On 5/16/24 09:12, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Creating a CXL region requires userspace intervention through the cxl
> sysfs files. Type2 support should allow accelerator drivers to create
> such cxl region from kernel code.
>
> Adding that functionality and integrating it with current support for
> memory expanders.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/cxl/core/region.c           | 262 ++++++++++++++++++++++------
>   include/linux/cxlmem.h              |   4 +
>   tools/testing/cxl/type2/pci_type2.c |  18 ++
>   3 files changed, 228 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 8228b7e96d8d..014684ff4343 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -479,22 +479,14 @@ static ssize_t interleave_ways_show(struct device *dev,
>   
>   static const struct attribute_group *get_cxl_region_target_group(void);
>   
> -static ssize_t interleave_ways_store(struct device *dev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t len)
> +static int set_interleave_ways(struct cxl_region *cxlr, int val)
>   {
> -	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>   	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>   	struct cxl_region_params *p = &cxlr->params;
> -	unsigned int val, save;
> -	int rc;
> +	int save, rc;
>   	u8 iw;
>   
> -	rc = kstrtouint(buf, 0, &val);
> -	if (rc)
> -		return rc;
> -
>   	rc = ways_to_eiw(val, &iw);
>   	if (rc)
>   		return rc;
> @@ -509,25 +501,42 @@ static ssize_t interleave_ways_store(struct device *dev,
>   		return -EINVAL;
>   	}
>   
> -	rc = down_write_killable(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> -	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
> -		rc = -EBUSY;
> -		goto out;
> -	}
> +	lockdep_assert_held_write(&cxl_region_rwsem);
> +	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
> +		return -EBUSY;
>   
>   	save = p->interleave_ways;
>   	p->interleave_ways = val;
>   	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
>   	if (rc)
>   		p->interleave_ways = save;
> -out:
> +
> +	return rc;
> +}
> +
> +static ssize_t interleave_ways_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t len)
> +{
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	unsigned int val;
> +	int rc;
> +
> +	rc = kstrtouint(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	rc = down_write_killable(&cxl_region_rwsem);
> +	if (rc)
> +		return rc;
> +
> +	rc = set_interleave_ways(cxlr, val);
>   	up_write(&cxl_region_rwsem);
>   	if (rc)
>   		return rc;
>   	return len;
>   }
> +
>   static DEVICE_ATTR_RW(interleave_ways);
>   
>   static ssize_t interleave_granularity_show(struct device *dev,
> @@ -547,21 +556,14 @@ static ssize_t interleave_granularity_show(struct device *dev,
>   	return rc;
>   }
>   
> -static ssize_t interleave_granularity_store(struct device *dev,
> -					    struct device_attribute *attr,
> -					    const char *buf, size_t len)
> +static int set_interleave_granularity(struct cxl_region *cxlr, int val)
>   {
> -	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
>   	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>   	struct cxl_region_params *p = &cxlr->params;
> -	int rc, val;
> +	int rc;
>   	u16 ig;
>   
> -	rc = kstrtoint(buf, 0, &val);
> -	if (rc)
> -		return rc;
> -
>   	rc = granularity_to_eig(val, &ig);
>   	if (rc)
>   		return rc;
> @@ -577,21 +579,36 @@ static ssize_t interleave_granularity_store(struct device *dev,
>   	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
>   		return -EINVAL;
>   
> +	lockdep_assert_held_write(&cxl_region_rwsem);
> +	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
> +		return -EBUSY;
> +
> +	p->interleave_granularity = val;
> +	return 0;
> +}
> +
> +static ssize_t interleave_granularity_store(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	int rc, val;
> +
> +	rc = kstrtoint(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
>   	rc = down_write_killable(&cxl_region_rwsem);
>   	if (rc)
>   		return rc;
> -	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
> -		rc = -EBUSY;
> -		goto out;
> -	}
>   
> -	p->interleave_granularity = val;
> -out:
> +	rc = set_interleave_granularity(cxlr, val);
>   	up_write(&cxl_region_rwsem);
>   	if (rc)
>   		return rc;
>   	return len;
>   }
> +
>   static DEVICE_ATTR_RW(interleave_granularity);
>   
>   static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
> @@ -2666,6 +2683,14 @@ cxl_find_region_by_name(struct cxl_root_decoder *cxlrd, const char *name)
>   	return to_cxl_region(region_dev);
>   }
>   
> +static void drop_region(struct cxl_region *cxlr)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_port *port = cxlrd_to_port(cxlrd);
> +
> +	devm_release_action(port->uport_dev, unregister_region, cxlr);
> +}
> +
>   static ssize_t delete_region_store(struct device *dev,
>   				   struct device_attribute *attr,
>   				   const char *buf, size_t len)
> @@ -3135,17 +3160,18 @@ static int match_region_by_range(struct device *dev, void *data)
>   	return rc;
>   }
>   
> -/* Establish an empty region covering the given HPA range */
> -static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> -					   struct cxl_endpoint_decoder *cxled)
> +static void construct_region_end(void)
> +{
> +	up_write(&cxl_region_rwsem);
> +}
> +
> +static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd,
> +						 struct cxl_endpoint_decoder *cxled)
>   {
>   	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_port *port = cxlrd_to_port(cxlrd);
> -	struct range *hpa = &cxled->cxld.hpa_range;
>   	struct cxl_region_params *p;
>   	struct cxl_region *cxlr;
> -	struct resource *res;
> -	int rc;
> +	int err = 0;
>   
>   	do {
>   		cxlr = __create_region(cxlrd, cxled->mode,
> @@ -3154,8 +3180,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
>   
>   	if (IS_ERR(cxlr)) {
> -		dev_err(cxlmd->dev.parent,
> -			"%s:%s: %s failed assign region: %ld\n",
> +		dev_err(cxlmd->dev.parent,"%s:%s: %s failed assign region: %ld\n",
>   			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>   			__func__, PTR_ERR(cxlr));
>   		return cxlr;
> @@ -3165,23 +3190,47 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   	p = &cxlr->params;
>   	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
>   		dev_err(cxlmd->dev.parent,
> -			"%s:%s: %s autodiscovery interrupted\n",
> +			"%s:%s: %s region setup interrupted\n",
>   			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>   			__func__);
> -		rc = -EBUSY;
> -		goto err;
> +		err = -EBUSY;
> +	}
> +
> +	if (err) {
> +		construct_region_end();
> +		drop_region(cxlr);
> +		return ERR_PTR(err);
>   	}
> +	return cxlr;
> +}
> +
> +
> +/* Establish an empty region covering the given HPA range */
> +static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> +					   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct range *hpa = &cxled->cxld.hpa_range;
> +	struct cxl_region_params *p;
> +	struct cxl_region *cxlr;
> +	struct resource *res;
> +	int rc;
> +
> +	cxlr = construct_region_begin(cxlrd, cxled);
> +	if (IS_ERR(cxlr))
> +		return cxlr;
>   
>   	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
>   
>   	res = kmalloc(sizeof(*res), GFP_KERNEL);
>   	if (!res) {
>   		rc = -ENOMEM;
> -		goto err;
> +		goto out;
>   	}
>   
>   	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>   				    dev_name(&cxlr->dev));
> +
>   	rc = insert_resource(cxlrd->res, res);
>   	if (rc) {
>   		/*
> @@ -3194,6 +3243,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   			 __func__, dev_name(&cxlr->dev));
>   	}
>   
> +	p = &cxlr->params;
>   	p->res = res;
>   	p->interleave_ways = cxled->cxld.interleave_ways;
>   	p->interleave_granularity = cxled->cxld.interleave_granularity;
> @@ -3201,24 +3251,124 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   
>   	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
>   	if (rc)
> -		goto err;
> +		goto out;
>   
>   	dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
> -		dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
> -		dev_name(&cxlr->dev), p->res, p->interleave_ways,
> -		p->interleave_granularity);
> +				   dev_name(&cxlmd->dev),
> +				   dev_name(&cxled->cxld.dev), __func__,
> +				   dev_name(&cxlr->dev), p->res,
> +				   p->interleave_ways,
> +				   p->interleave_granularity);
>   
>   	/* ...to match put_device() in cxl_add_to_region() */
>   	get_device(&cxlr->dev);
>   	up_write(&cxl_region_rwsem);
> +out:
> +	construct_region_end();
> +	if (rc) {
> +		drop_region(cxlr);
> +		return ERR_PTR(rc);
> +	}
> +	return cxlr;
> +}
> +
> +static struct cxl_region *
> +__construct_new_region(struct cxl_root_decoder *cxlrd,
> +		       struct cxl_endpoint_decoder **cxled, int ways)
> +{
> +	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> +	struct cxl_region_params *p;
> +	resource_size_t size = 0;
> +	struct cxl_region *cxlr;
> +	int rc, i;
> +
> +	/* If interleaving is not supported, why does ways need to be at least 1? */
> +	if (ways < 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	cxlr = construct_region_begin(cxlrd, cxled[0]);
> +	if (IS_ERR(cxlr))
> +		return cxlr;
> +
> +	rc = set_interleave_ways(cxlr, ways);
> +	if (rc)
> +		goto out;
> +
> +	rc = set_interleave_granularity(cxlr, cxld->interleave_granularity);
> +	if (rc)
> +		goto out;
> +
> +	down_read(&cxl_dpa_rwsem);
> +	for (i = 0; i < ways; i++) {
> +		if (!cxled[i]->dpa_res)
> +			break;
> +		size += resource_size(cxled[i]->dpa_res);
> +	}
> +	up_read(&cxl_dpa_rwsem);
> +
> +	if (i < ways)
> +		goto out;
> +
> +	rc = alloc_hpa(cxlr, size);
> +	if (rc)
> +		goto out;
> +
> +	down_read(&cxl_dpa_rwsem);
> +	for (i = 0; i < ways; i++) {
> +		rc = cxl_region_attach(cxlr, cxled[i], i);
> +		if (rc)
> +			break;
> +	}
> +	up_read(&cxl_dpa_rwsem);
> +
> +	if (rc)
> +		goto out;
> +
> +	rc = cxl_region_decode_commit(cxlr);
> +	if (rc)
> +		goto out;
>   
> +	p = &cxlr->params;
> +	p->state = CXL_CONFIG_COMMIT;
> +out:
> +	construct_region_end();
> +	if (rc) {
> +		drop_region(cxlr);
> +		return ERR_PTR(rc);
> +	}
>   	return cxlr;
> +}
>   
> -err:
> -	up_write(&cxl_region_rwsem);
> -	devm_release_action(port->uport_dev, unregister_region, cxlr);
> -	return ERR_PTR(rc);
> +/**
> + * cxl_create_region - Establish a region given an array of endpoint decoders
> + * @cxlrd: root decoder to allocate HPA
> + * @cxled: array of endpoint decoders with reserved DPA capacity
> + * @ways: size of @cxled array
> + *
> + * Returns a fully formed region in the commit state and attached to the
> + * cxl_region driver.
> + */
> +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> +				     struct cxl_endpoint_decoder **cxled,
> +				     int ways)
> +{
> +	struct cxl_region *cxlr;
> +
> +	mutex_lock(&cxlrd->range_lock);
> +	cxlr = __construct_new_region(cxlrd, cxled, ways);
> +	mutex_unlock(&cxlrd->range_lock);
> +
> +	if (IS_ERR(cxlr))
> +		return cxlr;
> +
> +	if (device_attach(&cxlr->dev) <= 0) {
> +		dev_err(&cxlr->dev, "failed to create region\n");
> +		drop_region(cxlr);
> +		return ERR_PTR(-ENODEV);
> +	}
> +	return cxlr;
>   }
> +EXPORT_SYMBOL_NS_GPL(cxl_create_region, CXL);
>   
>   int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>   {
> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
> index caf1cd86421c..fc963c2c2dc4 100644
> --- a/include/linux/cxlmem.h
> +++ b/include/linux/cxlmem.h
> @@ -875,4 +875,8 @@ struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint,
>   					     resource_size_t min,
>   					     resource_size_t max);
>   int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
> +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> +				     struct cxl_endpoint_decoder **cxled,
> +				     int ways);
> +
>   #endif /* __CXL_MEM_H__ */
> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c
> index 6499d709f54d..0e7f17c0c920 100644
> --- a/tools/testing/cxl/type2/pci_type2.c
> +++ b/tools/testing/cxl/type2/pci_type2.c
> @@ -4,8 +4,10 @@
>   #include <linux/cxlpci.h>
>   #include <linux/cxlmem.h>
>   
> +struct cxl_region_params *region_params;
>   struct cxl_endpoint_decoder *cxled;
>   struct cxl_root_decoder *cxlrd;
> +struct cxl_region *efx_region;
>   struct cxl_dev_state *cxlds;
>   struct cxl_memdev *cxlmd;
>   struct cxl_port *endpoint;
> @@ -109,6 +111,22 @@ static int type2_pci_probe(struct pci_dev *pci_dev,
>   		goto out;
>   	}
>   
> +	pci_info(pci_dev, "cxl create_region...");
> +	efx_region = cxl_create_region(cxlrd, &cxled, 1);
> +	if (!efx_region) {
> +		rc = PTR_ERR(cxled);
> +		goto out_dpa;
> +	}
> +
> +	region_params = &efx_region->params;
> +	pci_info(pci_dev, "CXL region: start=%llx, end=%llx\n", region_params->res->start,
> +			  region_params->res->end);
> +
> +	cxl_release_endpoint(cxlmd, endpoint);
> +	return 0;
> +
> +out_dpa:
> +	cxl_dpa_free(cxled);
>   out:
>   	cxl_release_endpoint(cxlmd, endpoint);
>   

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Jonathan Cameron]

On Wed, 12 Jun 2024 06:54:13 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 6/12/24 05:30, Christoph Hellwig wrote:
> > On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:  
> >> alucerop@ wrote:  
> >>> From: Alejandro Lucero <alucerop@amd.com>
> >>>
> >>> CXL Type 2 devices imply specific vendor drivers binding to those
> >>> devices instead of generic ones offered by CXL core like the PCI driver.  
> > No, it absolutelt does not.  There is no such thing as a vendor driver
> > in Linux.  
> 
> 
> Well, yes, I see your point, and it is absolutely correct.
> 
> It was a bad way of explaining the need of a driver supporting a 
> specific hardware.
> 
> 
> >> So I don't like this approach, there are details that are private to the
> >> CXL generic memory-expander use case, and some that are suitable for
> >> CXL.mem and CXL.cache capabilities in other drivers. That distinct
> >> subset should move to include/linux/. I.e. I want to see the incremental
> >> conversion of what 3rd party drivers need compared to the generic
> >> expander case, and consider when and where new shared infrastructure
> >> needs to be refactored.  
> > And I'd much rather keep all these drivers in drivers/cxl/ anyway so
> > that we can keep a tight control over them.
> >  
> 
> My initial thought was drivers for ethernet, gpus, or any other kind of 
> standard device binding to the CXL/PCIe.io and doing the CXL 
> initialization. Your comment seems to suggest other approach, which I 
> can only relate to using auxbus for the device CXL.mem and CXL.cache, 
> and  then being handled by a generic driver inside the CXL core. Is this 
> what you are suggesting?
> 
> If so, I'm not against it, although it would require to study the 
> implications.

I'd be very careful with that level of complexity.
So far I'm not seeing it as useful for this case (but I could be wrong)

Maybe just a case of well defined library code with only
opaque state etc being exposed by the CXL driver framework.

The bulk of any type2 driver needs to be in the appropriate subsystem
not drivers/cxl/  The non CXL parts will be more important to keep
aligned with appropriate subsystem than the CXL part. 

Jonathan

> 
> 

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Alejandro Lucero Palau]

On 6/12/24 11:07, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 06:54:13 +0100
> Alejandro Lucero Palau <alucerop@amd.com> wrote:
>
>> On 6/12/24 05:30, Christoph Hellwig wrote:
>>> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
>>>> alucerop@ wrote:
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> CXL Type 2 devices imply specific vendor drivers binding to those
>>>>> devices instead of generic ones offered by CXL core like the PCI driver.
>>> No, it absolutelt does not.  There is no such thing as a vendor driver
>>> in Linux.
>>
>> Well, yes, I see your point, and it is absolutely correct.
>>
>> It was a bad way of explaining the need of a driver supporting a
>> specific hardware.
>>
>>
>>>> So I don't like this approach, there are details that are private to the
>>>> CXL generic memory-expander use case, and some that are suitable for
>>>> CXL.mem and CXL.cache capabilities in other drivers. That distinct
>>>> subset should move to include/linux/. I.e. I want to see the incremental
>>>> conversion of what 3rd party drivers need compared to the generic
>>>> expander case, and consider when and where new shared infrastructure
>>>> needs to be refactored.
>>> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
>>> that we can keep a tight control over them.
>>>   
>> My initial thought was drivers for ethernet, gpus, or any other kind of
>> standard device binding to the CXL/PCIe.io and doing the CXL
>> initialization. Your comment seems to suggest other approach, which I
>> can only relate to using auxbus for the device CXL.mem and CXL.cache,
>> and  then being handled by a generic driver inside the CXL core. Is this
>> what you are suggesting?
>>
>> If so, I'm not against it, although it would require to study the
>> implications.
> I'd be very careful with that level of complexity.
> So far I'm not seeing it as useful for this case (but I could be wrong)


I agree the complexity could not be justified.

Indeed I think the privacy of a CXL.mem, something I pointed out, will 
make things more complicated.


> Maybe just a case of well defined library code with only
> opaque state etc being exposed by the CXL driver framework.


+1


> The bulk of any type2 driver needs to be in the appropriate subsystem
> not drivers/cxl/  The non CXL parts will be more important to keep
> aligned with appropriate subsystem than the CXL part.


+1


> Jonathan
>
>>

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Alejandro Lucero Palau]

On 6/12/24 07:04, Alejandro Lucero Palau wrote:
>
> On 6/12/24 05:43, Dan Williams wrote:
>> alucerop@ wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Differientiating Type3, aka memory expanders, from Type2, aka device
>> s/Differientiating/Differentiating/
>>
>> ...actually to make this imperative tense don't use gerund phrases, so:
>>
>> s/Differentiating/Differentiate/
>>
>> This "imperative tense" preference is borrowed from the x86 tip tree
>> patch recommendations [1], which reminds me that CXL should create a
>> document like that to make the grammar expectations known.
>
>
> OK.
>
>
>> [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>>
>>> accelerators, with a new function for initializing cxl_dev_state.
>>>
>>> Adding a type2 driver for a CXL emulated device inside CXL kernel
>> s/Adding/Add/
>>
>> I will also note that ChatGPT does a decent job at converting patch
>> changelogs to imperative tense.
>
>
> OK.
>
>
>>> testing infrastructure as a client for the functionality added.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> It would really help me out if the changelog mentions what you adopted
>> and what you modified. With a Link: to the patch where the code
>> originated.
>
>
> The patchset is mentioned/referenced in the cover letter.
>
> I will add individual references as well for any patch needing it.
>
>
>> I will still review the parts I wrote previously to see if I still agree
>> with them, but its taxing to come back to this patch cold and think "did
>> I write this routine, or is this new?". Can you repost with the
>> changelog commentary fixed up to reflect that?
>
>
> I'll do.
>
>
>>> ---
>>>   drivers/cxl/core/memdev.c           | 15 ++++++
>>>   include/linux/cxlmem.h              |  2 +
>>>   tools/testing/cxl/Kbuild            |  1 +
>>>   tools/testing/cxl/type2/Kbuild      |  7 +++
>>>   tools/testing/cxl/type2/pci_type2.c | 80 
>>> +++++++++++++++++++++++++++++
>>>   5 files changed, 105 insertions(+)
>>>   create mode 100644 tools/testing/cxl/type2/Kbuild
>>>   create mode 100644 tools/testing/cxl/type2/pci_type2.c
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index 07cd0b8b026f..0336b3f14f4a 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct 
>>> *work)
>>>     static struct lock_class_key cxl_memdev_key;
>>>   +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
>>> +{
>>> +    struct cxl_dev_state *cxlds;
>>> +
>>> +    cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
>>> +    if (!cxlds)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    cxlds->dev = dev;
>>> +    cxlds->type = CXL_DEVTYPE_DEVMEM;
>>> +
>>> +    return cxlds;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL);
>>> +
>>>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state 
>>> *cxlds,
>>>                          const struct file_operations *fops)
>>>   {
>>> diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h
>>> index 0d26a45a4af2..e8d12b543db1 100644
>>> --- a/include/linux/cxlmem.h
>>> +++ b/include/linux/cxlmem.h
>>> @@ -859,4 +859,6 @@ struct cxl_hdm {
>>>   struct seq_file;
>>>   struct dentry *cxl_debugfs_create_dir(const char *dir);
>>>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state 
>>> *cxlds);
>>> +
>>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>>>   #endif /* __CXL_MEM_H__ */
>>> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
>>> index 030b388800f0..a285719c4db6 100644
>>> --- a/tools/testing/cxl/Kbuild
>>> +++ b/tools/testing/cxl/Kbuild
>>> @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o
>>>   KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes 
>>> -Wmissing-declarations, $(KBUILD_CFLAGS))
>>>     obj-m += test/
>>> +obj-m += type2/
>>> diff --git a/tools/testing/cxl/type2/Kbuild 
>>> b/tools/testing/cxl/type2/Kbuild
>>> new file mode 100644
>>> index 000000000000..a96ad4d64924
>>> --- /dev/null
>>> +++ b/tools/testing/cxl/type2/Kbuild
>>> @@ -0,0 +1,7 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +obj-m += pci_type2.o
>>> +
>>> +cxl_pci_type2-y := cxl_pci_type2.o
>>> +
>>> +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes 
>>> -Wmissing-declarations, $(KBUILD_CFLAGS))
>>> diff --git a/tools/testing/cxl/type2/pci_type2.c 
>>> b/tools/testing/cxl/type2/pci_type2.c
>>> new file mode 100644
>>> index 000000000000..863ce7dc28ef
>>> --- /dev/null
>>> +++ b/tools/testing/cxl/type2/pci_type2.c
>>> @@ -0,0 +1,80 @@
>>> +#include <linux/module.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/cxl.h>
>>> +#include <linux/cxlpci.h>
>>> +#include <linux/cxlmem.h>
>>> +
>>> +struct cxl_dev_state *cxlds;
>>> +
>>> +#define CXL_TYPE2_MEM_SIZE   (1024*1024*256)
>>> +
>>> +static int type2_pci_probe(struct pci_dev *pci_dev,
>>> +               const struct pci_device_id *entry)
>> So to date, tools/testing/cxl/ has been for cxl_test which skips all the
>> PCI register emulation and just runs based on mocking core-kernel and
>> core-cxl interfaces. I would like to explore how far the cxl_test
>> approach can go and leave the PCI integration to when a driver can
>> reference real PCI ids.
>>
>> Otherwise, I feel like too much development effort can be diverted to
>> this "proxy" and increase the timeline to seeing the real thing.
>
>
> Fair enough.
>
> I think I could add the real driver in a new patchset version or maybe 
> a completely new one.
>
> From my previous comment about potentially using something like 
> auxbus,that would obviously mean a complete refactoring.
>
>
>>> +
>>> +{
>>> +    u16 dvsec;
>>> +
>>> +    dvsec = pci_find_dvsec_capability(pci_dev, 
>>> PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
>>> +
>>> +    if (!dvsec) {
>>> +        pci_info(pci_dev, "No CXL capability (vendor: %x\n", 
>>> pci_dev->vendor);
>>> +        return 0;
>>> +    } else {
>>> +        pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability 
>>> found");
>>> +    }
>>> +
>>> +    cxlds = cxl_accel_state_create(&pci_dev->dev);
>>> +    if (IS_ERR(cxlds))
>>> +        return PTR_ERR(cxlds);
>>> +
>>> +    pci_info(pci_dev, "Initializing cxlds...");
>>> +    cxlds->cxl_dvsec = dvsec;
>>> +    cxlds->serial = pci_dev->dev.id;
>>> +
>>> +    /* Should not this be based on DVSEC range size registers */
>>> +    cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE);
>>> +    cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, 
>>> "ram");
>> Especially at this stage of the driver there is nothing that require
>> QEMU emulation versus cxl_test mocking.
>>

I did use a slightly modified qemu hw/mem/cxl_type2.c for adding 
CXL.cache bits, and planned to submit it for anyone interested in Type2 
development ...


>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void type2_pci_remove(struct pci_dev *pci_dev)
>>> +{
>>> +
>>> +}
>>> +
>>> +/* PCI device ID table */
>>> +static const struct pci_device_id type2_pci_table[] = {
>>> +    {PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)},
>> Real vendor-ids should have real device-ids, also that particular
>> device-id choice is not appropriate.
>
And this is the PCI IDs used by such emulation. It is emulating a 
non-existing CXL Type2 device what I guess is more problematic than 
emulating a memory expander.

My interested was to test configuration and not any kind of emulated 
acceleration related to CXL.mem writes or reads, what could be a project 
by itself but not sure if useful at all.

If this is valuable, I will submit it. I think extending qemu CXL 
support for helping development could be really useful for all those 
things hard to test like interleaving and sooner or later, complex 
fabrics with a good number of CXL switches, plus x-FAM support.

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Alison Schofield]

On Tue, Jun 11, 2024 at 09:43:57PM -0700, Dan Williams wrote:
> alucerop@ wrote:
> > From: Alejandro Lucero <alucerop@amd.com>
> > 
> > Differientiating Type3, aka memory expanders, from Type2, aka device
> 
> s/Differientiating/Differentiating/
> 
> ...actually to make this imperative tense don't use gerund phrases, so:
> 
> s/Differentiating/Differentiate/
> 
> This "imperative tense" preference is borrowed from the x86 tip tree
> patch recommendations [1], which reminds me that CXL should create a
> document like that to make the grammar expectations known.
> 
> [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 

Hi Dan,

Imperative tense is the default expectation thoughout the kernel, so
'we' in CXL don't need to create any special rules around that.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.

-- Alison
> 

Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Dan Williams]

Alison Schofield wrote:
> On Tue, Jun 11, 2024 at 09:43:57PM -0700, Dan Williams wrote:
> > alucerop@ wrote:
> > > From: Alejandro Lucero <alucerop@amd.com>
> > > 
> > > Differientiating Type3, aka memory expanders, from Type2, aka device
> > 
> > s/Differientiating/Differentiating/
> > 
> > ...actually to make this imperative tense don't use gerund phrases, so:
> > 
> > s/Differentiating/Differentiate/
> > 
> > This "imperative tense" preference is borrowed from the x86 tip tree
> > patch recommendations [1], which reminds me that CXL should create a
> > document like that to make the grammar expectations known.
> > 
> > [1]: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> > 
> 
> Hi Dan,
> 
> Imperative tense is the default expectation thoughout the kernel, so
> 'we' in CXL don't need to create any special rules around that.
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Oh, yeah, forgot about that, thanks!

However, the motivation for a CXL maintainer-handbook entry would also
be to convey subsystem-local details like how often to ping on a series,
when the merge window cutoff typically lands, what baseline to use for
new patch submissions, do submissions need to worry about
reverse-xmas-tree variable declartations, etc...

Sidenote on that topic of formatting, I think it would also be useful to
declare "clang-format is good enough", i.e. even if clang-format spits
out something that could be made to look prettier by hand, the
auto-formatted patch is good enough. If someone really cares they can
update the clang-format template.

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Dan Williams]

Christoph Hellwig wrote:
> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
> > alucerop@ wrote:
> > > From: Alejandro Lucero <alucerop@amd.com>
> > > 
> > > CXL Type 2 devices imply specific vendor drivers binding to those
> > > devices instead of generic ones offered by CXL core like the PCI driver.
> 
> No, it absolutelt does not.  There is no such thing as a vendor driver
> in Linux.
> 
> > So I don't like this approach, there are details that are private to the
> > CXL generic memory-expander use case, and some that are suitable for
> > CXL.mem and CXL.cache capabilities in other drivers. That distinct
> > subset should move to include/linux/. I.e. I want to see the incremental
> > conversion of what 3rd party drivers need compared to the generic
> > expander case, and consider when and where new shared infrastructure
> > needs to be refactored.
> 
> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
> that we can keep a tight control over them.

Yeah, especially for the common pieces like CXL memory where the
endpoint driver is responsible for registering a 'struct cxl_memdev' and
the existing 'cxl_mem' driver just grows to accommodate device-memory
alongside host-only memory expansion.

For CXL accelerator use case that's where more specifics are needed
because CXL.cache operation gets entangled with IOMMU / DMA mapping
policy for ATS.

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Alejandro Lucero Palau]

On 6/12/24 22:18, Dan Williams wrote:
> Christoph Hellwig wrote:
>> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
>>> alucerop@ wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> CXL Type 2 devices imply specific vendor drivers binding to those
>>>> devices instead of generic ones offered by CXL core like the PCI driver.
>> No, it absolutelt does not.  There is no such thing as a vendor driver
>> in Linux.
>>
>>> So I don't like this approach, there are details that are private to the
>>> CXL generic memory-expander use case, and some that are suitable for
>>> CXL.mem and CXL.cache capabilities in other drivers. That distinct
>>> subset should move to include/linux/. I.e. I want to see the incremental
>>> conversion of what 3rd party drivers need compared to the generic
>>> expander case, and consider when and where new shared infrastructure
>>> needs to be refactored.
>> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
>> that we can keep a tight control over them.
> Yeah, especially for the common pieces like CXL memory where the
> endpoint driver is responsible for registering a 'struct cxl_memdev' and
> the existing 'cxl_mem' driver just grows to accommodate device-memory
> alongside host-only memory expansion.
>
> For CXL accelerator use case that's where more specifics are needed
> because CXL.cache operation gets entangled with IOMMU / DMA mapping
> policy for ATS.

I think we all agree the central pieces should be in CXL core and other 
drivers requiring CXL setup going through such central management.

But this can be implemented as I thought and tried, and Jonathan has 
also suggested, that is an API with opaque structs, or through a more 
disjointed way from a client/driver perspective using an approach like 
auxbus. Maybe there is another approach for "keep all these drivers in 
drivers/cxl/ anyway so that we can keep a tight control over them" which 
needs to be more explicitly elaborated.

My plan is for a v2 removing the test/mock driver and adding the sfc 
driver as a client, but only if the original approach with the CXL core 
exporting functions is the approved one. If auxbus or other way is the 
way to go, it will be just a new patchset and requiring more time.

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Dan Williams]

Alejandro Lucero Palau wrote:
[..]
> On 6/12/24 22:18, Dan Williams wrote:
> > Christoph Hellwig wrote:
> >> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
> >>> alucerop@ wrote:
> >>>> From: Alejandro Lucero <alucerop@amd.com>
> >>>>
> >>>> CXL Type 2 devices imply specific vendor drivers binding to those
> >>>> devices instead of generic ones offered by CXL core like the PCI driver.
> >> No, it absolutelt does not.  There is no such thing as a vendor driver
> >> in Linux.
> >>
> >>> So I don't like this approach, there are details that are private to the
> >>> CXL generic memory-expander use case, and some that are suitable for
> >>> CXL.mem and CXL.cache capabilities in other drivers. That distinct
> >>> subset should move to include/linux/. I.e. I want to see the incremental
> >>> conversion of what 3rd party drivers need compared to the generic
> >>> expander case, and consider when and where new shared infrastructure
> >>> needs to be refactored.
> >> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
> >> that we can keep a tight control over them.
> > Yeah, especially for the common pieces like CXL memory where the
> > endpoint driver is responsible for registering a 'struct cxl_memdev' and
> > the existing 'cxl_mem' driver just grows to accommodate device-memory
> > alongside host-only memory expansion.
> >
> > For CXL accelerator use case that's where more specifics are needed
> > because CXL.cache operation gets entangled with IOMMU / DMA mapping
> > policy for ATS.
> 
> I think we all agree the central pieces should be in CXL core and other 
> drivers requiring CXL setup going through such central management.
> 
> But this can be implemented as I thought and tried

Like I said earlier, start by minimizing the definitions to just what is
needed as that may trip over things that need to be refactored.

For example, we had a similar conversation about how to support the CXL
mailbox which appears to be showing up in more than just generic CXL
memory expanders. In that case the observation is that all the APIs that
currently assume a 'struct cxl_memdev_state *' probably need to be
refactored into something that takes a new 'struct cxl_mbox *'.

> and Jonathan has also suggested, that is an API with opaque structs,

Opaque structs is even nicer, but first step is "don't make the entirety
of the current definitions public" because that violates 'least
privilege' for many of these structures. 

> or through a more disjointed way from a client/driver perspective
> using an approach like auxbus.

What? No, I see no need for auxbus. "cxl" is already a bus.

> Maybe there is another approach for "keep all these drivers in
> drivers/cxl/ anyway so that we can keep a tight control over them"
> which needs to be more explicitly elaborated.

For now lets interpret that to mean that any driver that wants to
enumerate or provision a topology of CXL HDM decoders should be looking
to register a 'struct cxl_memdev' object which is driven by the cxl_mem
driver in drivers/cxl/. The client driver that registers that need not be
located in drivers/cxl/.

> My plan is for a v2 removing the test/mock driver and adding the sfc 
> driver as a client,

Oh, as in CONFIG_SFC? Can you say anything about the use case for
CXL.cache and CXL.mem so that we make sure the code architecture lines
are being drawn at the right level?

> but only if the original approach with the CXL core 
> exporting functions is the approved one.

Not sure what you mean here? Exporting common CXL core functions is how
the cxl_mem driver itself gets enabled. So there is no other path
besides refactoring the core to be useful outside of the general memory
expansion use case. This is analogous to how the PCI core provides
infrastructure for PCI drivers.

> If auxbus or other way is the way to go, it will be just a new
> patchset and requiring more time.

Mainline has the time to get it right.

Re: [RFC PATCH 01/13] cxl: move header files for absolute references

[Alejandro Lucero Palau]

On 6/14/24 02:22, Dan Williams wrote:
> Alejandro Lucero Palau wrote:
> [..]
>> On 6/12/24 22:18, Dan Williams wrote:
>>> Christoph Hellwig wrote:
>>>> On Tue, Jun 11, 2024 at 09:27:38PM -0700, Dan Williams wrote:
>>>>> alucerop@ wrote:
>>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>>
>>>>>> CXL Type 2 devices imply specific vendor drivers binding to those
>>>>>> devices instead of generic ones offered by CXL core like the PCI driver.
>>>> No, it absolutelt does not.  There is no such thing as a vendor driver
>>>> in Linux.
>>>>
>>>>> So I don't like this approach, there are details that are private to the
>>>>> CXL generic memory-expander use case, and some that are suitable for
>>>>> CXL.mem and CXL.cache capabilities in other drivers. That distinct
>>>>> subset should move to include/linux/. I.e. I want to see the incremental
>>>>> conversion of what 3rd party drivers need compared to the generic
>>>>> expander case, and consider when and where new shared infrastructure
>>>>> needs to be refactored.
>>>> And I'd much rather keep all these drivers in drivers/cxl/ anyway so
>>>> that we can keep a tight control over them.
>>> Yeah, especially for the common pieces like CXL memory where the
>>> endpoint driver is responsible for registering a 'struct cxl_memdev' and
>>> the existing 'cxl_mem' driver just grows to accommodate device-memory
>>> alongside host-only memory expansion.
>>>
>>> For CXL accelerator use case that's where more specifics are needed
>>> because CXL.cache operation gets entangled with IOMMU / DMA mapping
>>> policy for ATS.
>> I think we all agree the central pieces should be in CXL core and other
>> drivers requiring CXL setup going through such central management.
>>
>> But this can be implemented as I thought and tried
> Like I said earlier, start by minimizing the definitions to just what is
> needed as that may trip over things that need to be refactored.


Yes, I agree the headers treatment in the patch was too much coarse 
work. I'll work on that for v2.


> For example, we had a similar conversation about how to support the CXL
> mailbox which appears to be showing up in more than just generic CXL
> memory expanders. In that case the observation is that all the APIs that
> currently assume a 'struct cxl_memdev_state *' probably need to be
> refactored into something that takes a new 'struct cxl_mbox *'.
>
>> and Jonathan has also suggested, that is an API with opaque structs,
> Opaque structs is even nicer, but first step is "don't make the entirety
> of the current definitions public" because that violates 'least
> privilege' for many of these structures.
>
>> or through a more disjointed way from a client/driver perspective
>> using an approach like auxbus.
> What? No, I see no need for auxbus. "cxl" is already a bus.
>> Maybe there is another approach for "keep all these drivers in
>> drivers/cxl/ anyway so that we can keep a tight control over them"
>> which needs to be more explicitly elaborated.
> For now lets interpret that to mean that any driver that wants to
> enumerate or provision a topology of CXL HDM decoders should be looking
> to register a 'struct cxl_memdev' object which is driven by the cxl_mem
> driver in drivers/cxl/. The client driver that registers that need not be
> located in drivers/cxl/.


Great. This needed clarification after your previous email.


>> My plan is for a v2 removing the test/mock driver and adding the sfc
>> driver as a client,
> Oh, as in CONFIG_SFC? Can you say anything about the use case for
> CXL.cache and CXL.mem so that we make sure the code architecture lines
> are being drawn at the right level?


I did briefly comment on that in the cover-letter discussion exchange.

We got some hardware buffers now mapped from BAR regions which are 
written by the driver for low latency sending versus descriptor + packet 
through DMA.

The idea is those buffers being CXL.mem mappings now.

CXL.cache will be for receiving. It will not be part of the first 
products though.


>> but only if the original approach with the CXL core
>> exporting functions is the approved one.
> Not sure what you mean here? Exporting common CXL core functions is how
> the cxl_mem driver itself gets enabled. So there is no other path
> besides refactoring the core to be useful outside of the general memory
> expansion use case. This is analogous to how the PCI core provides
> infrastructure for PCI drivers.


OK. This is all related to that comment/suggestion for keeping all CXL 
capable drivers in the CXL core directory.

The only way I could envision such an approach was through a driver 
creating auxiliary devices for CXL.mem and CXL.cache which could bind to 
generic CXL drivers (with the appropriate support in current CXL core). 
Doable but probably too much complicated.


I think it is all clear now.

Thanks


>> If auxbus or other way is the way to go, it will be just a new
>> patchset and requiring more time.
> Mainline has the time to get it right.