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

[Eugenio Perez Martin]

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