diff mbox series

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

Message ID 20240715172835.24757-12-alejandro.lucero-palau@amd.com (mailing list archive)
State Not Applicable
Headers show
Series cxl: add Type2 device support | expand

Commit Message

Lucero Palau, Alejandro July 15, 2024, 5:28 p.m. UTC 
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(-)

Comments

Li, Ming4 July 16, 2024, 7:14 a.m. UTC | #1 
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.
Alejandro Lucero Palau July 16, 2024, 8:13 a.m. UTC | #2 
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


>
Alejandro Lucero Palau Aug. 28, 2024, 4:06 p.m. UTC | #3 
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.
diff mbox series

Patch

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)) {