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

["Cédric Le Goater" <clg@redhat.com>]

On 3/7/23 12:00, Joao Martins wrote:
> 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.

yes. That's fine. The important part is to be able to select a family
of events with '-trace vfio_listener_region*'

Thanks,

C.