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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3d97e89e-09ec-01c3-787c-0164c611cc4c@amd.com>
Date: Wed, 28 Aug 2024 17:06:00 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: "Li, Ming4" <ming4.li@...el.com>, alejandro.lucero-palau@....com,
 linux-cxl@...r.kernel.org, netdev@...r.kernel.org, dan.j.williams@...el.com,
 martin.habets@...inx.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
 richard.hughes@....com
Subject: Re: [PATCH v2 11/15] cxl: make region type based on endpoint type


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@....com wrote:
>>> From: Alejandro Lucero <alucerop@....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@....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.