Re: [PATCH v4 06/14] vfio/common: Consolidate skip/invalid section into helper

[Joao Martins]

On 07/03/2023 09:47, Cédric Le Goater wrote:
> On 3/7/23 10:13, Avihai Horon wrote:
>> On 07/03/2023 4:02, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>> The checks are replicated against region_add and region_del
>>> and will be soon added in another memory listener dedicated
>>> for dirty tracking.
>>>
>>> Move these into a new helper for avoid duplication.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/vfio/common.c | 52 +++++++++++++++++++-----------------------------
>>>   1 file changed, 21 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 99acb998eb14..54b4a4fc7daf 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -933,23 +933,14 @@ static bool
>>> vfio_known_safe_misalignment(MemoryRegionSection *section)
>>>       return true;
>>>   }
>>>
>>> -static void vfio_listener_region_add(MemoryListener *listener,
>>> -                                     MemoryRegionSection *section)
>>> +static bool vfio_listener_valid_section(MemoryRegionSection *section)
>>>   {
>>> -    VFIOContainer *container = container_of(listener, VFIOContainer, 
>>> listener);
>>> -    hwaddr iova, end;
>>> -    Int128 llend, llsize;
>>> -    void *vaddr;
>>> -    int ret;
>>> -    VFIOHostDMAWindow *hostwin;
>>> -    Error *err = NULL;
>>> -
>>>       if (vfio_listener_skipped_section(section)) {
>>>           trace_vfio_listener_region_add_skip(
>>>                   section->offset_within_address_space,
>>>                   section->offset_within_address_space +
>>>                   int128_get64(int128_sub(section->size, int128_one())));
>>
>> The original code uses two different traces depending on add or del --
>> trace_vfio_listener_region_{add,del}_skip.
>> Should we combine the two traces into a single trace? If the distinction is
>> important then maybe pass a flag or the caller name to indicate whether it's
>> add, del or dirty tracking update?
> 
> I think introducing a new trace event 'trace_vfio_listener_region_skip'
> to replace 'trace_vfio_listener_region_add_skip' above should be enough.
> 
OK, I'll introduce a predecessor patch to change the name.

> Thanks,
> 
> C.
> 
>>
>> But other than that:
>>
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>>
>> Thanks.
>>
>>> -        return;
>>> +        return false;
>>>       }
>>>
>>>       if (unlikely((section->offset_within_address_space &
>>> @@ -964,6 +955,24 @@ static void vfio_listener_region_add(MemoryListener
>>> *listener,
>>>                            section->offset_within_region,
>>>                            qemu_real_host_page_size());
>>>           }
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static void vfio_listener_region_add(MemoryListener *listener,
>>> +                                     MemoryRegionSection *section)
>>> +{
>>> +    VFIOContainer *container = container_of(listener, VFIOContainer, 
>>> listener);
>>> +    hwaddr iova, end;
>>> +    Int128 llend, llsize;
>>> +    void *vaddr;
>>> +    int ret;
>>> +    VFIOHostDMAWindow *hostwin;
>>> +    Error *err = NULL;
>>> +
>>> +    if (!vfio_listener_valid_section(section)) {
>>>           return;
>>>       }
>>>
>>> @@ -1182,26 +1191,7 @@ static void vfio_listener_region_del(MemoryListener
>>> *listener,
>>>       int ret;
>>>       bool try_unmap = true;
>>>
>>> -    if (vfio_listener_skipped_section(section)) {
>>> -        trace_vfio_listener_region_del_skip(
>>> -                section->offset_within_address_space,
>>> -                section->offset_within_address_space +
>>> -                int128_get64(int128_sub(section->size, int128_one())));
>>> -        return;
>>> -    }
>>> -
>>> -    if (unlikely((section->offset_within_address_space &
>>> -                  ~qemu_real_host_page_mask()) !=
>>> -                 (section->offset_within_region &
>>> ~qemu_real_host_page_mask()))) {
>>> -        if (!vfio_known_safe_misalignment(section)) {
>>> -            error_report("%s received unaligned region %s iova=0x%"PRIx64
>>> -                         " offset_within_region=0x%"PRIx64
>>> -                         " qemu_real_host_page_size=0x%"PRIxPTR,
>>> -                         __func__, memory_region_name(section->mr),
>>> -                         section->offset_within_address_space,
>>> -                         section->offset_within_region,
>>> -                         qemu_real_host_page_size());
>>> -        }
>>> +    if (!vfio_listener_valid_section(section)) {
>>>           return;
>>>       }
>>>
>>> -- 
>>> 2.17.2
>>>
>>
>