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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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/

>