Re: Adding a memory alias breaks v-rings

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Geoffrey,

On 10/24/19 10:27 AM, geoff@hostfission.com wrote:
> Hi All,
> 
> I have been working on adding a feature as a proof of concept to improve 
> the performance of applications like Looking Glass by avoiding 
> additional memory copies. My goal is to alias part of the IVSHMEM shared 
> memory over a pointer provided by the guest OS capture API (DXGI Desktop 
> Duplication or NVIDIA Frame Buffer Capture). I have managed to get this 
> working by adding a few additional configuration registers to the 
> IVSHMEM device and enhanced the IVSHMEM windows driver with suitable 
> IOCTLs to set this all up. While this concept is backwards it needs to 
> work this way as we do not have control over the destination buffer 
> allocation by the GPU driver.
> 
> This all works, however, it has exposed a bug (or I am doing things 
> improperly) with the way that vhost tracks memory. When calling 
> memory_region_add_subregion_overlap the memory listener in vhost fires 
> triggering vhost_region_add_section. According to the comments this code 
> depends on being called in memory address order, but because I am adding 
> the alias region late, it's out of order, and also splits the upper 
> memory region. This has the effect of corrupting/breaking one or more 
> random vrings, as evidenced by the crash/hang of vhost-net or other 
> virtio devices.

I'm not sure this is the same issue I had before, but you might
find Frederic and Alexey suggestions from this thread helpful:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg525833.html

Also note vhost_region_add_section() you mentioned has this comment:

     if (need_add) {
         ...
         /* The flatview isn't stable and we don't use it, making it NULL
          * means we can memcmp the list.
          */
         dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;

Maybe you need this change:

-- >8 --
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -642,6 +642,7 @@ static void vhost_region_add_section(struct 
vhost_dev *dev,
           */
          dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;
          memory_region_ref(section->mr);
+        memory_region_update_container_subregions(section->mr);
      }
  }

---

Regards,

Phil.

> The following and errors are also logged regarding 
> section alignment:
> 
> qemu-system-x86_64: vhost_region_add_section:Section rounded to 
> 3c0000000 prior to previous 3fc4f9000
> qemu-system-x86_64: vhost_region_add_section:Section rounded to 
> 3c0000000 prior to previous 3fc4f9000
> 
> Here is the flat view after the alias has been added.
> 
>    0000000100000000-00000003fc4f8fff (prio 0, ram): mem 
> @0000000080000000 kvm
>    00000003fc4f9000-00000003fc4f9fff (prio 1, ram): ivshmem kvm
>    00000003fc4fa000-000000043fffffff (prio 0, ram): mem 
> @000000037c4fa000 kvm
> 
> When the guest doesn't crash out due to the obvious corruption it is 
> possible to verify that the alias is in the right place and fully 
> functional. Unfortunately, I simply do not have enough of a grasp on 
> vhost to understand exactly what is going on and how to correct it.
> 
> Getting this feature working is highly desirable as it should be 
> possible to obtain GPU -> GPU memory transfers between guests without 
> requiring workstation/professional graphics cards.
> 
> Kind Regards,
> Geoffrey McRae
>