[virglrenderer-devel] Proposals for virtio-gpu and virglrenderer

[Stéphane Marchesin]

On Thu, Apr 26, 2018 at 6:05 PM, Dave Airlie <airlied at gmail.com> wrote:
>> I've been working on adding accelerated virtio-gpu support to crosvm as
>> well as adding support for sending dmabufs across virtio-wayland for
>> improved host-side compositor performance. Along the way, I've encountered
>> a few features that I would like to discuss to make things work better for
>> crosvm, and, I believe, other users of virtio-gpu/virglrenderer.
>
> Hey,
>
> I'm just back from a few weeks off so I might need help on these as I'm
> not fully back up to speed.
>>
>> 1) Add mmap support to virtio-gpu
>> 2) Add API to import buffers to virglrenderer
>> 3) Harden virglrenderer using memory safe language
>>
>> Elaboration for each feature below:
>>
>> 1) Currently, the only way to fill a resource in virtio-gpu is either via a
>> transfer from the backing memory to the resource, or to submit commands
>> that render to the resource. This is not optimal for software rendering or
>> texture upload situations because of the additional copy this transfer
>> necessitates. I propose that an additional virtio-gpu command, perhaps
>> gated by a virtio feature bit, be made that allows the guest to request
>> mapping of a resource directly into guest physical address space. With this
>> virtio command, requests to mmap a virgio-gpu resource by the guest's user
>> space applications, perhaps through a DRM ioctl on the buffer object handle
>> or an mmap on the dmabuf/prime FD, could be fulfilled without additional
>> copy operations to flush data that was written.
>
> How does this deal with tiling/unmappable resources? Most of the resources
> we allocate end up in tile VRAM hopefully, this may not even be mappable.
> When you use the GL APIs to map it we end up getting a transfer not a direct
> map so you get a copy there. If we bypass GL and go straight to exporting
> the buffer, we just get a tiled buffer which isn't that useful.
>
> Lets then assume you only want to map linear strided resources, you envisage
> exporting a precreated resource to a dma-buf, and somehow mmaping
> the pages behind that resource into the guest memory space.

For context, I am seeing that, when no format conversion needs to
happen, virgl texture upload spends about half of its time doing iovec
copies (the other half is usually glTex* calls). For this a direct
mapping mechanism would go a long way towards improving performance.
Such a mechanism could also be a good fit for Vulkan's coherent
memory. At least pushing this copy towards the bottom of the stack
would give us an opportunity to improve this on platforms that can do
it.

I am not sure what the best way forward is; maybe the simplest way is
to abstract platform-specific GPU memory constructs (dmabufs,
iosurfaces, ...) into a single entity.

Stéphane

>
> Now the rules for what you can map where in guest VM are a bit hazy for me,
> maybe someone can expend more on how we can do this, do we need a PCI
> BAR to have these things appear in or can they end up in random guest pages?
> Gerd?
>
>> 2) Currently, the virglrenderer library allocates host side GPU memory for
>> backing resources using standard OpenGL texture creation functions. While
>> virglrenderer does support exporting resources, this does not work very
>> well on Chrome OS. More details about that can be found at
>> http://crbug.com/779356 . The root of the issue seems to be that
>> EGL_MESA_image_dma_buf_export, the extension used by virglrenderer to
>> export resources, does not function properly. The solution that was agreed
>> upon was to use gbm (minigbm in the case of Chrome OS), to allocate all
>> buffers that are destined to be exported because this is the well tested
>> path. However, virglrenderer has no API for importing any kind of buffer,
>> so one would need to be added. I suggest adding an interface that could
>> take advantage of the EGL_EXT_image_dma_buf_import extension for creating
>> an EGLImageKHR from an FD and associated metadata (width, height, format,
>> stride, modifiers, etc) and encapsulating that as an ordinary resource.
>> Another possible alternative would be to accept an already created
>> EGLImageKHR for maximum flexibility on the part of virglrenderer.
>
> Yeah I can't see much problem with fixing up an importing interface,
> Again as long as you aren't mapping it, it seems like it should be fine.
> I sorta understand why you don't want to fix the API as it wouldn't be
> consistent
> across drivers.
>
>>
>> 3) In crosvm, there are two main approaches we use to ensure safety of the
>> hypervisor: each device is sandboxed in its own process with minimal
>> privilege, and the code is entirely written in Rust, libc and other minor
>> exceptions notwithstanding. Idiomatic Rust makes common methods of
>> exploitation much less common, and the sandboxing is an additional layer of
>> defense in case a device process gets owned. The concept of using
>> virglrenderer, and then mesa underneath that, breaks both approaches to
>> safety: both libraries are large C code bases that could have an elevated
>> rate of exploitable weaknesses, and they require access to DRM render nodes
>> which I consider to be relatively vulnerable kernel interfaces. Put another
>> way, virglrenderer/mesa provide a potential foothold for a malicious guest
>> to gain a foothold into the virtio-gpu device, and the render node is an
>> attractive escape route out of the sandbox for the malicious code. Because
>> hardening the kernel interfaces seems much harder, it is desirable to
>> harden virglrenderer using a memory safe language like Rust. I'm not
>> advocating for a complete rewrite, as I think the areas that need the most
>> focus are in decoding command submissions from the guest. Rewriting
>> vrend_decode.c in Rust and linking that in with the rest of virglrenderer
>> would go a long way towards maintaining the safety of crosvm and would be
>> maintainable enough to consider having in upstream virglrenderer, in my
>> opinion.
>
> This is a bit of a stickler for me, I'm not sure the qemu folks would want to
> pull a rust build dependency into the tree, and I'm not sure it would help
> as much as having vrend_decode.c stay as small as possible and have
> someone with a good security mind review patches to it, or even have
> the fuzzer stuff run over things more often. I could be persuaded that
> maybe an optional rust dep maintained upsteram might be worth it, but
> you'd really have to show me that rust is going to do better here, if you can
> show me that badly written rust won't be as bad as badly written C I'lll
> listen, but if the argument is the rust needs to be well written, I'd rather
> we endevaour to produce well written C or some sort of code generatior
> that we trust to produce either C or RUST, but I'd rather not have to
> maintain two copies of the code, as it's a receipe for bad things.
>
> Dave.
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel