Re: [PATCH v21 08/23] cxl/sfc: Map cxl component regs

[PJ Waskiewicz <ppwaskie@kernel.org>]

On Fri, 2025-11-21 at 11:01 +0000, Alejandro Lucero Palau wrote:
> 
> On 11/21/25 06:54, PJ Waskiewicz wrote:
> > On Wed, 2025-11-19 at 19:22 +0000, alejandro.lucero-palau@amd.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.

That would be a good addition.  Maybe something to indicate "hey, go
check if someone else already claimed ownership of this memory region"
instead of using a divining rod to find this in /proc/iomem on a hunch.
:)

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

If you could capture this in either a comment or just the patch
description, I feel like there's enough paper-trail for people doing
this sort of design work should be informed.

I'd just hate to see all this work you're doing to make it in, and a
hardware designer somewhere not knowing the restrictions, and getting
irritated when their shiny new chip doesn't work with your code.  We
can at least help them with documentation.

Cheers,
-PJ