Re: [PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

[David Hildenbrand <david@redhat.com>]

On 22.06.22 03:28, Zhenzhong Duan wrote:
> The comment of flatview_access_allowed() suggests to pass address
> within that memory region, this isn't ture in some call sites.

s/ture/true/

> 
> This makes qemu log in flatview_access_allowed() confusing and
> potential risk if the input parameter will be checked in the future.
> 
> Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_ACCESS_ERROR")

My opinion is to not use fixes tags unless there is really something
broken -- a user-visible effect of a BUG exists.

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  softmmu/physmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index fb16be57a6c6..214cb04c8fc3 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2850,7 +2850,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>          return MEMTX_ACCESS_ERROR;
>      }
>      return flatview_write_continue(fv, addr, attrs, buf, len,
> @@ -2917,7 +2917,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>          return MEMTX_ACCESS_ERROR;
>      }
>      return flatview_read_continue(fv, addr, attrs, buf, len,

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb