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

[Joao Martins <joao.m.martins@oracle.com>]

On 07/03/2023 09: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 should say that the way I distinct both of them is because there's a
dma_tracking_update new tracepoint where you can tell it's from. And there's a
region_add/del tracepoints. So despite the name we won't get confused IMHO

> But other than that:
> 
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
> Thanks!

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