Abstract this operation, that will be reused when validating the region
against the iova range that the device supports.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ea1aa71ad8..a1de6c7c9c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -24,6 +24,15 @@
 #include "trace.h"
 #include "qemu-common.h"
 
+static Int128 vhost_vdpa_section_end(const MemoryRegionSection *section)
+{
+    Int128 llend = int128_make64(section->offset_within_address_space);
+    llend = int128_add(llend, section->size);
+    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
+
+    return llend;
+}
+
 static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
 {
     return (!memory_region_is_ram(section->mr) &&
@@ -160,10 +169,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
     }
 
     iova = TARGET_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(TARGET_PAGE_MASK));
-
+    llend = vhost_vdpa_section_end(section);
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
@@ -221,9 +227,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
     }
 
     iova = TARGET_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(TARGET_PAGE_MASK));
+    llend = vhost_vdpa_section_end(section);
 
     trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
 
-- 
2.27.0

On Tue, Oct 05, 2021 at 10:01:30AM +0200, Eugenio Pérez wrote:
> Abstract this operation, that will be reused when validating the region
> against the iova range that the device supports.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Note that as defined end is actually 1 byte beyond end of section.
As such it can e.g. overflow if cast to u64.
So be careful to use int128 ops with it.
Also - document?

> ---
>  hw/virtio/vhost-vdpa.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index ea1aa71ad8..a1de6c7c9c 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -24,6 +24,15 @@
>  #include "trace.h"
>  #include "qemu-common.h"
>  
> +static Int128 vhost_vdpa_section_end(const MemoryRegionSection *section)
> +{
> +    Int128 llend = int128_make64(section->offset_within_address_space);
> +    llend = int128_add(llend, section->size);
> +    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> +
> +    return llend;
> +}
> +
>  static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
>  {
>      return (!memory_region_is_ram(section->mr) &&
> @@ -160,10 +169,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
>      }
>  
>      iova = TARGET_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(TARGET_PAGE_MASK));
> -
> +    llend = vhost_vdpa_section_end(section);
>      if (int128_ge(int128_make64(iova), llend)) {
>          return;
>      }
> @@ -221,9 +227,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
>      }
>  
>      iova = TARGET_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(TARGET_PAGE_MASK));
> +    llend = vhost_vdpa_section_end(section);
>  
>      trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
>  
> -- 
> 2.27.0

On Tue, Oct 5, 2021 at 10:15 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 05, 2021 at 10:01:30AM +0200, Eugenio Pérez wrote:
> > Abstract this operation, that will be reused when validating the region
> > against the iova range that the device supports.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> Note that as defined end is actually 1 byte beyond end of section.
> As such it can e.g. overflow if cast to u64.
> So be careful to use int128 ops with it.

You are right, but this is only the result of extracting "llend"
calculation in its own function, since it is going to be used a third
time in the next commit. This next commit contains a mistake because
of this, as you pointed out.

Since "last" would be a very misleading name, do you think we could
give a better name / type to it?

> Also - document?

It will be documented with that ("It returns one byte beyond end of
section" or similar) too.

Thanks!

>
> > ---
> >  hw/virtio/vhost-vdpa.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index ea1aa71ad8..a1de6c7c9c 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -24,6 +24,15 @@
> >  #include "trace.h"
> >  #include "qemu-common.h"
> >
> > +static Int128 vhost_vdpa_section_end(const MemoryRegionSection *section)
> > +{
> > +    Int128 llend = int128_make64(section->offset_within_address_space);
> > +    llend = int128_add(llend, section->size);
> > +    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> > +
> > +    return llend;
> > +}
> > +
> >  static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
> >  {
> >      return (!memory_region_is_ram(section->mr) &&
> > @@ -160,10 +169,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> >      }
> >
> >      iova = TARGET_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(TARGET_PAGE_MASK));
> > -
> > +    llend = vhost_vdpa_section_end(section);
> >      if (int128_ge(int128_make64(iova), llend)) {
> >          return;
> >      }
> > @@ -221,9 +227,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
> >      }
> >
> >      iova = TARGET_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(TARGET_PAGE_MASK));
> > +    llend = vhost_vdpa_section_end(section);
> >
> >      trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
> >
> > --
> > 2.27.0
>

