[RESEND] hw/cxl: fix the determination of illegal physical addresses

[RESEND] hw/cxl: fix the determination of illegal physical addresses

[peng guo]

When physical address range in the input payload of scan media command
exceeds static_mem_size but does not exceed the sum of static and dynamic
memory, the scan media mailbox command unexpectedly returns an error code
which is CXL_MBOX_INVALID_PA.

This patch determines whether the physical address is valid in two cases. 
If dynamic memory exists, check whether the address range of the request 
exceeds the range of static memory and dynamic memory.If dynamic memory 
does not exist, then check whether the address range of the request 
exceeds the static memory size.

Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")
Signed-off-by: peng guo <engguopeng@buaa.edu.cn>
---
 hw/cxl/cxl-mailbox-utils.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3ebbd32e10..b23c6b9b0b 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
     }
     query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
 
-    if (query_start + query_length > cxl_dstate->static_mem_size) {
-        return CXL_MBOX_INVALID_PA;
-    }
-    if (ct3d->dc.num_regions && query_start + query_length >=
+    if (ct3d->dc.num_regions) {
+        if (query_start + query_length >=
             cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
+                return CXL_MBOX_INVALID_PA;
+            }
+    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
         return CXL_MBOX_INVALID_PA;
     }
 
-- 
2.43.0

Re: [RESEND] hw/cxl: fix the determination of illegal physical addresses

[Jonathan Cameron]

On Mon, 19 Aug 2024 20:03:17 +0800
peng guo <engguopeng@buaa.edu.cn> wrote:

> When physical address range in the input payload of scan media command
> exceeds static_mem_size but does not exceed the sum of static and dynamic
> memory, the scan media mailbox command unexpectedly returns an error code
> which is CXL_MBOX_INVALID_PA.
> 
> This patch determines whether the physical address is valid in two cases. 
> If dynamic memory exists, check whether the address range of the request 
> exceeds the range of static memory and dynamic memory.If dynamic memory 
> does not exist, then check whether the address range of the request 
> exceeds the static memory size.
> 
> Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")
Is that the right one, this code is affecting cmd_media_scan_media()
not the capabilities one which always limits to static_mem_size and
hence also looks wrong.

> Signed-off-by: peng guo <engguopeng@buaa.edu.cn>

As with the other patch, this needs to go to qemu-devel list
+ both should have gone to Davidlohr as author the patch you
are fixing (sort of it, it's mostly down to what order patches
landed in I think).

Fan, Davidlohr, do we want to just cover the DCD regions as
well with all the scan_media commands?


> ---
>  hw/cxl/cxl-mailbox-utils.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3ebbd32e10..b23c6b9b0b 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
>      }
>      query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
>  
> -    if (query_start + query_length > cxl_dstate->static_mem_size) {
> -        return CXL_MBOX_INVALID_PA;
> -    }
> -    if (ct3d->dc.num_regions && query_start + query_length >=
> +    if (ct3d->dc.num_regions) {
> +        if (query_start + query_length >=
>              cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
> +                return CXL_MBOX_INVALID_PA;
> +            }
> +    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
>          return CXL_MBOX_INVALID_PA;
>      }
Can we not rely on dc.total_capacity == 0 if num_regions == 0/

>  

Re: [RESEND] hw/cxl: fix the determination of illegal physical addresses

[Davidlohr Bueso]

On Fri, 23 Aug 2024, Jonathan Cameron wrote:\n
>On Mon, 19 Aug 2024 20:03:17 +0800
>peng guo <engguopeng@buaa.edu.cn> wrote:
>
>> When physical address range in the input payload of scan media command
>> exceeds static_mem_size but does not exceed the sum of static and dynamic
>> memory, the scan media mailbox command unexpectedly returns an error code
>> which is CXL_MBOX_INVALID_PA.
>>
>> This patch determines whether the physical address is valid in two cases.
>> If dynamic memory exists, check whether the address range of the request
>> exceeds the range of static memory and dynamic memory.If dynamic memory
							 ^ space for a new sentence
>> does not exist, then check whether the address range of the request
>> exceeds the static memory size.
>>
>> Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")
>Is that the right one, this code is affecting cmd_media_scan_media()
>not the capabilities one which always limits to static_mem_size and
>hence also looks wrong.

Yeah it is the right one - both commands were merged together (and
yes, they both need to be updated for dcd).

Maybe have a helper to consolidate all cases where such ranges are used?

uint64_t ct3d_get_total_size(CXLDeviceState *cxl_dstate, CXLType3Dev *ct3d)
{

	return cxl_dstate->static_mem_size + ct3d->dc.total_capacity;
}

>
>> Signed-off-by: peng guo <engguopeng@buaa.edu.cn>
>
>As with the other patch, this needs to go to qemu-devel list
>+ both should have gone to Davidlohr as author the patch you
>are fixing (sort of it, it's mostly down to what order patches
>landed in I think).
>
>Fan, Davidlohr, do we want to just cover the DCD regions as
>well with all the scan_media commands?

Yeah, I think so - particularly since poison management already
considers dcd for CXL_MBOX_INVALID_PA.

Thanks,
Davidlohr

>
>
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 3ebbd32e10..b23c6b9b0b 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
>>      }
>>      query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
>>
>> -    if (query_start + query_length > cxl_dstate->static_mem_size) {
>> -        return CXL_MBOX_INVALID_PA;
>> -    }
>> -    if (ct3d->dc.num_regions && query_start + query_length >=
>> +    if (ct3d->dc.num_regions) {
>> +        if (query_start + query_length >=
>>              cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
>> +                return CXL_MBOX_INVALID_PA;
>> +            }
>> +    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
>>          return CXL_MBOX_INVALID_PA;
>>      }
>Can we not rely on dc.total_capacity == 0 if num_regions == 0/
>
>>
>

Re: [RESEND] hw/cxl: fix the determination of illegal physical addresses

[peng guo]

On Wed, Sep 04, 2024 at 01:01:55PM -0700, Davidlohr Bueso wrote:
> On Fri, 23 Aug 2024, Jonathan Cameron wrote:\n
> > On Mon, 19 Aug 2024 20:03:17 +0800
> > peng guo <engguopeng@buaa.edu.cn> wrote:
> > 
> > > When physical address range in the input payload of scan media command
> > > exceeds static_mem_size but does not exceed the sum of static and dynamic
> > > memory, the scan media mailbox command unexpectedly returns an error code
> > > which is CXL_MBOX_INVALID_PA.
> > > 
> > > This patch determines whether the physical address is valid in two cases.
> > > If dynamic memory exists, check whether the address range of the request
> > > exceeds the range of static memory and dynamic memory.If dynamic memory
> 							 ^ space for a new sentence
> > > does not exist, then check whether the address range of the request
> > > exceeds the static memory size.
> > > 
> > > Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")
> > Is that the right one, this code is affecting cmd_media_scan_media()
> > not the capabilities one which always limits to static_mem_size and
> > hence also looks wrong.
> 
> Yeah it is the right one - both commands were merged together (and
> yes, they both need to be updated for dcd).
> 
> Maybe have a helper to consolidate all cases where such ranges are used?
> 
> uint64_t ct3d_get_total_size(CXLDeviceState *cxl_dstate, CXLType3Dev *ct3d)
> {
> 
> 	return cxl_dstate->static_mem_size + ct3d->dc.total_capacity;
> }
> 
> > 
> > > Signed-off-by: peng guo <engguopeng@buaa.edu.cn>
> > 
> > As with the other patch, this needs to go to qemu-devel list
> > + both should have gone to Davidlohr as author the patch you
> > are fixing (sort of it, it's mostly down to what order patches
> > landed in I think).
> > 
> > Fan, Davidlohr, do we want to just cover the DCD regions as
> > well with all the scan_media commands?
> 
> Yeah, I think so - particularly since poison management already
> considers dcd for CXL_MBOX_INVALID_PA.
> 
> Thanks,
> Davidlohr

Hi Jonathan, Davidlohr,

Sorry for the late reply.

I’m following up on the discussion about the `scan media` command behavior 
when dynamic memory is present. Based on our earlier exchange, it seems 
that the command currently returns `CXL_MBOX_INVALID_PA` if the requested 
address falls within the dynamic memory region — even though this should 
be a valid case when DCD is enabled.

Just to confirm: should this be treated as a bug that needs to be fixed with 
an updated patch?

If so, I’d be happy to prepare a new version and send it to the proper recipients.

Thanks again for your feedback.

Best regards,
Peng Guo

> 
> > 
> > 
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 3ebbd32e10..b23c6b9b0b 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
> > >      }
> > >      query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> > > 
> > > -    if (query_start + query_length > cxl_dstate->static_mem_size) {
> > > -        return CXL_MBOX_INVALID_PA;
> > > -    }
> > > -    if (ct3d->dc.num_regions && query_start + query_length >=
> > > +    if (ct3d->dc.num_regions) {
> > > +        if (query_start + query_length >=
> > >              cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
> > > +                return CXL_MBOX_INVALID_PA;
> > > +            }
> > > +    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
> > >          return CXL_MBOX_INVALID_PA;
> > >      }
> > Can we not rely on dc.total_capacity == 0 if num_regions == 0/
> > 
> > > 
> > 

Re: [RESEND] hw/cxl: fix the determination of illegal physical addresses

[Jonathan Cameron]

On Tue, 5 Aug 2025 22:31:36 +0800
peng guo <engguopeng@buaa.edu.cn> wrote:

> On Wed, Sep 04, 2024 at 01:01:55PM -0700, Davidlohr Bueso wrote:
> > On Fri, 23 Aug 2024, Jonathan Cameron wrote:\n  
> > > On Mon, 19 Aug 2024 20:03:17 +0800
> > > peng guo <engguopeng@buaa.edu.cn> wrote:
> > >   
> > > > When physical address range in the input payload of scan media command
> > > > exceeds static_mem_size but does not exceed the sum of static and dynamic
> > > > memory, the scan media mailbox command unexpectedly returns an error code
> > > > which is CXL_MBOX_INVALID_PA.
> > > > 
> > > > This patch determines whether the physical address is valid in two cases.
> > > > If dynamic memory exists, check whether the address range of the request
> > > > exceeds the range of static memory and dynamic memory.If dynamic memory  
> > 							 ^ space for a new sentence  
> > > > does not exist, then check whether the address range of the request
> > > > exceeds the static memory size.
> > > > 
> > > > Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")  
> > > Is that the right one, this code is affecting cmd_media_scan_media()
> > > not the capabilities one which always limits to static_mem_size and
> > > hence also looks wrong.  
> > 
> > Yeah it is the right one - both commands were merged together (and
> > yes, they both need to be updated for dcd).
> > 
> > Maybe have a helper to consolidate all cases where such ranges are used?
> > 
> > uint64_t ct3d_get_total_size(CXLDeviceState *cxl_dstate, CXLType3Dev *ct3d)
> > {
> > 
> > 	return cxl_dstate->static_mem_size + ct3d->dc.total_capacity;
> > }
> >   
> > >   
> > > > Signed-off-by: peng guo <engguopeng@buaa.edu.cn>  
> > > 
> > > As with the other patch, this needs to go to qemu-devel list
> > > + both should have gone to Davidlohr as author the patch you
> > > are fixing (sort of it, it's mostly down to what order patches
> > > landed in I think).
> > > 
> > > Fan, Davidlohr, do we want to just cover the DCD regions as
> > > well with all the scan_media commands?  
> > 
> > Yeah, I think so - particularly since poison management already
> > considers dcd for CXL_MBOX_INVALID_PA.
> > 
> > Thanks,
> > Davidlohr  
> 
> Hi Jonathan, Davidlohr,
> 
> Sorry for the late reply.
> 
> I’m following up on the discussion about the `scan media` command behavior 
> when dynamic memory is present. Based on our earlier exchange, it seems 
> that the command currently returns `CXL_MBOX_INVALID_PA` if the requested 
> address falls within the dynamic memory region — even though this should 
> be a valid case when DCD is enabled.
> 
> Just to confirm: should this be treated as a bug that needs to be fixed with 
> an updated patch?
I'm not sure a device is obliged to support all features on DCD, but there is
no clean way to discover if it does or not.

So, probably not worth backporting to stable QEMU (as a bug) but worth fixing
going forwards.
> 
> If so, I’d be happy to prepare a new version and send it to the proper recipients.
> 
Great!

Jonathan

> Thanks again for your feedback.
> 
> Best regards,
> Peng Guo
> 
> >   
> > > 
> > >   
> > > > ---
> > > >  hw/cxl/cxl-mailbox-utils.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > index 3ebbd32e10..b23c6b9b0b 100644
> > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > > @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
> > > >      }
> > > >      query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> > > > 
> > > > -    if (query_start + query_length > cxl_dstate->static_mem_size) {
> > > > -        return CXL_MBOX_INVALID_PA;
> > > > -    }
> > > > -    if (ct3d->dc.num_regions && query_start + query_length >=
> > > > +    if (ct3d->dc.num_regions) {
> > > > +        if (query_start + query_length >=
> > > >              cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
> > > > +                return CXL_MBOX_INVALID_PA;
> > > > +            }
> > > > +    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
> > > >          return CXL_MBOX_INVALID_PA;
> > > >      }  
> > > Can we not rely on dc.total_capacity == 0 if num_regions == 0/
> > >   
> > > >   
> > >   
> 
>