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

[Cédric Le Goater]

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.