Message-ID: <9b62e7cc-0b58-4354-b4ed-3538a536bd42@intel.com>
Date: Wed, 15 Oct 2025 10:52:15 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>,
 "Cheatham, Benjamin" <benjamin.cheatham@....com>,
 alejandro.lucero-palau@....com
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.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
Subject: Re: [PATCH v19 11/22] cxl: Define a driver interface for HPA free
 space enumeration



On 10/10/25 4:16 AM, Alejandro Lucero Palau wrote:
> 
> On 10/9/25 21:55, Cheatham, Benjamin wrote:
>> On 10/6/2025 5:01 AM, alejandro.lucero-palau@....com wrote:
>>> From: Alejandro Lucero <alucerop@....com>
>>>
>>> CXL region creation involves allocating capacity from Device Physical Address
>>> (DPA) and assigning it to decode a given Host Physical Address (HPA). Before
>>> determining how much DPA to allocate the amount of available HPA must be
>>> determined. Also, not all HPA is created equal, some HPA targets RAM, some
>>> targets PMEM, some is prepared for device-memory flows like HDM-D and HDM-DB,
>>> and some is HDM-H (host-only).
>>>
>>> In order to support Type2 CXL devices, wrap all of those concerns into
>>> an API that retrieves a root decoder (platform CXL window) that fits the
>>> specified constraints and the capacity available for a new region.
>>>
>>> Add a complementary function for releasing the reference to such root
>>> decoder.
>>>
>>> Based on https://lore.kernel.org/linux-cxl/168592159290.1948938.13522227102445462976.stgit@dwillia2-xfh.jf.intel.com/
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>>> ---
>> Hey Alejandro,
> 
> 
> Hi Ben,
> 
> 
>> I've been testing this on my setup and noticed a few issues when BIOS sets up the HDM decoders. It came down to 2 issues:
>>     1) Enabling "Specific Purpose Memory" added a Soft Reserve resource below the CXL window resource and broke
>>     this patch (more below)
> 
> 
> Maybe we should talk (first) about this internally as it is about AMD BIOS (I guess). I have been talking with the BIOS team about this EFI_MEMORY_SP vs EFI_RESERVED_MEMORY, and I'm afraid the discussion is not over yet :-).
> 
> 
>>     2) The endpoint decoder was already set up which broke DPA allocation and then CXL region creation (see my response
>>     to patch 18/22 for fix and explanation)
> 
> 
> Yes, if the BIOS configures the device HDM decoder, the current patchset does not do the right thing. As I said in the cover letter, my expectation at the time, and hopefully in the future as well, although I'm not sure about it, was the BIOS not doing so. Most of the implementation is based on QEMU, so I found this problem when dealing with a real system with a Type2 aware BIOS ... . I was tempted to include support for this case, but I did not do so for several reasons:
> 
> 
> 1) I want to think the patchset is close to being accepted and changes at this state could delay it further. After more than a year, and because this patchset is about "initial Type2 support", I think it is better to do so in a follow-up work, and when there is a client requiring it.
> 
> 2) Because that conversation with BIOS guys, I prefer to be sure what to do, as there are other things we need to clarify and in my opinion, far more important than current Type2 support.
> 
> 3) CXL is a fast moving part of the kernel and I bet we will find another case which the current patchset is not dealing with properly. In fact there is another report of devices with the BAR with CXL information being also used by the driver for other purposes and existing a problem when mapping the CXL registers after the driver did map the whole BAR.
> 
> 
> So, I think the current patchset where most of the API for Type2 drivers is implemented should go as soon as possible, which will facilitate those follow-up works for the case you describe and the other one about BAR mappings. If not, even if retirement is still far away for me, I'll be concerned about the impending future of this work ... but of course, this is my suggestion, so let's see other opinions about it.
>

My 2 cents is that if the current code set works as intended for the hardware/user you have then lets get that enabled and we can deal with the other cases as next iterations.

Are there any use cases where the BIOS need to pre-commit the type2 decoders instead of allowing the driver/OS to do so?

DJ 
> 
> Thank you.
> 
> 
>>
>> The fix I did for 1 is a bit hacky but it's essentially checking none of the resources below the CXL window are onlined as
>> system memory. It's roughly equivalent to what's being done in the CXL_PARTMODE_RAM case of cxl_region_probe(), but
>> I'm restricting the resources to "Soft Reserved" to be safe.
>>
>> The diff for 2 is pretty big. If you don't want to take it at this point I can send it as a follow up. In that case I'd definitely
>> add that auto regions won't work in at least the cover letter (and in the description of 18/22 as well?).
>>
>> ---
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index acaca64764bf..2d60131edff3 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -784,6 +784,19 @@ static int find_max_hpa(struct device *dev, void *data)
>>          lockdep_assert_held_read(&cxl_rwsem.region);
>>          res = cxlrd->res->child;
>>
>> +       /*
>> +        * BIOS may have marked the CXL window as soft reserved. Make sure it's
>> +        * free to use.
>> +        */
>> +       while (res && resource_size(res) == resource_size(cxlrd->res)) {
>> +               if ((res->flags & IORESOURCE_BUSY) ||
>> +                   (res->flags & IORESOURCE_SYSRAM) ||
>> +                   strcmp(res->name, "Soft Reserved") != 0)
>> +                       return 0;
>> +
>> +               res = res->child;
>> +       }
>> +
>>          /* With no resource child the whole parent resource is available */
>>          if (!res)
>>                  max = resource_size(cxlrd->res);

Powered by blists - more mailing lists