Re: [PATCH v3 02/18] system/memory: Better describe @plen argument of flatview_translate()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Thomas,

On 30/9/25 10:24, Thomas Huth wrote:
> On 30/09/2025 10.21, Philippe Mathieu-Daudé wrote:
>> flatview_translate()'s @plen argument is output-only and can be NULL.
>>
>> When Xen is enabled, only update @plen_out when non-NULL.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/system/memory.h | 5 +++--
>>   system/physmem.c        | 9 +++++----
>>   2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/memory.h b/include/system/memory.h
>> index aa85fc27a10..3e5bf3ef05e 100644
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -2992,13 +2992,14 @@ IOMMUTLBEntry 
>> address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>>    * @addr: address within that address space
>>    * @xlat: pointer to address within the returned memory region 
>> section's
>>    * #MemoryRegion.
>> - * @len: pointer to length
>> + * @plen_out: pointer to valid read/write length of the translated 
>> address.
>> + *            It can be @NULL when we don't care about it.
>>    * @is_write: indicates the transfer direction
>>    * @attrs: memory attributes
>>    */
>>   MemoryRegion *flatview_translate(FlatView *fv,
>>                                    hwaddr addr, hwaddr *xlat,
>> -                                 hwaddr *len, bool is_write,
>> +                                 hwaddr *plen_out, bool is_write,
>>                                    MemTxAttrs attrs);
>>   static inline MemoryRegion *address_space_translate(AddressSpace *as,
>> diff --git a/system/physmem.c b/system/physmem.c
>> index 8a8be3a80e2..86422f294e2 100644
>> --- a/system/physmem.c
>> +++ b/system/physmem.c
>> @@ -566,7 +566,7 @@ iotlb_fail:
>>   /* Called from RCU critical section */
>>   MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr 
>> *xlat,
>> -                                 hwaddr *plen, bool is_write,
>> +                                 hwaddr *plen_out, bool is_write,
>>                                    MemTxAttrs attrs)
>>   {
>>       MemoryRegion *mr;
>> @@ -574,13 +574,14 @@ MemoryRegion *flatview_translate(FlatView *fv, 
>> hwaddr addr, hwaddr *xlat,
>>       AddressSpace *as = NULL;
>>       /* This can be MMIO, so setup MMIO bit. */
>> -    section = flatview_do_translate(fv, addr, xlat, plen, NULL,
>> +    section = flatview_do_translate(fv, addr, xlat, plen_out, NULL,
>>                                       is_write, true, &as, attrs);
>>       mr = section.mr;
>> -    if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs)) {
>> +    if (xen_enabled() && plen_out && memory_access_is_direct(mr, 
>> is_write,
>> +                                                             attrs)) {
>>           hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) 
>> - addr;
>> -        *plen = MIN(page, *plen);
>> +        *plen_out = MIN(page, *plen_out);
>>       }
> 
> My question from the previous version is still unanswered:
> 
> https://lore.kernel.org/qemu- 
> devel/22ff756a-51a2-43f4-8fe1-05f17ff4a371@redhat.com/

This patches
- checks for plen not being NULL
- describes it as
   "When Xen is enabled, only update @plen_out when non-NULL."
- mention that in the updated flatview_translate() documentation
   "It can be @NULL when we don't care about it." as documented for
   the flatview_do_translate() callee in commit d5e5fafd11b ("exec:
   add page_mask for flatview_do_translate")

before:

   it was not clear whether we can pass plen=NULL without having
   to look at the code.

after:

   no change when plen is not NULL, we can pass plen=NULL safely
   (it is documented).

I shouldn't be understanding your original question, do you mind
rewording it?

Thanks,

Phil.