Re: [RFC PATCH 0/3] cxl: Auto-create a region for Type-2 memdev attach

[Alejandro Lucero Palau <alejandro.lucero-palau@amd.com>]

Hi Richard,


Some comments below.


Thanks!


On 8/5/26 08:40, Richard Cheng wrote:
> A Type-2 accelerator driver calls devm_cxl_probe_mem() to register its
> memdev and get an HPA range, but today only if FW already committed a
> region. Real accelerators can have usable memory with no committed decoder,
> so get nothing.


The previous paragraph describes the current situation and the next one 
is about what the patchset tries to address. Maybe to explicitly make 
the difference would help people not so used to the subject.


> If no FW region is mapped, the core picks the device's unused manual
> DEVMEM decoder and a compatible x1 Type-2 RAM root


I had to look for this x1 reference ... and I would say it creates 
confusion. At least it does to me. Not sure if you meant interleaving, 
because I do not think you are referring to link lanes here ...


> , allocates the full
> volatile DPA and HPA, commits the decoder, and returns the range.


This is something requiring discussion or clarification. I think it 
would make sense the provider/driver specifying a DPA size instead of 
using the default full size. I think there is a good reason for this 
non-default size use: why would the kernel create a region from a CXL 
Type2 device using the full DPA size when the FW/BIOS did not do so?


This leads us to wondering why the FW/BIOS would not do so, the use 
case. Current Intel/AMD BIOS (I think you have the aim at ARM servers) 
are not allowing this case ... for a Type2 device having all the bits in 
place. If something requires to be specifically configured, would not 
the driver do so before using the CXL mem? If this logic makes sense, 
the auto-creation should not be the way to go.


The first 15 Type2 basic support patchset versions supported the case of 
a driver specifying the size for the cxl region to be created. And it 
was through a specific API call after the memdev was created. Last Type2 
patchset and the functionality finally merged only supported the case of 
auto-create regions from committed decoders, and using this final 
agreement for region attachment by the Type2 memdev/driver. I think it 
makes sense in that supported case to have the auto-create region but I 
can not see the reason for the case you are addressing now.


>   Unbind
> resets and removes it. Strict first cut with single decoder, IW=1, minimum
> granularity, first-compatible root.


I'm lost here.


>
> The design intent is that the provider F_LOCKs its region against userspace
> but must reset its own software region on detach. A plain flag would also
> allow reset on generic kill/delete paths, so we thread a reset context
> through teardown and commit rollback. devm_cxl_probe_mem() may now commit
> decoders.


If there is a real use case for this auto-create region from 
non-committed decoders, I think your patchset makes sense. But I'm 
afraid we need to discuss this further.


Thank you,

Alejandro.


> Testing result is in the following.
> - Built clean with clang/LLVM on arm64
> - cxl_test, type2_test=1. accel0 takes the unchanged attach path. accel1
>    drives auto_create -> a committed 512 MB RAM region. The test asserts
>    the 512 MB HPA range. committed state and 256 byte granularity confirmed
>    via sysfs.
> - Unbind tears the region down with no orphaned decoder, rebind re-creates
>    a fresh committed region.
> - Mock test only. Real accelerators whose FW commits a decoder take the
>    attach path, and vfio-cxl binds only FW-committed devices, so auto-create
>    has no real-HW caller yet.
>
> Best regards,
> Richard Cheng.
>
> Richard Cheng (3):
>    cxl/region: Reset software-created regions on memdev detach
>    cxl/region: Auto-create a region for memdev attach
>    cxl/test: Exercise Type-2 automatic region creation
>
>   drivers/cxl/core/region.c      | 422 +++++++++++++++++++++++++++++----
>   tools/testing/cxl/test/accel.c |   7 +
>   tools/testing/cxl/test/cxl.c   |  61 ++++-
>   3 files changed, 439 insertions(+), 51 deletions(-)
>
>
> base-commit: 1c6b4ceafc3b994871c29340e0c1ddb0af5800e7