Adding a memory alias breaks v-rings

Adding a memory alias breaks v-rings

[geoff]

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

Re: Adding a memory alias breaks v-rings

[Philippe Mathieu-Daudé]

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
> 

Re: Adding a memory alias breaks v-rings

[geoff--- via]

Hi Phil

On 2019-10-24 22:07, Philippe Mathieu-Daudé wrote:
> 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);
>      }
>  }
> 
> ---

Unfortunately not, `memory_region_update_container_subregions` is 
private in memory.c hangs the VM even if I expose it and call it as you 
suggested. It is also already called via 
memory_region_add_subregion_overlap anyway.

Thanks for the suggestion though :)

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