Message-ID: <11dd7fca-0862-4a56-85e8-06580b041f3b@amd.com>
Date: Thu, 16 Jan 2025 10:02:29 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: 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


On 1/16/25 06:16, Dan Williams wrote:
> 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.


The confusion is mine now. Why do we need locking here? It is at cxl 
accel driver initialization time, no memdev has been created yet, no 
region either. We have just created the cxl_dev_state. What can make use 
of this requiring locking? What are we overriding here? In my view we 
are just initializing the cxl_dev_state struct.


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


A decoder is not mandatory so we need to make the distinction.


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


It does not delete anything, just releases a resource.

I agree it could not be needed if always relying on dpa_res destruction, 
what is the case at least in our case. Having the complementary of 
request_resource does not seem problematic to me, but I wonder if it 
could be really needed. Maybe there is the case for virtualization when 
assigning the device or parts of it to a VM, or other cases where a 
driver does other more complex things.


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


Again, I can not see the need for protection during cxl driver 
initialization. What could go wrong? Or where are the potential users 
colliding here?


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


Sure.


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


Right.


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


Perfect.


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


I meant the statically resource array created by the platform driver 
like in arch/arm/mach-omap1/dma.c with the res array defined in line 79.

It is used for giving the resources to the platform dev core then 
removed due to __initdate. In the SFC case, we can do a similar static 
definition, but I guess other drivers or SFC in the future requiring to 
create such array dynamically.


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


Yes, I noticed that, but not child reservation for ram or pmem released 
with the current code.


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


That's super. I hope it can be integrated soon.


Thank you

Powered by blists - more mailing lists