Alejandro Lucero Palau wrote:

On 1/7/25 23:42, Dan Williams wrote:

quoted

alejandro.lucero-palau@ wrote:

quoted

From: Alejandro Lucero <redacted>

Differentiate CXL memory expanders (type 3) from CXL device accelerators
(type 2) with a new function for initializing cxl_dev_state.

Create accessors to cxl_dev_state to be used by accel drivers.

Based on previous work by Dan Williams [1]

Link: [1] https://lore.kernel.org/linux-cxl/168592160379.1948938.12863272903570476312.stgit@dwillia2-xfh.jf.intel.com/ (local)
Signed-off-by: Alejandro Lucero <redacted>
Co-developed-by: Dan Williams <redacted>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Fan Ni <redacted>

This patch causes

quoted

---
  drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
  drivers/cxl/core/pci.c    |  1 +
  drivers/cxl/cxlpci.h      | 16 ------------
  drivers/cxl/pci.c         | 13 +++++++---
  include/cxl/cxl.h         | 21 ++++++++++++++++
  include/cxl/pci.h         | 23 ++++++++++++++++++
  6 files changed, 105 insertions(+), 20 deletions(-)
  create mode 100644 include/cxl/cxl.h
  create mode 100644 include/cxl/pci.h

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index ae3dfcbe8938..99f533caae1e 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c

@@ -7,6 +7,7 @@
  #include <linux/slab.h>
  #include <linux/idr.h>
  #include <linux/pci.h>
+#include <cxl/cxl.h>
  #include <cxlmem.h>
  #include "trace.h"
  #include "core.h"

@@ -616,6 +617,25 @@ 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)

Lets just call this cxl_dev_state_create and have cxl_memdev_state use
it internally for the truly common init functionality.

Move the cxlds->type setting to a passed in parameter as that appears to
be the only common init piece that needs to change to make this usable
by cxl_memdev_state_create().

That would also fix the missing initialization of these values the
cxl_memdev_state_create() currently handles:

         mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
         mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
         mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;

Ok. It makes sense.

quoted

quoted

+{
+	struct cxl_dev_state *cxlds;
+
+	cxlds = kzalloc(sizeof(*cxlds), GFP_KERNEL);
+	if (!cxlds)
+		return ERR_PTR(-ENOMEM);
+
+	cxlds->dev = dev;
+	cxlds->type = CXL_DEVTYPE_DEVMEM;
+
+	cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa");
+	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram");
+	cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem");
+
+	return cxlds;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, "CXL");

So, this is the only new function I would expect in this patch based on
the changelog...

quoted

+
  static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
  					   const struct file_operations *fops)
  {

@@ -693,6 +713,37 @@ static int cxl_memdev_open(struct inode *inode, struct file *file)
  	return 0;
  }
  
+void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
+{
+	cxlds->cxl_dvsec = dvsec;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_set_dvsec, "CXL");
+
+void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial)
+{
+	cxlds->serial = serial;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_set_serial, "CXL");

What are these doing in this patch? Why are new exports needed for such
trivial functions? If they are common values to move to init time I would
just make them common argument to cxl_dev_state_create().

I was told to merge those simple changes in this one instead of 
additional patches.

And I have no problem dropping them and use extra  args.

I'll do so it v10.

quoted

quoted

+
+int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,

Additionally, why does this take a 'struct resource' rather than a 'struct resource *'?

The driver does not need the resource but for this initialization, so it 
is a locally allocated resource which will not exist later on.

It is a small struct so I guess your concern is not with the stack, 
maybe about security. If it is due to some rule to avoid it which I'm 
not familiar with, it has gone undetected through a lot of eyes ...

It's not about security its about the semantic of how does an
accelerator initialize the DPA address space of a device, and can it do
it in a generic way that can be shared across acclerators,
pre-HDM-decoder expander devices, post HDM-decoder expanders, and
new-fangled DCD capable expanders.

quoted

quoted

+		     enum cxl_resource type)
+{
+	switch (type) {
+	case CXL_RES_DPA:
+		cxlds->dpa_res = res;
+		return 0;
+	case CXL_RES_RAM:
+		cxlds->ram_res = res;
+		return 0;
+	case CXL_RES_PMEM:
+		cxlds->pmem_res = res;
+		return 0;

This appears to misunderstand the relationship between these resources.
dpa_res is the overall device internal DPA address space resource tree.
ram_res and pmem_res are shortcuts to get to the volatile and pmem
partitions of the dpa space. I can imagine it would ever be desirable to
trust the caller to fully initialize all the values of the resource,
especially 'parent', 'sibling', and 'child' which should only be touched
under the resource lock in the common case.

No, I'm aware of this, but also I think there is a need for setting them 
independently, and the reason behind this code.

Maybe you have in mind some complex devices requiring another approach 
for this set up.

DPA space layout initialization is a common operation so I am looking
for a way for expanders and accelerators to stay unified on shared
library code for the similar semantics.

Let me see if I can draft something that also considers what the DCD
code is trying to do.