diff mbox series

[v1,10/15] libvhost-user: Factor out search for memory region by GPA and simplify

Message ID 20240202215332.118728-11-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series libvhost-user: support more memslots and cleanup memslot handling code | expand

Commit Message

David Hildenbrand Feb. 2, 2024, 9:53 p.m. UTC
Memory regions cannot overlap, and if we ever hit that case something
would be really flawed.

For example, when vhost code in QEMU decides to increase the size of memory
regions to cover full huge pages, it makes sure to never create overlaps,
and if there would be overlaps, it would bail out.

QEMU commits 48d7c9757749 ("vhost: Merge sections added to temporary
list"), c1ece84e7c93 ("vhost: Huge page align and merge") and
e7b94a84b6cb ("vhost: Allow adjoining regions") added and clarified that
handling and how overlaps are impossible.

Consequently, each GPA can belong to at most one memory region, and
everything else doesn't make sense. Let's factor out our search to prepare
for further changes.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 79 +++++++++++++----------
 1 file changed, 45 insertions(+), 34 deletions(-)

Comments

Raphael Norwitz Feb. 4, 2024, 1:47 a.m. UTC | #1
On Fri, Feb 2, 2024 at 4:54 PM David Hildenbrand <david@redhat.com> wrote:
>
> Memory regions cannot overlap, and if we ever hit that case something
> would be really flawed.
>
> For example, when vhost code in QEMU decides to increase the size of memory
> regions to cover full huge pages, it makes sure to never create overlaps,
> and if there would be overlaps, it would bail out.
>
> QEMU commits 48d7c9757749 ("vhost: Merge sections added to temporary
> list"), c1ece84e7c93 ("vhost: Huge page align and merge") and
> e7b94a84b6cb ("vhost: Allow adjoining regions") added and clarified that
> handling and how overlaps are impossible.
>
> Consequently, each GPA can belong to at most one memory region, and
> everything else doesn't make sense. Let's factor out our search to prepare
> for further changes.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Raphael Norwitz <raphael@enfabrica.net>

> ---
>  subprojects/libvhost-user/libvhost-user.c | 79 +++++++++++++----------
>  1 file changed, 45 insertions(+), 34 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 22154b217f..d036b54ed0 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -195,30 +195,47 @@ vu_panic(VuDev *dev, const char *msg, ...)
>       */
>  }
>
> +/* Search for a memory region that covers this guest physical address. */
> +static VuDevRegion *
> +vu_gpa_to_mem_region(VuDev *dev, uint64_t guest_addr)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * Memory regions cannot overlap in guest physical address space. Each
> +     * GPA belongs to exactly one memory region, so there can only be one
> +     * match.
> +     */
> +    for (i = 0; i < dev->nregions; i++) {
> +        VuDevRegion *cur = &dev->regions[i];
> +
> +        if (guest_addr >= cur->gpa && guest_addr < cur->gpa + cur->size) {
> +            return cur;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  /* Translate guest physical address to our virtual address.  */
>  void *
>  vu_gpa_to_va(VuDev *dev, uint64_t *plen, uint64_t guest_addr)
>  {
> -    unsigned int i;
> +    VuDevRegion *r;
>
>      if (*plen == 0) {
>          return NULL;
>      }
>
> -    /* Find matching memory region.  */
> -    for (i = 0; i < dev->nregions; i++) {
> -        VuDevRegion *r = &dev->regions[i];
> -
> -        if ((guest_addr >= r->gpa) && (guest_addr < (r->gpa + r->size))) {
> -            if ((guest_addr + *plen) > (r->gpa + r->size)) {
> -                *plen = r->gpa + r->size - guest_addr;
> -            }
> -            return (void *)(uintptr_t)
> -                guest_addr - r->gpa + r->mmap_addr + r->mmap_offset;
> -        }
> +    r = vu_gpa_to_mem_region(dev, guest_addr);
> +    if (!r) {
> +        return NULL;
>      }
>
> -    return NULL;
> +    if ((guest_addr + *plen) > (r->gpa + r->size)) {
> +        *plen = r->gpa + r->size - guest_addr;
> +    }
> +    return (void *)(uintptr_t)guest_addr - r->gpa + r->mmap_addr +
> +           r->mmap_offset;
>  }
>
>  /* Translate qemu virtual address to our virtual address.  */
> @@ -854,8 +871,8 @@ static inline bool reg_equal(VuDevRegion *vudev_reg,
>  static bool
>  vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>      VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m;
> -    unsigned int i;
> -    bool found = false;
> +    unsigned int idx;
> +    VuDevRegion *r;
>
>      if (vmsg->fd_num > 1) {
>          vmsg_close_fds(vmsg);
> @@ -882,28 +899,22 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>      DPRINT("    mmap_offset      0x%016"PRIx64"\n",
>             msg_region->mmap_offset);
>
> -    for (i = 0; i < dev->nregions; i++) {
> -        if (reg_equal(&dev->regions[i], msg_region)) {
> -            VuDevRegion *r = &dev->regions[i];
> -
> -            munmap((void *)(uintptr_t)r->mmap_addr, r->size + r->mmap_offset);
> -
> -            /* Shift all affected entries by 1 to close the hole at index. */
> -            memmove(dev->regions + i, dev->regions + i + 1,
> -                    sizeof(VuDevRegion) * (dev->nregions - i - 1));
> -            DPRINT("Successfully removed a region\n");
> -            dev->nregions--;
> -            i--;
> -
> -            found = true;
> -            break;
> -        }
> -    }
> -
> -    if (!found) {
> +    r = vu_gpa_to_mem_region(dev, msg_region->guest_phys_addr);
> +    if (!r || !reg_equal(r, msg_region)) {
> +        vmsg_close_fds(vmsg);
>          vu_panic(dev, "Specified region not found\n");
> +        return false;
>      }
>
> +    munmap((void *)(uintptr_t)r->mmap_addr, r->size + r->mmap_offset);
> +
> +    idx = r - dev->regions;
> +    assert(idx < dev->nregions);
> +    /* Shift all affected entries by 1 to close the hole. */
> +    memmove(r, r + 1, sizeof(VuDevRegion) * (dev->nregions - idx - 1));
> +    DPRINT("Successfully removed a region\n");
> +    dev->nregions--;
> +
>      vmsg_close_fds(vmsg);
>
>      return false;
> --
> 2.43.0
>
>
diff mbox series

Patch

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 22154b217f..d036b54ed0 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -195,30 +195,47 @@  vu_panic(VuDev *dev, const char *msg, ...)
      */
 }
 
+/* Search for a memory region that covers this guest physical address. */
+static VuDevRegion *
+vu_gpa_to_mem_region(VuDev *dev, uint64_t guest_addr)
+{
+    unsigned int i;
+
+    /*
+     * Memory regions cannot overlap in guest physical address space. Each
+     * GPA belongs to exactly one memory region, so there can only be one
+     * match.
+     */
+    for (i = 0; i < dev->nregions; i++) {
+        VuDevRegion *cur = &dev->regions[i];
+
+        if (guest_addr >= cur->gpa && guest_addr < cur->gpa + cur->size) {
+            return cur;
+        }
+    }
+    return NULL;
+}
+
 /* Translate guest physical address to our virtual address.  */
 void *
 vu_gpa_to_va(VuDev *dev, uint64_t *plen, uint64_t guest_addr)
 {
-    unsigned int i;
+    VuDevRegion *r;
 
     if (*plen == 0) {
         return NULL;
     }
 
-    /* Find matching memory region.  */
-    for (i = 0; i < dev->nregions; i++) {
-        VuDevRegion *r = &dev->regions[i];
-
-        if ((guest_addr >= r->gpa) && (guest_addr < (r->gpa + r->size))) {
-            if ((guest_addr + *plen) > (r->gpa + r->size)) {
-                *plen = r->gpa + r->size - guest_addr;
-            }
-            return (void *)(uintptr_t)
-                guest_addr - r->gpa + r->mmap_addr + r->mmap_offset;
-        }
+    r = vu_gpa_to_mem_region(dev, guest_addr);
+    if (!r) {
+        return NULL;
     }
 
-    return NULL;
+    if ((guest_addr + *plen) > (r->gpa + r->size)) {
+        *plen = r->gpa + r->size - guest_addr;
+    }
+    return (void *)(uintptr_t)guest_addr - r->gpa + r->mmap_addr +
+           r->mmap_offset;
 }
 
 /* Translate qemu virtual address to our virtual address.  */
@@ -854,8 +871,8 @@  static inline bool reg_equal(VuDevRegion *vudev_reg,
 static bool
 vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m;
-    unsigned int i;
-    bool found = false;
+    unsigned int idx;
+    VuDevRegion *r;
 
     if (vmsg->fd_num > 1) {
         vmsg_close_fds(vmsg);
@@ -882,28 +899,22 @@  vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     DPRINT("    mmap_offset      0x%016"PRIx64"\n",
            msg_region->mmap_offset);
 
-    for (i = 0; i < dev->nregions; i++) {
-        if (reg_equal(&dev->regions[i], msg_region)) {
-            VuDevRegion *r = &dev->regions[i];
-
-            munmap((void *)(uintptr_t)r->mmap_addr, r->size + r->mmap_offset);
-
-            /* Shift all affected entries by 1 to close the hole at index. */
-            memmove(dev->regions + i, dev->regions + i + 1,
-                    sizeof(VuDevRegion) * (dev->nregions - i - 1));
-            DPRINT("Successfully removed a region\n");
-            dev->nregions--;
-            i--;
-
-            found = true;
-            break;
-        }
-    }
-
-    if (!found) {
+    r = vu_gpa_to_mem_region(dev, msg_region->guest_phys_addr);
+    if (!r || !reg_equal(r, msg_region)) {
+        vmsg_close_fds(vmsg);
         vu_panic(dev, "Specified region not found\n");
+        return false;
     }
 
+    munmap((void *)(uintptr_t)r->mmap_addr, r->size + r->mmap_offset);
+
+    idx = r - dev->regions;
+    assert(idx < dev->nregions);
+    /* Shift all affected entries by 1 to close the hole. */
+    memmove(r, r + 1, sizeof(VuDevRegion) * (dev->nregions - idx - 1));
+    DPRINT("Successfully removed a region\n");
+    dev->nregions--;
+
     vmsg_close_fds(vmsg);
 
     return false;