On Tue, Oct 05, 2021 at 11:52:37AM +0200, Eugenio Perez Martin wrote:
> On Tue, Oct 5, 2021 at 10:15 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 05, 2021 at 10:01:30AM +0200, Eugenio Pérez wrote:
> > > Abstract this operation, that will be reused when validating the region
> > > against the iova range that the device supports.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >
> > Note that as defined end is actually 1 byte beyond end of section.
> > As such it can e.g. overflow if cast to u64.
> > So be careful to use int128 ops with it.
> 
> You are right, but this is only the result of extracting "llend"
> calculation in its own function, since it is going to be used a third
> time in the next commit. This next commit contains a mistake because
> of this, as you pointed out.
> 
> Since "last" would be a very misleading name, do you think we could
> give a better name / type to it?
> 
> > Also - document?
> 
> It will be documented with that ("It returns one byte beyond end of
> section" or similar) too.
> 
> Thanks!

that's how c++ containers work so maybe it's not too bad as long
as we document this carefully.

> >
> > > ---
> > >  hw/virtio/vhost-vdpa.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index ea1aa71ad8..a1de6c7c9c 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -24,6 +24,15 @@
> > >  #include "trace.h"
> > >  #include "qemu-common.h"
> > >
> > > +static Int128 vhost_vdpa_section_end(const MemoryRegionSection *section)
> > > +{
> > > +    Int128 llend = int128_make64(section->offset_within_address_space);
> > > +    llend = int128_add(llend, section->size);
> > > +    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> > > +
> > > +    return llend;
> > > +}
> > > +
> > >  static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
> > >  {
> > >      return (!memory_region_is_ram(section->mr) &&
> > > @@ -160,10 +169,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> > >      }
> > >
> > >      iova = TARGET_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(TARGET_PAGE_MASK));
> > > -
> > > +    llend = vhost_vdpa_section_end(section);
> > >      if (int128_ge(int128_make64(iova), llend)) {
> > >          return;
> > >      }
> > > @@ -221,9 +227,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
> > >      }
> > >
> > >      iova = TARGET_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(TARGET_PAGE_MASK));
> > > +    llend = vhost_vdpa_section_end(section);
> > >
> > >      trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
> > >
> > > --
> > > 2.27.0
> >

On Tue, Oct 5, 2021 at 12:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 05, 2021 at 11:52:37AM +0200, Eugenio Perez Martin wrote:
> > On Tue, Oct 5, 2021 at 10:15 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 05, 2021 at 10:01:30AM +0200, Eugenio Pérez wrote:
> > > > Abstract this operation, that will be reused when validating the region
> > > > against the iova range that the device supports.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > > Note that as defined end is actually 1 byte beyond end of section.
> > > As such it can e.g. overflow if cast to u64.
> > > So be careful to use int128 ops with it.
> >
> > You are right, but this is only the result of extracting "llend"
> > calculation in its own function, since it is going to be used a third
> > time in the next commit. This next commit contains a mistake because
> > of this, as you pointed out.
> >
> > Since "last" would be a very misleading name, do you think we could
> > give a better name / type to it?
> >
> > > Also - document?
> >
> > It will be documented with that ("It returns one byte beyond end of
> > section" or similar) too.
> >
> > Thanks!
>
> that's how c++ containers work so maybe it's not too bad as long
> as we document this carefully.
>

I tend to see it that way except when the name is "last", that I read
as "last one addressable/valid", as discussed in the
VHOST_VDPA_GET_IOVA_RANGE call mail thread. So end = past range, last
= last one valid.

It would be great to have something like void * / hwaddr, or c++
chrono time_point<second> vs time_point<millisecond>, that moves to
type system the verification of not mixing different range types. But
this may be overthinking at this moment.

Thanks!

> > >
> > > > ---
> > > >  hw/virtio/vhost-vdpa.c | 18 +++++++++++-------
> > > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index ea1aa71ad8..a1de6c7c9c 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -24,6 +24,15 @@
> > > >  #include "trace.h"
> > > >  #include "qemu-common.h"
> > > >
> > > > +static Int128 vhost_vdpa_section_end(const MemoryRegionSection *section)
> > > > +{
> > > > +    Int128 llend = int128_make64(section->offset_within_address_space);
> > > > +    llend = int128_add(llend, section->size);
> > > > +    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
> > > > +
> > > > +    return llend;
> > > > +}
> > > > +
> > > >  static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
> > > >  {
> > > >      return (!memory_region_is_ram(section->mr) &&
> > > > @@ -160,10 +169,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
> > > >      }
> > > >
> > > >      iova = TARGET_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(TARGET_PAGE_MASK));
> > > > -
> > > > +    llend = vhost_vdpa_section_end(section);
> > > >      if (int128_ge(int128_make64(iova), llend)) {
> > > >          return;
> > > >      }
> > > > @@ -221,9 +227,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
> > > >      }
> > > >
> > > >      iova = TARGET_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(TARGET_PAGE_MASK));
> > > > +    llend = vhost_vdpa_section_end(section);
> > > >
> > > >      trace_vhost_vdpa_listener_region_del(v, iova, int128_get64(llend));
> > > >
> > > > --
> > > > 2.27.0
> > >
>