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

[Eugenio Perez Martin]

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
>