Message-ID: <6786eab3a124c_20f3294f4@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 14 Jan 2025 14:52:35 -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/8/25 14:32, Alejandro Lucero Palau wrote:
> >
> > On 1/8/25 01:33, Dan Williams wrote:
> >> 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
> >> Whoops, forgot to complete this thought. Someting in this series causes:
> >>
> >> depmod: ERROR: Cycle detected: ecdh_generic
> >> depmod: ERROR: Cycle detected: tpm
> >> depmod: ERROR: Cycle detected: cxl_mock -> cxl_core -> cxl_mock
> >> depmod: ERROR: Cycle detected: encrypted_keys
> >> depmod: ERROR: Found 2 modules in dependency cycles!
> >>
> >> I think the non CXL ones are false likely triggered by the CXL causing
> >> depmod to exit early.
> >>
> >> Given cxl-test is unfamiliar territory to many submitters I always offer
> >> to fix up the breakage. I came up with the below incremental patch to
> >> fold in that also addresses my other feedback.
> >>
> >> Now the depmod error is something Alison saw too, and while I can also
> >> see it on patch1 if I do:
> >>
> >> - apply whole series
> >> - build => see the error
> >> - rollback patch1
> >> - build => see the error
> >>
> >> ...a subsequent build the error goes away, so I think that transient
> >> behavior is a quirk of how cxl-test is built, but some later patch in
> >> that series makes the failure permanent.
> >>
> >> In any event I figured that out after creating the below fixup and
> >> realizing that it does not fix the cxl-test build issue:
> >
> >
> > Ok. but it is a good way of showing what you had in your mind about 
> > the suggested changes.
> >
> > I'll use it for v10.
> >
> > Thanks
> >
> 
> Hi Dan,
> 
> 
> There's a problem with this approach and it is the need of the driver 
> having access to internal cxl structs like cxl_dev_state.

Apologies for stepping away for a few days, there was a chance to get
the long pending DAX page reference count series into v6.14 that I
needed to devote some review cycles.

> Your patch does not cover it but for an accel driver that struct needs 
> to be allocated before using the new cxl_dev_state_init.

Why does the cxl core need to wrap malloc on behalf of the driver?

> I think we reached an agreement in initial discussions about avoiding 
> this need through an API for accel drivers indirectly doing whatever is 
> needed regarding internal CXL structs. Initially it was stated this 
> being necessary for avoiding drivers doing wrong things but Jonathan 
> pointed out the main concern being changing those internal structs in 
> the future could benefit from this approach. Whatever the reason, that 
> was the assumption.

I think there is a benefit from a driver being able to do someting like:

struct my_cxl_accelerator_context {
    ...
    struct cxl_dev_state cxlds;
    ...
};

Even if the rule is that direct consumption of 'struct cxl_dev_state'
outside of the cxl core is unwanted.

C does not make this easy, so it is either make the definition of
'struct cxl_dev_state' public to CXL accelerator drivers so that they
know the size, or add an allocation API that takes in the extra size
that accelerator needs to allocate the core CXL context.

Unless and until we run into a real life problem of accelerator drivers
misusing 'struct cxl_dev_state' I think I prefer the explicit approach
of make the data structure embeddable and only require the core to do
the initialization.

> I could add a function for accel drivers doing the allocation as with 
> current v9 code, and then using your changes for having common code.

Let me go look at what you have there, but the design principle of the
CXL core is a library and enabling (but not requiring) users to have a container_of()
relationship between the core context and their local context feels the
most maintainable at this point.

> Also, I completely agree with merging the serial and dvsec 
> initializations through arguments to cxl_dev_state_init, but we need the 
> cxl_set_resource function for accel drivers. The current code for adding 
> resources with memdev is relying on mbox commands, and although we could 
> change that code for supporting accel drivers without an mbox, I would 
> say the function/code added is simple enough for not requiring that 
> effort. Note my goal is for an accel device without an mbox, but we will 
> see devices with one in the future, so I bet for leaving any change 
> there to that moment.

...but the way it was adding those resources was just wrong. This also
collides with some of the changes proposed for DCD partition management.
I needs a rethink in terms of a data structure and API to describe the
DPA address map of a CXL.mem capable device to the core and the core
should not be hard-coding a memory-expander-class device definition to
that layout.

I am imagining something similar to the way that resource range
resources are transmitted to platform device registration.

Powered by blists - more mailing lists