Re: [PATCH v2 11/15] cxl: make region type based on endpoint type

Alejandro Lucero Palau
2024-08-28T16:06:11Z

On 7/16/24 09:13, Alejandro Lucero Palau wrote:
>
> On 7/16/24 08:14, Li, Ming4 wrote:
>> On 7/16/2024 1:28 AM, alejandro.lucero-palau@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Current code is expecting Type3 or CXL_DECODER_HOSTONLYMEM devices
>>> only.
>>> Suport for Type2 implies region type needs to be based on the endpoint
>>> type instead.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/region.c | 14 +++++++++-----
>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index ca464bfef77b..5cc71b8868bc 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -2645,7 +2645,8 @@ static ssize_t create_ram_region_show(struct
>>> device *dev,
>>>   }
>>>     static struct cxl_region *__create_region(struct
>>> cxl_root_decoder *cxlrd,
>>> -                      enum cxl_decoder_mode mode, int id)
>>> +                      enum cxl_decoder_mode mode, int id,
>>> +                      enum cxl_decoder_type target_type)
>>>   {
>>>       int rc;
>>>   @@ -2667,7 +2668,7 @@ static struct cxl_region
>>> *__create_region(struct cxl_root_decoder *cxlrd,
>>>           return ERR_PTR(-EBUSY);
>>>       }
>>>   -    return devm_cxl_add_region(cxlrd, id, mode,
>>> CXL_DECODER_HOSTONLYMEM);
>>> +    return devm_cxl_add_region(cxlrd, id, mode, target_type);
>>>   }
>>>     static ssize_t create_pmem_region_store(struct device *dev,
>>> @@ -2682,7 +2683,8 @@ static ssize_t create_pmem_region_store(struct
>>> device *dev,
>>>       if (rc != 1)
>>>           return -EINVAL;
>>>   -    cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id);
>>> +    cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id,
>>> +                   CXL_DECODER_HOSTONLYMEM);
>>>       if (IS_ERR(cxlr))
>>>           return PTR_ERR(cxlr);
>>>   @@ -2702,7 +2704,8 @@ static ssize_t
>>> create_ram_region_store(struct device *dev,
>>>       if (rc != 1)
>>>           return -EINVAL;
>>>   -    cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id);
>>> +    cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id,
>>> +                   CXL_DECODER_HOSTONLYMEM);
>>>       if (IS_ERR(cxlr))
>>>           return PTR_ERR(cxlr);
>>>   @@ -3364,7 +3367,8 @@ static struct cxl_region
>>> *construct_region(struct cxl_root_decoder *cxlrd,
>>>         do {
>>>           cxlr = __create_region(cxlrd, cxled->mode,
>>> - atomic_read(&cxlrd->region_id));
>>> +                       atomic_read(&cxlrd->region_id),
>>> +                       cxled->cxld.target_type);
>>>       } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
>>>         if (IS_ERR(cxlr)) {
>> I think that one more check between the type of root decoder and
>> endpoint decoder is necessary in this case. Currently, root decoder
>> type is hard coded to CXL_DECODER_HOSTONLYMEM, but it should be
>> CXL_DECODER_DEVMEM or CXL_DECODER_HOSTONLYMEM based on
>> cfmws->restrictions.
>>
>
> I think you are completely right.
>
> I will work on this looking also for other implications.
>
> Thanks
>
>
>>

I think the check could be performed inside cxl_attach_region where the
region type is already matched against the endpoint type. That is the
check triggering a failure for my Type2 support and the reason behind
this patch.

However, I think the way encoder type is managed requires a refactoring.
From the cedt cfmw restrictions I assume a decoder can support different
types and not restricted to just one, what is what the code does now
using a enumeration for the encoder type. With no restrictions, what is
the current implementation with qemu, I would say a root decoder should
be fine for a Type3 or a Type2. Adding that check for matching the root
decoder type with the region type is therefore not possible without
major changes. Because other potential restrictions like only supporting
RAM and no PMEM is not currently being managed, I think this initial
type2 support should be fine without the checking you propose, but a
following patch should address this problem, of course, assuming I'm not
wrong with all this.