Re: Adding a memory alias breaks v-rings

[geoff--- via <qemu-devel@nongnu.org>]

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