Re: [PATCH v4 07/14] vfio/common: Add helper to consolidate iova/end calculation
On 07/03/2023 09:52, Avihai Horon wrote: > > On 07/03/2023 4:02, Joao Martins wrote: >> External email: Use caution opening links or attachments >> >> >> In preparation to be used in device dirty tracking, move the code that >> calculates a iova/end range from the container/section. This avoids >> duplication on the common checks across listener callbacks. >> >> Signed-off-by: Joao Martins >> --- >> hw/vfio/common.c | 37 ++--- >> 1 file changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 54b4a4fc7daf..3a6491dbc523 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -961,6 +961,35 @@ static bool >> vfio_listener_valid_section(MemoryRegionSection *section) >> return true; >> } >> >> +/* >> + * Called for the dirty tracking memory listener to calculate the iova/end >> + * for a given memory region section. >> + */ > > Should we just delete this comment? The function name is pretty clear. > Besides that, the comment is not completely accurate -- in this patch we are > not > using it yet for dirty tracking and it's also used for region_{add,del}. > Yes, let me delete it. > Thanks. > >> +static bool vfio_get_section_iova_range(VFIOContainer *container, >> + MemoryRegionSection *section, >> + hwaddr *out_iova, hwaddr *out_end, >> + Int128 *out_llend) >> +{ >> + Int128 llend; >> + hwaddr iova; >> + >> + iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> + llend = int128_make64(section->offset_within_address_space); >> + llend = int128_add(llend, section->size); >> + llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); >> + >> + if (int128_ge(int128_make64(iova), llend)) { >> + return false; >> + } >> + >> + *out_iova = iova; >> + *out_end = int128_get64(int128_sub(llend, int128_one())); >> + if (out_llend) { >> + *out_llend = llend; >> + } >> + return true; >> +} >> + >> static void vfio_listener_region_add(MemoryListener *listener, >> MemoryRegionSection *section) >> { >> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> return; >> } >> >> - iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> - llend = int128_make64(section->offset_within_address_space); >> - llend = int128_add(llend, section->size); >> - llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); >> - >> - if (int128_ge(int128_make64(iova), llend)) { >> + if (!vfio_get_section_iova_range(container, section, &iova, &end, >> &llend)) { >> if (memory_region_is_ram_device(section->mr)) { >> trace_vfio_listener_region_add_no_dma_map( >> memory_region_name(section->mr), >> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> } >> return; >> } >> - end = int128_get64(int128_sub(llend, int128_one())); >> >> if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { >> hwaddr pgsize = 0; >> -- >> 2.17.2 >>
Re: [PATCH v4 07/14] vfio/common: Add helper to consolidate iova/end calculation
On 07/03/2023 02:40, Alex Williamson wrote: > On Tue, 7 Mar 2023 02:02:51 + > Joao Martins wrote: > >> In preparation to be used in device dirty tracking, move the code that >> calculates a iova/end range from the container/section. This avoids >> duplication on the common checks across listener callbacks. >> >> Signed-off-by: Joao Martins >> --- >> hw/vfio/common.c | 37 ++--- >> 1 file changed, 30 insertions(+), 7 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 54b4a4fc7daf..3a6491dbc523 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -961,6 +961,35 @@ static bool >> vfio_listener_valid_section(MemoryRegionSection *section) >> return true; >> } >> >> +/* >> + * Called for the dirty tracking memory listener to calculate the iova/end >> + * for a given memory region section. >> + */ >> +static bool vfio_get_section_iova_range(VFIOContainer *container, >> +MemoryRegionSection *section, >> +hwaddr *out_iova, hwaddr *out_end, >> +Int128 *out_llend) >> +{ >> +Int128 llend; >> +hwaddr iova; >> + >> +iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> +llend = int128_make64(section->offset_within_address_space); >> +llend = int128_add(llend, section->size); >> +llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); >> + >> +if (int128_ge(int128_make64(iova), llend)) { >> +return false; >> +} >> + >> +*out_iova = iova; >> +*out_end = int128_get64(int128_sub(llend, int128_one())); >> +if (out_llend) { >> +*out_llend = llend; >> +} >> +return true; >> +} >> + >> static void vfio_listener_region_add(MemoryListener *listener, >> MemoryRegionSection *section) >> { >> @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> return; >> } >> >> -iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> -llend = int128_make64(section->offset_within_address_space); >> -llend = int128_add(llend, section->size); >> -llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); >> - >> -if (int128_ge(int128_make64(iova), llend)) { >> +if (!vfio_get_section_iova_range(container, section, &iova, &end, >> &llend)) { >> if (memory_region_is_ram_device(section->mr)) { >> trace_vfio_listener_region_add_no_dma_map( >> memory_region_name(section->mr), >> @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> } >> return; >> } >> -end = int128_get64(int128_sub(llend, int128_one())); >> >> if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { >> hwaddr pgsize = 0; > > Shouldn't this convert vfio_listener_region_del() too? Thanks, Yeap.
Re: [PATCH v4 07/14] vfio/common: Add helper to consolidate iova/end calculation
On 07/03/2023 4:02, Joao Martins wrote: External email: Use caution opening links or attachments In preparation to be used in device dirty tracking, move the code that calculates a iova/end range from the container/section. This avoids duplication on the common checks across listener callbacks. Signed-off-by: Joao Martins --- hw/vfio/common.c | 37 ++--- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 54b4a4fc7daf..3a6491dbc523 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -961,6 +961,35 @@ static bool vfio_listener_valid_section(MemoryRegionSection *section) return true; } +/* + * Called for the dirty tracking memory listener to calculate the iova/end + * for a given memory region section. + */ Should we just delete this comment? The function name is pretty clear. Besides that, the comment is not completely accurate -- in this patch we are not using it yet for dirty tracking and it's also used for region_{add,del}. Thanks. +static bool vfio_get_section_iova_range(VFIOContainer *container, +MemoryRegionSection *section, +hwaddr *out_iova, hwaddr *out_end, +Int128 *out_llend) +{ +Int128 llend; +hwaddr iova; + +iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); +llend = int128_make64(section->offset_within_address_space); +llend = int128_add(llend, section->size); +llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); + +if (int128_ge(int128_make64(iova), llend)) { +return false; +} + +*out_iova = iova; +*out_end = int128_get64(int128_sub(llend, int128_one())); +if (out_llend) { +*out_llend = llend; +} +return true; +} + static void vfio_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener *listener, return; } -iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); -llend = int128_make64(section->offset_within_address_space); -llend = int128_add(llend, section->size); -llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); - -if (int128_ge(int128_make64(iova), llend)) { +if (!vfio_get_section_iova_range(container, section, &iova, &end, &llend)) { if (memory_region_is_ram_device(section->mr)) { trace_vfio_listener_region_add_no_dma_map( memory_region_name(section->mr), @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener *listener, } return; } -end = int128_get64(int128_sub(llend, int128_one())); if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { hwaddr pgsize = 0; -- 2.17.2
Re: [PATCH v4 07/14] vfio/common: Add helper to consolidate iova/end calculation
On Tue, 7 Mar 2023 02:02:51 + Joao Martins wrote: > In preparation to be used in device dirty tracking, move the code that > calculates a iova/end range from the container/section. This avoids > duplication on the common checks across listener callbacks. > > Signed-off-by: Joao Martins > --- > hw/vfio/common.c | 37 ++--- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 54b4a4fc7daf..3a6491dbc523 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -961,6 +961,35 @@ static bool > vfio_listener_valid_section(MemoryRegionSection *section) > return true; > } > > +/* > + * Called for the dirty tracking memory listener to calculate the iova/end > + * for a given memory region section. > + */ > +static bool vfio_get_section_iova_range(VFIOContainer *container, > +MemoryRegionSection *section, > +hwaddr *out_iova, hwaddr *out_end, > +Int128 *out_llend) > +{ > +Int128 llend; > +hwaddr iova; > + > +iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); > +llend = int128_make64(section->offset_within_address_space); > +llend = int128_add(llend, section->size); > +llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); > + > +if (int128_ge(int128_make64(iova), llend)) { > +return false; > +} > + > +*out_iova = iova; > +*out_end = int128_get64(int128_sub(llend, int128_one())); > +if (out_llend) { > +*out_llend = llend; > +} > +return true; > +} > + > static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > @@ -976,12 +1005,7 @@ static void vfio_listener_region_add(MemoryListener > *listener, > return; > } > > -iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); > -llend = int128_make64(section->offset_within_address_space); > -llend = int128_add(llend, section->size); > -llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask())); > - > -if (int128_ge(int128_make64(iova), llend)) { > +if (!vfio_get_section_iova_range(container, section, &iova, &end, > &llend)) { > if (memory_region_is_ram_device(section->mr)) { > trace_vfio_listener_region_add_no_dma_map( > memory_region_name(section->mr), > @@ -991,7 +1015,6 @@ static void vfio_listener_region_add(MemoryListener > *listener, > } > return; > } > -end = int128_get64(int128_sub(llend, int128_one())); > > if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > hwaddr pgsize = 0; Shouldn't this convert vfio_listener_region_del() too? Thanks, Alex