On Tue, Oct 5, 2021 at 12:47 PM Michael S. Tsirkin <m...@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 <m...@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 <epere...@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
> > >
>
