Re: [PATCH 2/3] vdpa: Add vhost_vdpa_section_end

["Michael S. Tsirkin" <mst@redhat.com>]

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
> >