Message-ID: <6788a451b241a_20fa294f9@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 15 Jan 2025 22:16:49 -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:
[..]
> >> 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 think you say is wrong because you did not look at patch 8 where the 
> resources are requested based on the parent resource (dpa).

Patch 8 does not make things better. It leaves in place the confusion of
cxl_set_resource() which blindly overrides, without locks, the values of
the DPA resource tree. It further perpetuates this "enum
cxl_resource_type" proposal which I think is an API mistake given we
already have "enum cxl_decoder_mode" complication relative to new
partition types being added by DCD. The usage of release_resource() is
problematic because that deletes child resources, and static DPA
resources do not need to be released when ->dpa_res is also being
destroyed.

Much of this stems from the fact that cxl_pci and the cxl_core have been
too cozy to date with assumptions like "cxl_mem_create_range_info() can
be lockless because it is easy to see that the only user does the right
thing".

In fact part of the motivation for the 'EXPORT_SYMBOL_NS_GPL(..., "CXL")'
symbol namespace was to flag potential consumers outside
of cxl_pci to consider that they do not know the cxl_core internal
rules.

For accelerators, which may be spread across many drivers in the kernel,
clearer semantics and API safety are needed. I.e. with accelerator
drivers are entering an era where the cxl_core exported APIs can no
longer assume a known quantity cxl_pci consumer. It needs to be a
responsible library that limits API misuse.

> After seeing Ira's DCD patchset regarding the resources allocation 
> changes, and your comment there, I think I know that you have in mind. 
> But for Type2 the way resources information is obtained changes, at 
> least for the case of one without mailbox. In our case we are hardcoding 
> the resource definitions, although in the future (and likely other 
> drivers) we will use an internal/firmware path for obtaining the 
> information. So we have two cases:
> 
> 
> 1) accel driver with mailbox: an additional API function should allow 
> such accel driver to obtain the info or trigger the resource allocation 
> based on that command.
> 
> 2) accel driver without mailbox: a function for allocating the resources 
> based on hardcoded or driver-dependent dynamically-obtained data.

Lets just make those cases and the memory expander cases all the same.

The DPA resource map is always constructed and passed in explicitly, not
a side effect of other operations. Whether it came from a mailbox, or
was statically known by the driver ahead of time, the cxl_core should
not care.

> The current patchset is supporting the second one, and with the linked 
> use case, the first one should be delayed until some accel driver 
> requires it.
> 
> 
> I can adapt the current API for using the resource array exposed by 
> DCD's patches, and use  add_dpa_res function instead of current patchset 
> code.

The DCD code is also suffering from the explicit ->ram_res ->pmem_res
attributes of cxl_dev_state. That needs to cleaned up for both this
type-2 series and the DCD series to build on top.

> > I am imagining something similar to the way that resource range
> > resources are transmitted to platform device registration.
> 
> 
> I guess you mean to have  static/dynamic resource array with __init flag 
> for freeing the data after initialization, a CXL core function for 
> processing the array and calling add_dpa_res and aware of DCD patch 
> needs, and the resources linked to the device and released automatically 
> when unloading the driver.

The DPA address map is not dynamic. Once it is initialized there is no
need to release them. The DPA child reservations need to be released, but the
initial map is released just by freeing the cxl_dev_state. Notice how
there is no DPA resource release in cxl_pci for the top-level resources.

I am taking a shot at putting code where my mouth is to unblock the
type-2 and DCD series. I.e. just converting ram and pmem to an array,
and splitting cxl_mem_create_range_info() into cxl_mem_dpa_fetch() and
cxl_dpa_setup() where both the accelerator case and expander case will
share cxl_dpa_setup().

Going through tests now...

Powered by blists - more mailing lists