Re: [PATCH 3/4] cxl: Introduce 'struct cxl_dpa_partition' and 'struct cxl_range_info'

[Alejandro Lucero Palau <alucerop@amd.com>]

On 1/17/25 06:10, Dan Williams wrote:
> The pending efforts to add CXL Accelerator (type-2) device [1], and
> Dynamic Capacity (DCD) support [2], tripped on the
> no-longer-fit-for-purpose design in the CXL subsystem for tracking
> device-physical-address (DPA) metadata. Trip hazards include:
>
> - CXL Memory Devices need to consider a PMEM partition, but Accelerator
>    devices with CXL.mem likely do not in the common case.
>
> - CXL Memory Devices enumerate DPA through Memory Device mailbox
>    commands like Partition Info, Accelerators devices do not.
>
> - CXL Memory Devices that support DCD support more than 2 partitions.
>    Some of the driver algorithms are awkward to expand to > 2 partition
>    cases.
>
> - DPA performance data is a general capability that can be shared with
>    accelerators, so tracking it in 'struct cxl_memdev_state' is no longer
>    suitable.
>
> - 'enum cxl_decoder_mode' is sometimes a partition id and sometimes a
>    memory property, it should be phased in favor of a partition id and
>    the memory property comes from the partition info.
>
> Towards cleaning up those issues and allowing a smoother landing for the
> aforementioned pending efforts, introduce a 'struct cxl_dpa_partition'
> array to 'struct cxl_dev_state', and 'struct cxl_range_info' as a shared
> way for Memory Devices and Accelerators to initialize the DPA information
> in 'struct cxl_dev_state'.
>
> For now, split a new cxl_dpa_setup() from cxl_mem_create_range_info() to
> get the new data structure initialized, and cleanup some qos_class init.
> Follow on patches will go further to use the new data structure to
> cleanup algorithms that are better suited to loop over all possible
> partitions.
>
> cxl_dpa_setup() follows the locking expectations of mutating the device
> DPA map, and is suitable for Accelerator drivers to use. Accelerators
> likely only have one hardcoded 'ram' partition to convey to the
> cxl_core.


<snip>

> +/* if this fails the caller must destroy @cxlds, there is no recovery */
> +int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info)
> +{
> +	struct device *dev = cxlds->dev;
> +
> +	guard(rwsem_write)(&cxl_dpa_rwsem);
> +


This explains to me what you meant about locking when setting the 
resources for Type2.


However, I think this is no necessary because there is no user space, or 
that is my idea, involved when creating CXL regions for a Type2. It is 
all up to the accel driver to do so, therefore no locking needed because 
none is going to traverse the child resource list while 
initialising/updating it.

It does not harm to have it for current Type2 case, and always a good 
idea to have it for potential future cases.