Message-ID: <69e5c565-3a19-4290-b0b5-9a0a749b5045@amd.com>
Date: Fri, 21 Nov 2025 11:01:37 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: PJ Waskiewicz <ppwaskie@...nel.org>, alejandro.lucero-palau@....com,
 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
Cc: Edward Cree <ecree.xilinx@...il.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Ben Cheatham <benjamin.cheatham@....com>
Subject: Re: [PATCH v21 08/23] cxl/sfc: Map cxl component regs


On 11/21/25 06:54, PJ Waskiewicz wrote:
> On Wed, 2025-11-19 at 19:22 +0000, alejandro.lucero-palau@....com
> wrote:
>
> Hi Alejandro,


Hi PJ,


<snip>


>> +	}
>> +
>> +	rc = cxl_map_component_regs(&cxl->cxlds.reg_map,
>> +				    &cxl->cxlds.regs.component,
>> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
> I'm going to reiterate a previous concern here with this.  When all of
> this was in the CXL core, the CXL core owned whatever BAR these
> registers were in in its entirety.  Now with a Type2 device, splitting
> this out has implications.


I have not forgotten your concern and as I said then, I will work on a 
follow-up for this once basic Type2 support patchset goes through.

The client linked to this patchset is the sfc driver and we do not have 
this problem for the card supporting CXL. But I fully understand this is 
a problem for other more than potential Type2 API clients.


> The cxl_map_component_regs() is going to try and map the register map
> you request as a reserved resource, which will fail if this Type2
> driver has the BAR mapped (which basically all of these drivers do).
>
> I think it's worth either a big comment or something explicit in the
> patch description that calls this limitation or restriction out.
> Hardware designers will be caught off-guard if they design their
> hardware where the CXL component regs are in a BAR shared by other
> register maps in their devices.  If they land the CXL regs in the
> middle of that BAR, they will have to do some serious gymnastics in the
> drivers to map pieces of their BAR to allow the kernel to map the
> component regs.  OR...they can have some breadcrumbs to try and design
> the HW where the CXL component regs are at the very beginning or very
> end of their BAR.  That way drivers have an easier way to reserve a
> subset of a contiguous BAR, and allow the kernel to grab the remainder
> for CXL access and management.


I have thought about the proper solution for this and IMO implies to add 
a new argument where the client can specify the already mapped memory 
for getting the CXL regs available to the CXL core. It should not be too 
much complicated, but I prefer to leave it for a follow up. Not sure if 
you want something more complicated where the code can solve this 
without the driver's write awareness, but the call failing could be more 
chatty about this possibility so the user can know.


But I agree the current patchset should at least specifically comment on 
this in the code. I will do so in v22, but if there exists generic 
concern about this case not being supported in the current work, I'll be 
addressing this for such a next patchset version.


Thank you!



>
> I think this is a pretty serious implication that I don't see a way
> around.  But letting a HW designer fall into this hole and realize they
> can only fix it with a horrible set of driver hacks, or a silicon
> respin, really sucks.
>
> Cheers,
> -PJ
>
>> +	if (rc) {
>> +		pci_err(pci_dev, "Failed to map RAS capability.\n");
>> +		return rc;
>> +	}
>> +
>> +	/*
>> +	 * Set media ready explicitly as there are neither mailbox
>> for checking
>> +	 * this state nor the CXL register involved, both not
>> mandatory for
>> +	 * type2.
>> +	 */
>> +	cxl->cxlds.media_ready = true;
>> +
>>   	probe_data->cxl = cxl;
>>   
>>   	return 0;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 13d448686189..7f2e23bce1f7 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -70,6 +70,10 @@ struct cxl_regs {
>>   	);
>>   };
>>   
>> +#define   CXL_CM_CAP_CAP_ID_RAS 0x2
>> +#define   CXL_CM_CAP_CAP_ID_HDM 0x5
>> +#define   CXL_CM_CAP_CAP_HDM_VERSION 1
>> +
>>   struct cxl_reg_map {
>>   	bool valid;
>>   	int id;
>> @@ -223,4 +227,19 @@ struct cxl_dev_state
>> *_devm_cxl_dev_state_create(struct device *dev,
>>   		(drv_struct *)_devm_cxl_dev_state_create(parent,
>> type, serial, dvsec,	\
>>   						
>> sizeof(drv_struct), mbox);	\
>>   	})
>> +
>> +/**
>> + * cxl_map_component_regs - map cxl component registers
>> + *
>> + * @map: cxl register map to update with the mappings
>> + * @regs: cxl component registers to work with
>> + * @map_mask: cxl component regs to map
>> + *
>> + * Returns integer: success (0) or error (-ENOMEM)
>> + *
>> + * Made public for Type2 driver support.
>> + */
>> +int cxl_map_component_regs(const struct cxl_register_map *map,
>> +			   struct cxl_component_regs *regs,
>> +			   unsigned long map_mask);
>>   #endif /* __CXL_CXL_H__ */
>> diff --git a/include/cxl/pci.h b/include/cxl/pci.h
>> new file mode 100644
>> index 000000000000..a172439f08c6
>> --- /dev/null
>> +++ b/include/cxl/pci.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>> +
>> +#ifndef __CXL_CXL_PCI_H__
>> +#define __CXL_CXL_PCI_H__
>> +
>> +/* Register Block Identifier (RBI) */
>> +enum cxl_regloc_type {
>> +	CXL_REGLOC_RBI_EMPTY = 0,
>> +	CXL_REGLOC_RBI_COMPONENT,
>> +	CXL_REGLOC_RBI_VIRT,
>> +	CXL_REGLOC_RBI_MEMDEV,
>> +	CXL_REGLOC_RBI_PMU,
>> +	CXL_REGLOC_RBI_TYPES
>> +};
>> +
>> +struct cxl_register_map;
>> +
>> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type
>> type,
>> +		       struct cxl_register_map *map);
>> +#endif

Powered by blists - more mailing lists