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

[Duan, Zhenzhong]

>-----Original Message-----
>From: David Hildenbrand <david@redhat.com>
>Sent: Friday, July 22, 2022 2:49 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: pbonzini@redhat.com; peterx@redhat.com; f4bug@amsat.org
>Subject: Re: [PATCH] softmmu/physmem: Fix input parameters for
>flatview_access_allowed()
>
>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/

Will fix.

>
>>
>> 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.

Thanks David, I will remove it in v2.

Zhenzhong

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