| Message ID | 20210519001414.786439-4-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-gpu: Add support for Blob resources feature (v5) | expand |
> +#ifdef CONFIG_LINUX > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > +#else > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > +{ > + /* nothing (stub) */ > + return NULL > +} Fails to build for !linux ... You can place the stubs in a file in the stubs/ directory instead. They'll be used via weak symbol references instead of #ifdefs then. Advantage: the stubs are compiled unconditionally so errors like this don't go unnoticed that easily. take care, Gerd
Hi Gerd, > > +#ifdef CONFIG_LINUX > > > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > > +#else > > > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > +{ > > + /* nothing (stub) */ > > + return NULL > > +} > > Fails to build for !linux ... > > You can place the stubs in a file in the stubs/ directory instead. > They'll be used via weak symbol references instead of #ifdefs then. [Kasireddy, Vivek] Will do; should I send the whole series (v6) again with this and the other error in patch #10 fixed or just the fixed versions of these specific patches? Sorry for the tangential discussion... On a completely different topic, I wanted to ask a question on behalf of a colleague who is trying to enable passthrough with virtio-gpu but for a Windows guest. It appears the guest boots only if we specify the option -vga virtio (not sure what happens with virtio=std) as Windows launches a "Microsoft Basic Display Adapter" driver for this VGA device and Qemu displays the Desktop for this device (via the calls in virtio-vga.c). However, since we only care about virtio-gpu-pci device for which we created a guest driver, we want to have Qemu display the content/fb from virtio-gpu instead of the vga device. I see that in set_scanout: g->parent_obj.enable = 1; and, then this in virtio-vga.c: static void virtio_vga_base_update_display(void *opaque) VirtIOVGABase *vvga = opaque; VirtIOGPUBase *g = vvga->vgpu; if (g->enable) { g->hw_ops->gfx_update(g); } else { vvga->vga.hw_ops->gfx_update(&vvga->vga); } Since the parent_obj is different the above code is always going into the else part. Is the goal here to show the content from virtio-gpu if there is a set_scanout? The only way we are able to get everything to work as expected is to enable our virtio-gpu guest driver for the VGA device instead of the virtio-gpu PCI device. But we are not sure if this would be OK or not. We don't run into these issues for Linux guests as we only enable virtio-gpu-pci as we have -vga none. We'd like to the do the same for Windows guests but it looks like it needs the VGA device to be available to boot. So, our other option (as we cannot disable the vga device) is to have Qemu accept content only from virtio-gpu-pci instead of virtio-vga. Would it make sense to do this? Do you think there is a better way to do what we are trying to do? Thanks, Vivek
On Thu, May 20, 2021 at 06:23:58AM +0000, Kasireddy, Vivek wrote: > Hi Gerd, > > > > +#ifdef CONFIG_LINUX > > > > > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > > > > +#else > > > > > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > > +{ > > > + /* nothing (stub) */ > > > + return NULL > > > +} > > > > Fails to build for !linux ... > > > > You can place the stubs in a file in the stubs/ directory instead. > > They'll be used via weak symbol references instead of #ifdefs then. > [Kasireddy, Vivek] Will do; should I send the whole series (v6) again with this and the > other error in patch #10 fixed or just the fixed versions of these specific patches? New series please. > I see that in set_scanout: > > g->parent_obj.enable = 1; Yep. When the guest configured a scanout for the first time enable is set. device reset clears it btw. And set_scanout_blob should take care to set enable too. > VirtIOGPUBase *g = vvga->vgpu; > > if (g->enable) { > g->hw_ops->gfx_update(g); > } else { > vvga->vga.hw_ops->gfx_update(&vvga->vga); > } > > Since the parent_obj is different the above code is always going into the else part. No. VirtIOGPU->parent_obj actually *is* VirtIOGPUBase. > Is the goal here to show the content from virtio-gpu if there is a set_scanout? Yes. The idea is to switch into virtio-gpu mode when the guest driver did load and successfully initialized the scanout. Go back into vga mode when the device get reset due to reboot (or other reasons). > The only way we are able to get everything to work as expected is to enable our virtio-gpu > guest driver for the VGA device instead of the virtio-gpu PCI device. But we are not sure > if this would be OK or not. We don't run into these issues for Linux guests as we only > enable virtio-gpu-pci as we have -vga none. I'd suggest to run qemu with "-vga none" or "-nodefaults" so qemu will not automatically add a display device. Then explicitly add the device you want. "-device virtio-gpu-pci" is the pure virtio-gpu. "-device virtio-vga" is the virtio-gpu device with vga compatibility. Both should work just fine with both linux and windows. The only difference is that you'll get boot messages with virtio-vga (thanks to vga compat mode) whereas virtio-gpu-pci doesn't display anything until the guest display driver is loaded. OVMF can handle both virtio-gpu-pci and virtio-vga so you should see the grub boot menu with both devices. A GOP (used by efifb) is only available with virtio-vga though. Not sure how windows behaves here, probably it wants a GOP too for the early boot screen. The standard vga is "-device VGA", cirrus is "-device cirrus-vga". HTH, Gerd
Hi Gerd, > > > +#ifdef CONFIG_LINUX > > > +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > > +#else > > > +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) > > +{ > > + /* nothing (stub) */ > > + return NULL > > +} > > Fails to build for !linux ... > > You can place the stubs in a file in the stubs/ directory instead. > They'll be used via weak symbol references instead of #ifdefs then. > > Advantage: the stubs are compiled unconditionally so errors like this > don't go unnoticed that easily. [Kasireddy, Vivek] Gave it a try but because of res->image, we'd need to consider the Pixman dependency. I think we have the following options to address this: 1) Add pixman dependency to stubs. This may not be acceptable given that the other dependencies are glib, socket, etc which are pretty basic. 2) Cast the objects (such as virtio_gpu_simple_resource) as void * to the functions that we have stubs for. This also may not be acceptable as I don't see other stubs doing this. 3) Move some core objects (such as VirtIOGPUBase and its dependencies that do not deal with pixman) into a new header and include that in virtio-gpu.h and the new stubs file. This seems like the way to go but needs some work as virtio-gpu.h has a lot of stuff and is included in a lot of places. If it is not a problem, I can do this in a small separate series but keep the ifdef for this series? Will send out a v6 for this series soon. Thanks, Vivek > > take care, > Gerd
> [Kasireddy, Vivek] Gave it a try but because of res->image, we'd need to consider the > Pixman dependency. I think we have the following options to address this: > 1) Add pixman dependency to stubs. This may not be acceptable given that the other > dependencies are glib, socket, etc which are pretty basic. res->image is a pointer not an embedded struct so this could be handled without requiring pixman. Beside that pixman is a core dependency for system emulation, so a pixman dependency in stubs should not cause any trouble either. take care, Gerd
diff --git a/hw/display/meson.build b/hw/display/meson.build index aaf797c5e9..224b33f2bb 100644 --- a/hw/display/meson.build +++ b/hw/display/meson.build @@ -55,7 +55,7 @@ softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 'ati_2d if config_all_devices.has_key('CONFIG_VIRTIO_GPU') virtio_gpu_ss = ss.source_set() virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU', - if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c'), pixman]) + if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c', 'virtio-gpu-udmabuf.c'), pixman]) virtio_gpu_ss.add(when: 'CONFIG_VHOST_USER_GPU', if_true: files('vhost-user-gpu.c')) hw_display_modules += {'virtio-gpu': virtio_gpu_ss} diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c new file mode 100644 index 0000000000..3f569ae9c8 --- /dev/null +++ b/hw/display/virtio-gpu-udmabuf.c @@ -0,0 +1,181 @@ +/* + * Virtio GPU Device + * + * Copyright Red Hat, Inc. 2013-2014 + * + * Authors: + * Dave Airlie <airlied@redhat.com> + * Gerd Hoffmann <kraxel@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qemu/units.h" +#include "qemu-common.h" +#include "qemu/iov.h" +#include "ui/console.h" +#include "hw/virtio/virtio-gpu.h" +#include "hw/virtio/virtio-gpu-pixman.h" +#include "trace.h" + +#ifdef CONFIG_LINUX + +#include "exec/ramblock.h" +#include "sysemu/hostmem.h" +#include <sys/fcntl.h> +#include <sys/ioctl.h> +#include <linux/memfd.h> +#include "standard-headers/linux/udmabuf.h" + +static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res) +{ + struct udmabuf_create_list *list; + RAMBlock *rb; + ram_addr_t offset; + int udmabuf, i; + + udmabuf = udmabuf_fd(); + if (udmabuf < 0) { + return; + } + + list = g_malloc0(sizeof(struct udmabuf_create_list) + + sizeof(struct udmabuf_create_item) * res->iov_cnt); + + for (i = 0; i < res->iov_cnt; i++) { + rcu_read_lock(); + rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset); + rcu_read_unlock(); + + if (!rb || rb->fd < 0) { + g_free(list); + return; + } + + list->list[i].memfd = rb->fd; + list->list[i].offset = offset; + list->list[i].size = res->iov[i].iov_len; + } + + list->count = res->iov_cnt; + list->flags = UDMABUF_FLAGS_CLOEXEC; + + res->dmabuf_fd = ioctl(udmabuf, UDMABUF_CREATE_LIST, list); + if (res->dmabuf_fd < 0) { + warn_report("%s: UDMABUF_CREATE_LIST: %s", __func__, + strerror(errno)); + } + g_free(list); +} + +static void virtio_gpu_remap_udmabuf(struct virtio_gpu_simple_resource *res) +{ + res->remapped = mmap(NULL, res->blob_size, PROT_READ, + MAP_SHARED, res->dmabuf_fd, 0); + if (res->remapped == MAP_FAILED) { + warn_report("%s: dmabuf mmap failed: %s", __func__, + strerror(errno)); + res->remapped = NULL; + } +} + +static void virtio_gpu_destroy_udmabuf(struct virtio_gpu_simple_resource *res) +{ + if (res->remapped) { + munmap(res->remapped, res->blob_size); + res->remapped = NULL; + } + if (res->dmabuf_fd >= 0) { + close(res->dmabuf_fd); + res->dmabuf_fd = -1; + } +} + +static int find_memory_backend_type(Object *obj, void *opaque) +{ + bool *memfd_backend = opaque; + int ret; + + if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) { + HostMemoryBackend *backend = MEMORY_BACKEND(obj); + RAMBlock *rb = backend->mr.ram_block; + + if (rb && rb->fd > 0) { + ret = fcntl(rb->fd, F_GET_SEALS); + if (ret > 0) { + *memfd_backend = true; + } + } + } + + return 0; +} + +bool virtio_gpu_have_udmabuf(void) +{ + Object *memdev_root; + int udmabuf; + bool memfd_backend = false; + + udmabuf = udmabuf_fd(); + if (udmabuf < 0) { + return false; + } + + memdev_root = object_resolve_path("/objects", NULL); + object_child_foreach(memdev_root, find_memory_backend_type, &memfd_backend); + + return memfd_backend; +} + +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) +{ + void *pdata = NULL; + + res->dmabuf_fd = -1; + if (res->iov_cnt == 1) { + pdata = res->iov[0].iov_base; + } else { + virtio_gpu_create_udmabuf(res); + if (res->dmabuf_fd < 0) { + return; + } + virtio_gpu_remap_udmabuf(res); + if (!res->remapped) { + return; + } + pdata = res->remapped; + } + + res->blob = pdata; +} + +void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res) +{ + if (res->remapped) { + virtio_gpu_destroy_udmabuf(res); + } +} + +#else + +bool virtio_gpu_have_udmabuf(void) +{ + /* nothing (stub) */ + return false; +} + +void *virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res) +{ + /* nothing (stub) */ + return NULL +} + +void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res) +{ + /* nothing (stub) */ +} + +#endif diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index 8ca2c55d9a..265b1c516c 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -50,6 +50,12 @@ struct virtio_gpu_simple_resource { uint32_t scanout_bitmask; pixman_image_t *image; uint64_t hostmem; + + uint64_t blob_size; + void *blob; + int dmabuf_fd; + uint8_t *remapped; + QTAILQ_ENTRY(virtio_gpu_simple_resource) next; }; @@ -238,6 +244,11 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g, struct virtio_gpu_scanout *s, uint32_t resource_id); +/* virtio-gpu-udmabuf.c */ +bool virtio_gpu_have_udmabuf(void); +void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res); +void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res); + /* virtio-gpu-3d.c */ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd);
Add helper functions to create a dmabuf for a resource and mmap it. Also, introduce the fields blob and blob_size so that these helpers can start to use them but the full picture will emerge only after adding create_blob API in patch 8 of this series. To be able to create a dmabuf using the udmabuf driver, Qemu needs to be lauched with the memfd memory backend like this: qemu-system-x86_64 -m 8192m -object memory-backend-memfd,id=mem1,size=8192M -machine memory-backend=mem1 Based-on-patch-by: Gerd Hoffmann <kraxel@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- hw/display/meson.build | 2 +- hw/display/virtio-gpu-udmabuf.c | 181 ++++++++++++++++++++++++++++++++ include/hw/virtio/virtio-gpu.h | 11 ++ 3 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 hw/display/virtio-gpu-udmabuf.c