netdev - Re: [PATCH v19 18/22] cxl: Allow region creation by type2 drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f09f8d3-5bc1-40a1-a9fa-1ffe14bf2eaa@amd.com>
Date: Mon, 10 Nov 2025 13:47:40 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v19 18/22] cxl: Allow region creation by type2 drivers


On 10/7/25 15:11, Jonathan Cameron wrote:
> On Mon, 6 Oct 2025 11:01:26 +0100
> <alejandro.lucero-palau@....com> wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Creating a CXL region requires userspace intervention through the cxl
>> sysfs files. Type2 support should allow accelerator drivers to create
>> such cxl region from kernel code.
>>
>> Adding that functionality and integrating it with current support for
>> memory expanders.
>>
>> Support an action by the type2 driver to be linked to the created region
>> for unwinding the resources allocated properly.
>>
>> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> One new question below (and a trivial thing beyond that).
>
> If you adopt one of the suggested ways of tidying that up, then keep the RB
> if not I'll want to take another look so drop it.
>

I will drop your RB since it is not clear to me if your suggestion below 
makes sense.


>>   #ifdef CONFIG_CXL_REGION
>>   extern struct device_attribute dev_attr_create_pmem_region;
>>   extern struct device_attribute dev_attr_create_ram_region;
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 26dfc15e57cd..e3b6d85cd43e 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> +
>> +/* Establish an empty region covering the given HPA range */
>> +static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>> +					   struct cxl_endpoint_decoder *cxled)
>> +{
>> +	struct cxl_port *port = cxlrd_to_port(cxlrd);
>> +	struct cxl_region *cxlr;
>> +	int rc;
>> +
>> +	cxlr = construct_region_begin(cxlrd, cxled);
>>   
>>   	rc = __construct_region(cxlr, cxlrd, cxled);
>>   	if (rc) {
>> @@ -3621,6 +3639,106 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>>   	return cxlr;
>>   }
>>   
>> +DEFINE_FREE(cxl_region_drop, struct cxl_region *, if (_T) drop_region(_T))
>> +
>> +static struct cxl_region *
>> +__construct_new_region(struct cxl_root_decoder *cxlrd,
>> +		       struct cxl_endpoint_decoder **cxled, int ways)
> Why pass in an array of struct cxl_endpoint_decoder * if this is only
> ever going to use the first element?
>
> I think we need to indicate that somehow.  Could just pass in the
> relevant decoder (I assume there is only one?)  Or pass the array
> and an index (here 0).


Just the first one is use for creating the region, what means the 
struct/object which will be initialised with the attaching phase later on.

The region will be created with the target type and mode of the first 
decoder used, but the attaching implies to check the other decoders 
align with this. And all the decoders are used for that and for 
calculating the hpa size to request.

So I do not think there is a problem here, at least regarding your concern.


>
>> +{
>> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled[0]);
>> +	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>> +	struct cxl_region_params *p;
>> +	resource_size_t size = 0;
>> +	int rc, i;
>> +
>> +	struct cxl_region *cxlr __free(cxl_region_drop) =
>> +		construct_region_begin(cxlrd, cxled[0]);
>> +	if (IS_ERR(cxlr))
>> +		return cxlr;
>> +
>> +	guard(rwsem_write)(&cxl_rwsem.region);
>> +
>> +	/*
>> +	 * Sanity check. This should not happen with an accel driver handling
>> +	 * the region creation.
>> +	 */
>> +	p = &cxlr->params;
>> +	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
>> +		dev_err(cxlmd->dev.parent,
>> +			"%s:%s: %s  unexpected region state\n",
>> +			dev_name(&cxlmd->dev), dev_name(&cxled[0]->cxld.dev),
>> +			__func__);
>> +		return ERR_PTR(-EBUSY);
>> +	}
>> +
>> +	rc = set_interleave_ways(cxlr, ways);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	rc = set_interleave_granularity(cxlr, cxld->interleave_granularity);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	scoped_guard(rwsem_read, &cxl_rwsem.dpa) {
>> +		for (i = 0; i < ways; i++) {
>> +			if (!cxled[i]->dpa_res)
>> +				break;
>> +			size += resource_size(cxled[i]->dpa_res);
>> +		}
>> +		if (i < ways)
>> +			return ERR_PTR(-EINVAL);
>> +
>> +		rc = alloc_hpa(cxlr, size);
>> +		if (rc)
>> +			return ERR_PTR(rc);
>> +
>> +		for (i = 0; i < ways; i++) {
>> +			rc = cxl_region_attach(cxlr, cxled[i], 0);
>> +			if (rc)
>> +				return ERR_PTR(rc);
>> +		}
>> +	}
>> +
>> +	rc = cxl_region_decode_commit(cxlr);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	p->state = CXL_CONFIG_COMMIT;
>> +
>> +	return no_free_ptr(cxlr);
> return_ptr()
>> +}