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

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 [off-list ref] wrote:

quoted

On Tue, Oct 05, 2021 at 10:01:30AM +0200, Eugenio Pérez wrote:

quoted

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?

quoted

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.

quoted

quoted

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization