Let's factor it out, reducing quite some code duplication and perparing
for further changes.
If we fail to mmap a region and panic, we now simply don't add that
(broken) region.
Note that we now increment dev->nregions as we are successfully
adding memory regions, and don't increment dev->nregions if anything went
wrong.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
subprojects/libvhost-user/libvhost-user.c | 168 ++++++++--------------
1 file changed, 60 insertions(+), 108 deletions(-)
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index d9e2214ad2..a2baefe84b 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -256,6 +256,61 @@ vu_remove_all_mem_regs(VuDev *dev)
dev->nregions = 0;
}
+static void
+_vu_add_mem_reg(VuDev *dev, VhostUserMemoryRegion *msg_region, int fd)
+{
+ int prot = PROT_READ | PROT_WRITE;
+ VuDevRegion *r;
+ void *mmap_addr;
+
+ DPRINT("Adding region %d\n", dev->nregions);
+ DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
+ msg_region->guest_phys_addr);
+ DPRINT(" memory_size: 0x%016"PRIx64"\n",
+ msg_region->memory_size);
+ DPRINT(" userspace_addr 0x%016"PRIx64"\n",
+ msg_region->userspace_addr);
+ DPRINT(" mmap_offset 0x%016"PRIx64"\n",
+ msg_region->mmap_offset);
+
+ if (dev->postcopy_listening) {
+ /*
+ * In postcopy we're using PROT_NONE here to catch anyone
+ * accessing it before we userfault
+ */
+ prot = PROT_NONE;
+ }
+
+ /*
+ * We don't use offset argument of mmap() since the mapped address has
+ * to be page aligned, and we use huge pages.
+ */
+ mmap_addr = mmap(0, msg_region->memory_size + msg_region->mmap_offset,
+ prot, MAP_SHARED | MAP_NORESERVE, fd, 0);
+ if (mmap_addr == MAP_FAILED) {
+ vu_panic(dev, "region mmap error: %s", strerror(errno));
+ return;
+ }
+ DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
+ (uint64_t)(uintptr_t)mmap_addr);
+
+ r = &dev->regions[dev->nregions];
+ r->gpa = msg_region->guest_phys_addr;
+ r->size = msg_region->memory_size;
+ r->qva = msg_region->userspace_addr;
+ r->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
+ r->mmap_offset = msg_region->mmap_offset;
+ dev->nregions++;
+
+ if (dev->postcopy_listening) {
+ /*
+ * Return the address to QEMU so that it can translate the ufd
+ * fault addresses back.
+ */
+ msg_region->userspace_addr = r->mmap_addr + r->mmap_offset;
+ }
+}
+
static void
vmsg_close_fds(VhostUserMsg *vmsg)
{
@@ -727,10 +782,7 @@ generate_faults(VuDev *dev) {
static bool
vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
int i;
- bool track_ramblocks = dev->postcopy_listening;
VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m;
- VuDevRegion *dev_region = &dev->regions[dev->nregions];
- void *mmap_addr;
if (vmsg->fd_num != 1) {
vmsg_close_fds(vmsg);
@@ -760,69 +812,20 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
* we know all the postcopy client bases have been received, and we
* should start generating faults.
*/
- if (track_ramblocks &&
+ if (dev->postcopy_listening &&
vmsg->size == sizeof(vmsg->payload.u64) &&
vmsg->payload.u64 == 0) {
(void)generate_faults(dev);
return false;
}
- DPRINT("Adding region: %u\n", dev->nregions);
- DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
- msg_region->guest_phys_addr);
- DPRINT(" memory_size: 0x%016"PRIx64"\n",
- msg_region->memory_size);
- DPRINT(" userspace_addr 0x%016"PRIx64"\n",
- msg_region->userspace_addr);
- DPRINT(" mmap_offset 0x%016"PRIx64"\n",
- msg_region->mmap_offset);
-
- dev_region->gpa = msg_region->guest_phys_addr;
- dev_region->size = msg_region->memory_size;
- dev_region->qva = msg_region->userspace_addr;
- dev_region->mmap_offset = msg_region->mmap_offset;
-
- /*
- * We don't use offset argument of mmap() since the
- * mapped address has to be page aligned, and we use huge
- * pages.
- */
- if (track_ramblocks) {
- /*
- * In postcopy we're using PROT_NONE here to catch anyone
- * accessing it before we userfault.
- */
- mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
- PROT_NONE, MAP_SHARED | MAP_NORESERVE,
- vmsg->fds[0], 0);
- } else {
- mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
- PROT_READ | PROT_WRITE, MAP_SHARED | MAP_NORESERVE,
- vmsg->fds[0], 0);
- }
-
- if (mmap_addr == MAP_FAILED) {
- vu_panic(dev, "region mmap error: %s", strerror(errno));
- } else {
- dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
- DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
- dev_region->mmap_addr);
- }
-
+ _vu_add_mem_reg(dev, msg_region, vmsg->fds[0]);
close(vmsg->fds[0]);
- if (track_ramblocks) {
- /*
- * Return the address to QEMU so that it can translate the ufd
- * fault addresses back.
- */
- msg_region->userspace_addr = dev_region->mmap_addr +
- dev_region->mmap_offset;
-
+ if (dev->postcopy_listening) {
/* Send the message back to qemu with the addresses filled in. */
vmsg->fd_num = 0;
DPRINT("Successfully added new region in postcopy\n");
- dev->nregions++;
return true;
} else {
for (i = 0; i < dev->max_queues; i++) {
@@ -835,7 +838,6 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
}
DPRINT("Successfully added new region\n");
- dev->nregions++;
return false;
}
}
@@ -940,63 +942,13 @@ static bool
vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
{
VhostUserMemory m = vmsg->payload.memory, *memory = &m;
- int prot = PROT_READ | PROT_WRITE;
unsigned int i;
- if (dev->postcopy_listening) {
- /*
- * In postcopy we're using PROT_NONE here to catch anyone
- * accessing it before we userfault
- */
- prot = PROT_NONE;
- }
-
vu_remove_all_mem_regs(dev);
- dev->nregions = memory->nregions;
DPRINT("Nregions: %u\n", memory->nregions);
- for (i = 0; i < dev->nregions; i++) {
- void *mmap_addr;
- VhostUserMemoryRegion *msg_region = &memory->regions[i];
- VuDevRegion *dev_region = &dev->regions[i];
-
- DPRINT("Region %d\n", i);
- DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
- msg_region->guest_phys_addr);
- DPRINT(" memory_size: 0x%016"PRIx64"\n",
- msg_region->memory_size);
- DPRINT(" userspace_addr 0x%016"PRIx64"\n",
- msg_region->userspace_addr);
- DPRINT(" mmap_offset 0x%016"PRIx64"\n",
- msg_region->mmap_offset);
-
- dev_region->gpa = msg_region->guest_phys_addr;
- dev_region->size = msg_region->memory_size;
- dev_region->qva = msg_region->userspace_addr;
- dev_region->mmap_offset = msg_region->mmap_offset;
-
- /* We don't use offset argument of mmap() since the
- * mapped address has to be page aligned, and we use huge
- * pages. */
- mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
- prot, MAP_SHARED | MAP_NORESERVE, vmsg->fds[i], 0);
-
- if (mmap_addr == MAP_FAILED) {
- vu_panic(dev, "region mmap error: %s", strerror(errno));
- } else {
- dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
- DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
- dev_region->mmap_addr);
- }
-
- if (dev->postcopy_listening) {
- /*
- * Return the address to QEMU so that it can translate the ufd
- * fault addresses back.
- */
- msg_region->userspace_addr = dev_region->mmap_addr +
- dev_region->mmap_offset;
- }
+ for (i = 0; i < memory->nregions; i++) {
+ _vu_add_mem_reg(dev, &memory->regions[i], vmsg->fds[i]);
close(vmsg->fds[i]);
}
--
2.43.0On Fri, Feb 2, 2024 at 4:55 PM David Hildenbrand <david@redhat.com> wrote:
>
> Let's factor it out, reducing quite some code duplication and perparing
> for further changes.
>
> If we fail to mmap a region and panic, we now simply don't add that
> (broken) region.
>
> Note that we now increment dev->nregions as we are successfully
> adding memory regions, and don't increment dev->nregions if anything went
> wrong.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Raphael Norwitz <raphael@enfabrica.net>
> ---
> subprojects/libvhost-user/libvhost-user.c | 168 ++++++++--------------
> 1 file changed, 60 insertions(+), 108 deletions(-)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index d9e2214ad2..a2baefe84b 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -256,6 +256,61 @@ vu_remove_all_mem_regs(VuDev *dev)
> dev->nregions = 0;
> }
>
> +static void
> +_vu_add_mem_reg(VuDev *dev, VhostUserMemoryRegion *msg_region, int fd)
> +{
> + int prot = PROT_READ | PROT_WRITE;
> + VuDevRegion *r;
> + void *mmap_addr;
> +
> + DPRINT("Adding region %d\n", dev->nregions);
> + DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
> + msg_region->guest_phys_addr);
> + DPRINT(" memory_size: 0x%016"PRIx64"\n",
> + msg_region->memory_size);
> + DPRINT(" userspace_addr 0x%016"PRIx64"\n",
> + msg_region->userspace_addr);
> + DPRINT(" mmap_offset 0x%016"PRIx64"\n",
> + msg_region->mmap_offset);
> +
> + if (dev->postcopy_listening) {
> + /*
> + * In postcopy we're using PROT_NONE here to catch anyone
> + * accessing it before we userfault
> + */
> + prot = PROT_NONE;
> + }
> +
> + /*
> + * We don't use offset argument of mmap() since the mapped address has
> + * to be page aligned, and we use huge pages.
> + */
> + mmap_addr = mmap(0, msg_region->memory_size + msg_region->mmap_offset,
> + prot, MAP_SHARED | MAP_NORESERVE, fd, 0);
> + if (mmap_addr == MAP_FAILED) {
> + vu_panic(dev, "region mmap error: %s", strerror(errno));
> + return;
> + }
> + DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
> + (uint64_t)(uintptr_t)mmap_addr);
> +
> + r = &dev->regions[dev->nregions];
> + r->gpa = msg_region->guest_phys_addr;
> + r->size = msg_region->memory_size;
> + r->qva = msg_region->userspace_addr;
> + r->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
> + r->mmap_offset = msg_region->mmap_offset;
> + dev->nregions++;
> +
> + if (dev->postcopy_listening) {
> + /*
> + * Return the address to QEMU so that it can translate the ufd
> + * fault addresses back.
> + */
> + msg_region->userspace_addr = r->mmap_addr + r->mmap_offset;
> + }
> +}
> +
> static void
> vmsg_close_fds(VhostUserMsg *vmsg)
> {
> @@ -727,10 +782,7 @@ generate_faults(VuDev *dev) {
> static bool
> vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
> int i;
> - bool track_ramblocks = dev->postcopy_listening;
> VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m;
> - VuDevRegion *dev_region = &dev->regions[dev->nregions];
> - void *mmap_addr;
>
> if (vmsg->fd_num != 1) {
> vmsg_close_fds(vmsg);
> @@ -760,69 +812,20 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
> * we know all the postcopy client bases have been received, and we
> * should start generating faults.
> */
> - if (track_ramblocks &&
> + if (dev->postcopy_listening &&
> vmsg->size == sizeof(vmsg->payload.u64) &&
> vmsg->payload.u64 == 0) {
> (void)generate_faults(dev);
> return false;
> }
>
> - DPRINT("Adding region: %u\n", dev->nregions);
> - DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
> - msg_region->guest_phys_addr);
> - DPRINT(" memory_size: 0x%016"PRIx64"\n",
> - msg_region->memory_size);
> - DPRINT(" userspace_addr 0x%016"PRIx64"\n",
> - msg_region->userspace_addr);
> - DPRINT(" mmap_offset 0x%016"PRIx64"\n",
> - msg_region->mmap_offset);
> -
> - dev_region->gpa = msg_region->guest_phys_addr;
> - dev_region->size = msg_region->memory_size;
> - dev_region->qva = msg_region->userspace_addr;
> - dev_region->mmap_offset = msg_region->mmap_offset;
> -
> - /*
> - * We don't use offset argument of mmap() since the
> - * mapped address has to be page aligned, and we use huge
> - * pages.
> - */
> - if (track_ramblocks) {
> - /*
> - * In postcopy we're using PROT_NONE here to catch anyone
> - * accessing it before we userfault.
> - */
> - mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
> - PROT_NONE, MAP_SHARED | MAP_NORESERVE,
> - vmsg->fds[0], 0);
> - } else {
> - mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
> - PROT_READ | PROT_WRITE, MAP_SHARED | MAP_NORESERVE,
> - vmsg->fds[0], 0);
> - }
> -
> - if (mmap_addr == MAP_FAILED) {
> - vu_panic(dev, "region mmap error: %s", strerror(errno));
> - } else {
> - dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
> - DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
> - dev_region->mmap_addr);
> - }
> -
> + _vu_add_mem_reg(dev, msg_region, vmsg->fds[0]);
> close(vmsg->fds[0]);
>
> - if (track_ramblocks) {
> - /*
> - * Return the address to QEMU so that it can translate the ufd
> - * fault addresses back.
> - */
> - msg_region->userspace_addr = dev_region->mmap_addr +
> - dev_region->mmap_offset;
> -
> + if (dev->postcopy_listening) {
> /* Send the message back to qemu with the addresses filled in. */
> vmsg->fd_num = 0;
> DPRINT("Successfully added new region in postcopy\n");
> - dev->nregions++;
> return true;
> } else {
> for (i = 0; i < dev->max_queues; i++) {
> @@ -835,7 +838,6 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
> }
>
> DPRINT("Successfully added new region\n");
> - dev->nregions++;
> return false;
> }
> }
> @@ -940,63 +942,13 @@ static bool
> vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> {
> VhostUserMemory m = vmsg->payload.memory, *memory = &m;
> - int prot = PROT_READ | PROT_WRITE;
> unsigned int i;
>
> - if (dev->postcopy_listening) {
> - /*
> - * In postcopy we're using PROT_NONE here to catch anyone
> - * accessing it before we userfault
> - */
> - prot = PROT_NONE;
> - }
> -
> vu_remove_all_mem_regs(dev);
> - dev->nregions = memory->nregions;
>
> DPRINT("Nregions: %u\n", memory->nregions);
> - for (i = 0; i < dev->nregions; i++) {
> - void *mmap_addr;
> - VhostUserMemoryRegion *msg_region = &memory->regions[i];
> - VuDevRegion *dev_region = &dev->regions[i];
> -
> - DPRINT("Region %d\n", i);
> - DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
> - msg_region->guest_phys_addr);
> - DPRINT(" memory_size: 0x%016"PRIx64"\n",
> - msg_region->memory_size);
> - DPRINT(" userspace_addr 0x%016"PRIx64"\n",
> - msg_region->userspace_addr);
> - DPRINT(" mmap_offset 0x%016"PRIx64"\n",
> - msg_region->mmap_offset);
> -
> - dev_region->gpa = msg_region->guest_phys_addr;
> - dev_region->size = msg_region->memory_size;
> - dev_region->qva = msg_region->userspace_addr;
> - dev_region->mmap_offset = msg_region->mmap_offset;
> -
> - /* We don't use offset argument of mmap() since the
> - * mapped address has to be page aligned, and we use huge
> - * pages. */
> - mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
> - prot, MAP_SHARED | MAP_NORESERVE, vmsg->fds[i], 0);
> -
> - if (mmap_addr == MAP_FAILED) {
> - vu_panic(dev, "region mmap error: %s", strerror(errno));
> - } else {
> - dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
> - DPRINT(" mmap_addr: 0x%016"PRIx64"\n",
> - dev_region->mmap_addr);
> - }
> -
> - if (dev->postcopy_listening) {
> - /*
> - * Return the address to QEMU so that it can translate the ufd
> - * fault addresses back.
> - */
> - msg_region->userspace_addr = dev_region->mmap_addr +
> - dev_region->mmap_offset;
> - }
> + for (i = 0; i < memory->nregions; i++) {
> + _vu_add_mem_reg(dev, &memory->regions[i], vmsg->fds[i]);
> close(vmsg->fds[i]);
> }
>
> --
> 2.43.0
>
>
© 2016 - 2024 Red Hat, Inc.