Message-ID: <6786f7bff1389_20f329429@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 14 Jan 2025 15:48:16 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Dan Williams
	<dan.j.williams@...el.com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<martin.habets@...inx.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>
Subject: Re: [PATCH v8 01/27] cxl: add type2 device basic support

Alejandro Lucero Palau wrote:
> 
> On 1/7/25 23:42, Dan Williams wrote:
> > alejandro.lucero-palau@ wrote:
> >> From: Alejandro Lucero <alucerop@....com>
> >>
> >> 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/
> >> Signed-off-by: Alejandro Lucero <alucerop@....com>
> >> Co-developed-by: Dan Williams <dan.j.williams@...el.com>
> >> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> >> Reviewed-by: Fan Ni <fan.ni@...sung.com>
> > This patch causes
> >> ---
> >>   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.
> 
> 
> >> +{
> >> +	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...
> >
> >> +
> >>   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.
> 
> 
> >> +
> >> +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.

> >> +		     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.

Powered by blists - more mailing lists