| Message ID | 20230307020258.58215-7-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Device dirty page tracking | expand |
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 >
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. Thanks, C. > > 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 >> >
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 >>
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. > Thanks, > > C. > >> >> 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 >>> >> >
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.
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.
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()))); - 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; }