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

[Avihai Horon]

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?

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