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

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

On 30/9/25 11:18, Thomas Huth wrote:
> On 30/09/2025 10.31, Philippe Mathieu-Daudé wrote:
>> 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?
> 
> Ah, you've updated the patch in v3 to include a check for plen_out in 
> the if-statement! It was not there in v2. Ok, this should be fine now:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> I just re-complained since you did not respond to my mail in v2, and 
> when I looked at the changelog in your v3 cover letter, you did not 
> mention the modification here, so I blindly assumed that this patch was 
> unchanged.

Ah I see... OK I'll try to be more explicit in my respins.

Thanks for your review!

Phil.