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

[Joao Martins]

On 07/03/2023 10:22, Joao Martins wrote:
> 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.
> 

Albeit this trace_vfio_listener_region_skip will have a new argument which the
caller passes e.g. region_add, region_skip, tracking_update.