| Message ID | 20240411102002.240536-8-dmitry.osipenko@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support blob memory and venus on qemu | expand |
On 2024/04/11 19:19, Dmitry Osipenko wrote: > From: Antonio Caggiano <antonio.caggiano@collabora.com> > > Support BLOB resources creation, mapping and unmapping by calling the > new stable virglrenderer 0.10 interface. Only enabled when available and > via the blob config. E.g. -device virtio-vga-gl,blob=true > > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > Signed-off-by: Huang Rui <ray.huang@amd.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > hw/display/virtio-gpu-virgl.c | 196 +++++++++++++++++++++++++++++++++ > hw/display/virtio-gpu.c | 4 +- > include/hw/virtio/virtio-gpu.h | 1 + > 3 files changed, 200 insertions(+), 1 deletion(-) > > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > index c2057b0c2147..ec63f5d698b7 100644 > --- a/hw/display/virtio-gpu-virgl.c > +++ b/hw/display/virtio-gpu-virgl.c > @@ -32,6 +32,55 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie) > } > #endif > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > +static int > +virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res, > + uint64_t offset) > +{ > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + uint64_t size; > + void *data; > + int ret; > + > + if (!virtio_gpu_hostmem_enabled(b->conf)) { > + return -EOPNOTSUPP; > + } > + > + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); > + if (ret) { > + return -ret; > + } > + > + res->mr = g_new0(MemoryRegion, 1); > + memory_region_init_ram_ptr(res->mr, OBJECT(res->mr), "blob", > + size, data); > + memory_region_add_subregion(&b->hostmem, offset, res->mr); > + memory_region_set_enabled(res->mr, true); > + > + return 0; > +} > + > +static void > +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, > + struct virtio_gpu_simple_resource *res) > +{ > + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); > + > + if (!res->mr) { > + return; > + } > + > + memory_region_set_enabled(res->mr, false); > + memory_region_del_subregion(&b->hostmem, res->mr); > + > + /* memory region owns res->mr object and frees it when mr is released */ > + res->mr = NULL; > + > + virgl_renderer_resource_unmap(res->resource_id); Hi, First, thanks for keeping working on this. This patch has some changes since the previous version, but it is still vulnerable to the race condition pointed out. The memory region is asynchronously unmapped from the guest address space, but the backing memory on the host address space is unmapped synchronously before that. This results in use-after-free. The whole unmapping operation needs to be implemented in an asynchronous manner. Regards, Akihiko Odaki > +} > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ > + > static void virgl_cmd_create_resource_2d(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > @@ -145,6 +194,10 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, > return; > } > > +#ifdef HAVE_VIRGL_RESOURCE_BLOB > + virtio_gpu_virgl_unmap_resource_blob(g, res); > +#endif > + > virgl_renderer_resource_detach_iov(unref.resource_id, > &res_iovs, > &num_iovs); > @@ -495,6 +548,140 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, > } > > #ifdef HAVE_VIRGL_RESOURCE_BLOB > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; > + struct virtio_gpu_resource_create_blob cblob; > + struct virtio_gpu_simple_resource *res; > + int ret; > + > + if (!virtio_gpu_blob_enabled(g->parent_obj.conf)) { > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; > + return; > + } > + > + VIRTIO_GPU_FILL_CMD(cblob); > + virtio_gpu_create_blob_bswap(&cblob); > + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); > + > + if (cblob.resource_id == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", > + __func__); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = virtio_gpu_find_resource(g, cblob.resource_id); > + if (res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", > + __func__, cblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + res = g_new0(struct virtio_gpu_simple_resource, 1); > + res->resource_id = cblob.resource_id; > + res->blob_size = cblob.size; > + res->dmabuf_fd = -1; > + > + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { > + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), > + cmd, &res->addrs, > + &res->iov, &res->iov_cnt); > + if (!ret) { > + g_free(res); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + return; > + } > + } > + > + QTAILQ_INSERT_HEAD(&g->reslist, res, next); > + > + virgl_args.res_handle = cblob.resource_id; > + virgl_args.ctx_id = cblob.hdr.ctx_id; > + virgl_args.blob_mem = cblob.blob_mem; > + virgl_args.blob_id = cblob.blob_id; > + virgl_args.blob_flags = cblob.blob_flags; > + virgl_args.size = cblob.size; > + virgl_args.iovecs = res->iov; > + virgl_args.num_iovs = res->iov_cnt; > + > + ret = virgl_renderer_resource_create_blob(&virgl_args); > + if (ret) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", > + __func__, strerror(-ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; > + } > +} > + > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_resource_map_blob mblob; > + struct virtio_gpu_simple_resource *res; > + struct virtio_gpu_resp_map_info resp; > + int ret; > + > + VIRTIO_GPU_FILL_CMD(mblob); > + virtio_gpu_map_blob_bswap(&mblob); > + > + res = virtio_gpu_find_resource(g, mblob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + if (res->mr) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", > + __func__, mblob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset); > + if (ret) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", > + __func__, strerror(ret)); > + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; > + return; > + } > + > + memset(&resp, 0, sizeof(resp)); > + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; > + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); > + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); > +} > + > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, > + struct virtio_gpu_ctrl_command *cmd) > +{ > + struct virtio_gpu_resource_unmap_blob ublob; > + struct virtio_gpu_simple_resource *res; > + > + VIRTIO_GPU_FILL_CMD(ublob); > + virtio_gpu_unmap_blob_bswap(&ublob); > + > + res = virtio_gpu_find_resource(g, ublob.resource_id); > + if (!res) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", > + __func__, ublob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + if (!res->mr) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", > + __func__, ublob.resource_id); > + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; > + return; > + } > + > + virtio_gpu_virgl_unmap_resource_blob(g, res); > +} > + > static void virgl_cmd_set_scanout_blob(VirtIOGPU *g, > struct virtio_gpu_ctrl_command *cmd) > { > @@ -661,6 +848,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, > virtio_gpu_get_edid(g, cmd); > break; > #ifdef HAVE_VIRGL_RESOURCE_BLOB > + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: > + virgl_cmd_resource_create_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: > + virgl_cmd_resource_map_blob(g, cmd); > + break; > + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: > + virgl_cmd_resource_unmap_blob(g, cmd); > + break; > case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB: > virgl_cmd_set_scanout_blob(g, cmd); > break; > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 1e57a53d346c..e6e09f4bf8da 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1478,10 +1478,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > +#ifndef HAVE_VIRGL_RESOURCE_BLOB > if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { > - error_setg(errp, "blobs and virgl are not compatible (yet)"); > + error_setg(errp, "old virglrenderer, blob resources unsupported"); > return; > } > +#endif > } > > if (!virtio_gpu_base_device_realize(qdev, > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h > index 44c676c3ca4a..f3681476eaf6 100644 > --- a/include/hw/virtio/virtio-gpu.h > +++ b/include/hw/virtio/virtio-gpu.h > @@ -60,6 +60,7 @@ struct virtio_gpu_simple_resource { > void *blob; > int dmabuf_fd; > uint8_t *remapped; > + MemoryRegion *mr; > > QTAILQ_ENTRY(virtio_gpu_simple_resource) next; > };
Hello, On 4/13/24 14:57, Akihiko Odaki wrote: ... >> +static void >> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, >> + struct >> virtio_gpu_simple_resource *res) >> +{ >> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >> + >> + if (!res->mr) { >> + return; >> + } >> + >> + memory_region_set_enabled(res->mr, false); >> + memory_region_del_subregion(&b->hostmem, res->mr); >> + >> + /* memory region owns res->mr object and frees it when mr is >> released */ >> + res->mr = NULL; >> + >> + virgl_renderer_resource_unmap(res->resource_id); > > Hi, > > First, thanks for keeping working on this. > > This patch has some changes since the previous version, but it is still > vulnerable to the race condition pointed out. The memory region is > asynchronously unmapped from the guest address space, but the backing > memory on the host address space is unmapped synchronously before that. > This results in use-after-free. The whole unmapping operation needs to > be implemented in an asynchronous manner. Thanks for the clarification! I missed this point from the previous discussion. Could you please clarify what do you mean by the "asynchronous manner"? Virglrenderer API works only in the virtio-gpu-gl context, it can't be accessed from other places. The memory_region_del_subregion() should remove the region as long as nobody references it, isn't it? On Linux guest nobody should reference hostmem regions besides virtio-gpu device on the unmap, don't know about other guests. We can claim it a guest's fault if MR lives after the deletion and in that case exit Qemu with a noisy error msg or leak resource. WDYT?
On 2024/04/15 17:03, Dmitry Osipenko wrote: > Hello, > > On 4/13/24 14:57, Akihiko Odaki wrote: > ... >>> +static void >>> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, >>> + struct >>> virtio_gpu_simple_resource *res) >>> +{ >>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >>> + >>> + if (!res->mr) { >>> + return; >>> + } >>> + >>> + memory_region_set_enabled(res->mr, false); >>> + memory_region_del_subregion(&b->hostmem, res->mr); >>> + >>> + /* memory region owns res->mr object and frees it when mr is >>> released */ >>> + res->mr = NULL; >>> + >>> + virgl_renderer_resource_unmap(res->resource_id); >> >> Hi, >> >> First, thanks for keeping working on this. >> >> This patch has some changes since the previous version, but it is still >> vulnerable to the race condition pointed out. The memory region is >> asynchronously unmapped from the guest address space, but the backing >> memory on the host address space is unmapped synchronously before that. >> This results in use-after-free. The whole unmapping operation needs to >> be implemented in an asynchronous manner. > > Thanks for the clarification! I missed this point from the previous > discussion. > > Could you please clarify what do you mean by the "asynchronous manner"? > Virglrenderer API works only in the virtio-gpu-gl context, it can't be > accessed from other places. > > The memory_region_del_subregion() should remove the region as long as > nobody references it, isn't it? On Linux guest nobody should reference > hostmem regions besides virtio-gpu device on the unmap, don't know about > other guests. > > We can claim it a guest's fault if MR lives after the deletion and in > that case exit Qemu with a noisy error msg or leak resource. WDYT? > We need to be prepared for a faulty guest for reliability and security as they are common goals of virtualization, and it is nice to have them after all. You need to call virgl_renderer_resource_unmap() after the MR actually gets freed. The virtio-gpu-gl context is just a context with BQL so it is fine to call virgl functions in most places. Also the command response must be delayed in a similar manner. Currently virtio_gpu_virgl_process_cmd() synchronously returns command responses so this needs to be changed. Regards, Akihiko Odaki
On 4/15/24 11:13, Akihiko Odaki wrote: > On 2024/04/15 17:03, Dmitry Osipenko wrote: >> Hello, >> >> On 4/13/24 14:57, Akihiko Odaki wrote: >> ... >>>> +static void >>>> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, >>>> + struct >>>> virtio_gpu_simple_resource *res) >>>> +{ >>>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >>>> + >>>> + if (!res->mr) { >>>> + return; >>>> + } >>>> + >>>> + memory_region_set_enabled(res->mr, false); >>>> + memory_region_del_subregion(&b->hostmem, res->mr); >>>> + >>>> + /* memory region owns res->mr object and frees it when mr is >>>> released */ >>>> + res->mr = NULL; >>>> + >>>> + virgl_renderer_resource_unmap(res->resource_id); >>> >>> Hi, >>> >>> First, thanks for keeping working on this. >>> >>> This patch has some changes since the previous version, but it is still >>> vulnerable to the race condition pointed out. The memory region is >>> asynchronously unmapped from the guest address space, but the backing >>> memory on the host address space is unmapped synchronously before that. >>> This results in use-after-free. The whole unmapping operation needs to >>> be implemented in an asynchronous manner. >> >> Thanks for the clarification! I missed this point from the previous >> discussion. >> >> Could you please clarify what do you mean by the "asynchronous manner"? >> Virglrenderer API works only in the virtio-gpu-gl context, it can't be >> accessed from other places. >> >> The memory_region_del_subregion() should remove the region as long as >> nobody references it, isn't it? On Linux guest nobody should reference >> hostmem regions besides virtio-gpu device on the unmap, don't know about >> other guests. >> >> We can claim it a guest's fault if MR lives after the deletion and in >> that case exit Qemu with a noisy error msg or leak resource. WDYT? >> > > We need to be prepared for a faulty guest for reliability and security > as they are common goals of virtualization, and it is nice to have them > after all. > > You need to call virgl_renderer_resource_unmap() after the MR actually > gets freed. The virtio-gpu-gl context is just a context with BQL so it > is fine to call virgl functions in most places. Do you have example of a legit use-case where hostmem MR could outlive resource mapping? Turning it into a error condition is much more reasonable to do than to to worry about edge case that nobody cares about, which can't be tested easily and that not trivial to support, IMO.
On 2024/04/15 17:49, Dmitry Osipenko wrote: > On 4/15/24 11:13, Akihiko Odaki wrote: >> On 2024/04/15 17:03, Dmitry Osipenko wrote: >>> Hello, >>> >>> On 4/13/24 14:57, Akihiko Odaki wrote: >>> ... >>>>> +static void >>>>> +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, >>>>> + struct >>>>> virtio_gpu_simple_resource *res) >>>>> +{ >>>>> + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); >>>>> + >>>>> + if (!res->mr) { >>>>> + return; >>>>> + } >>>>> + >>>>> + memory_region_set_enabled(res->mr, false); >>>>> + memory_region_del_subregion(&b->hostmem, res->mr); >>>>> + >>>>> + /* memory region owns res->mr object and frees it when mr is >>>>> released */ >>>>> + res->mr = NULL; >>>>> + >>>>> + virgl_renderer_resource_unmap(res->resource_id); >>>> >>>> Hi, >>>> >>>> First, thanks for keeping working on this. >>>> >>>> This patch has some changes since the previous version, but it is still >>>> vulnerable to the race condition pointed out. The memory region is >>>> asynchronously unmapped from the guest address space, but the backing >>>> memory on the host address space is unmapped synchronously before that. >>>> This results in use-after-free. The whole unmapping operation needs to >>>> be implemented in an asynchronous manner. >>> >>> Thanks for the clarification! I missed this point from the previous >>> discussion. >>> >>> Could you please clarify what do you mean by the "asynchronous manner"? >>> Virglrenderer API works only in the virtio-gpu-gl context, it can't be >>> accessed from other places. >>> >>> The memory_region_del_subregion() should remove the region as long as >>> nobody references it, isn't it? On Linux guest nobody should reference >>> hostmem regions besides virtio-gpu device on the unmap, don't know about >>> other guests. >>> >>> We can claim it a guest's fault if MR lives after the deletion and in >>> that case exit Qemu with a noisy error msg or leak resource. WDYT? >>> >> >> We need to be prepared for a faulty guest for reliability and security >> as they are common goals of virtualization, and it is nice to have them >> after all. >> >> You need to call virgl_renderer_resource_unmap() after the MR actually >> gets freed. The virtio-gpu-gl context is just a context with BQL so it >> is fine to call virgl functions in most places. > > Do you have example of a legit use-case where hostmem MR could outlive > resource mapping? MR outliving after memory_region_del_subregion() is not a use-case, but a situation that occurs due to the limitation of the memory subsystem. It is not easy to answer how often such a situation happens. > > Turning it into a error condition is much more reasonable to do than to > to worry about edge case that nobody cares about, which can't be tested > easily and that not trivial to support, IMO. > I'm not sure what you mean by turning into an error condition. I doubt it's possible to emit errors when someone touches an unmapped region. Reproducing this issue is not easy as it's often cases for use-after-free bugs, but fixing it is not that complicated in my opinion since you already have an implementation which asynchronously unmaps the region in v6. I write my suggestions to fix problems in v6: - Remove ref member in virgl_gpu_resource, vres_get_ref(), vres_put_ref(), and virgl_resource_unmap(). - Change virtio_gpu_virgl_process_cmd(), virgl_cmd_resource_unmap_blob(), and virgl_cmd_resource_unref() to return a bool, which tells if the command was processed or suspended. - In virtio_gpu_process_cmdq(), break if the command was suspended. - In virgl_resource_blob_async_unmap(), call virtio_gpu_gl_block(g, false). - In virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref(), call memory_region_del_subregion() and virtio_gpu_gl_block(g, true), and tell that the command was suspended if the reference counter of MemoryRegion > 0. Free and unmap the MR otherwise. Regards, Akihiko Odaki
On 4/15/24 13:05, Akihiko Odaki wrote: ... >> Do you have example of a legit use-case where hostmem MR could outlive >> resource mapping? > > MR outliving after memory_region_del_subregion() is not a use-case, but > a situation that occurs due to the limitation of the memory subsystem. > It is not easy to answer how often such a situation happens. > >> >> Turning it into a error condition is much more reasonable to do than to >> to worry about edge case that nobody cares about, which can't be tested >> easily and that not trivial to support, IMO. >> > I'm not sure what you mean by turning into an error condition. I doubt > it's possible to emit errors when someone touches an unmapped region. My idea was about failing in virtio_gpu_virgl_unmap_resource_blob() where we could check whether MR has external reference and fail if it has. > Reproducing this issue is not easy as it's often cases for > use-after-free bugs, but fixing it is not that complicated in my opinion > since you already have an implementation which asynchronously unmaps the > region in v6. I write my suggestions to fix problems in v6: > > - Remove ref member in virgl_gpu_resource, vres_get_ref(), > vres_put_ref(), and virgl_resource_unmap(). > > - Change virtio_gpu_virgl_process_cmd(), > virgl_cmd_resource_unmap_blob(), and virgl_cmd_resource_unref() to > return a bool, which tells if the command was processed or suspended. > > - In virtio_gpu_process_cmdq(), break if the command was suspended. > > - In virgl_resource_blob_async_unmap(), call virtio_gpu_gl_block(g, false). > > - In virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref(), > call memory_region_del_subregion() and virtio_gpu_gl_block(g, true), and > tell that the command was suspended if the reference counter of > MemoryRegion > 0. Free and unmap the MR otherwise. Your suggestion works, I'll proceed with it in v8. Thanks for the review
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c index c2057b0c2147..ec63f5d698b7 100644 --- a/hw/display/virtio-gpu-virgl.c +++ b/hw/display/virtio-gpu-virgl.c @@ -32,6 +32,55 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie) } #endif +#ifdef HAVE_VIRGL_RESOURCE_BLOB +static int +virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res, + uint64_t offset) +{ + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); + uint64_t size; + void *data; + int ret; + + if (!virtio_gpu_hostmem_enabled(b->conf)) { + return -EOPNOTSUPP; + } + + ret = virgl_renderer_resource_map(res->resource_id, &data, &size); + if (ret) { + return -ret; + } + + res->mr = g_new0(MemoryRegion, 1); + memory_region_init_ram_ptr(res->mr, OBJECT(res->mr), "blob", + size, data); + memory_region_add_subregion(&b->hostmem, offset, res->mr); + memory_region_set_enabled(res->mr, true); + + return 0; +} + +static void +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res) +{ + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); + + if (!res->mr) { + return; + } + + memory_region_set_enabled(res->mr, false); + memory_region_del_subregion(&b->hostmem, res->mr); + + /* memory region owns res->mr object and frees it when mr is released */ + res->mr = NULL; + + virgl_renderer_resource_unmap(res->resource_id); +} +#endif /* HAVE_VIRGL_RESOURCE_BLOB */ + static void virgl_cmd_create_resource_2d(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { @@ -145,6 +194,10 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, return; } +#ifdef HAVE_VIRGL_RESOURCE_BLOB + virtio_gpu_virgl_unmap_resource_blob(g, res); +#endif + virgl_renderer_resource_detach_iov(unref.resource_id, &res_iovs, &num_iovs); @@ -495,6 +548,140 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, } #ifdef HAVE_VIRGL_RESOURCE_BLOB +static void virgl_cmd_resource_create_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virgl_renderer_resource_create_blob_args virgl_args = { 0 }; + struct virtio_gpu_resource_create_blob cblob; + struct virtio_gpu_simple_resource *res; + int ret; + + if (!virtio_gpu_blob_enabled(g->parent_obj.conf)) { + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER; + return; + } + + VIRTIO_GPU_FILL_CMD(cblob); + virtio_gpu_create_blob_bswap(&cblob); + trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size); + + if (cblob.resource_id == 0) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n", + __func__); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = virtio_gpu_find_resource(g, cblob.resource_id); + if (res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n", + __func__, cblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + res = g_new0(struct virtio_gpu_simple_resource, 1); + res->resource_id = cblob.resource_id; + res->blob_size = cblob.size; + res->dmabuf_fd = -1; + + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) { + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob), + cmd, &res->addrs, + &res->iov, &res->iov_cnt); + if (!ret) { + g_free(res); + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; + return; + } + } + + QTAILQ_INSERT_HEAD(&g->reslist, res, next); + + virgl_args.res_handle = cblob.resource_id; + virgl_args.ctx_id = cblob.hdr.ctx_id; + virgl_args.blob_mem = cblob.blob_mem; + virgl_args.blob_id = cblob.blob_id; + virgl_args.blob_flags = cblob.blob_flags; + virgl_args.size = cblob.size; + virgl_args.iovecs = res->iov; + virgl_args.num_iovs = res->iov_cnt; + + ret = virgl_renderer_resource_create_blob(&virgl_args); + if (ret) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n", + __func__, strerror(-ret)); + cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; + } +} + +static void virgl_cmd_resource_map_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virtio_gpu_resource_map_blob mblob; + struct virtio_gpu_simple_resource *res; + struct virtio_gpu_resp_map_info resp; + int ret; + + VIRTIO_GPU_FILL_CMD(mblob); + virtio_gpu_map_blob_bswap(&mblob); + + res = virtio_gpu_find_resource(g, mblob.resource_id); + if (!res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", + __func__, mblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + if (res->mr) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n", + __func__, mblob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset); + if (ret) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n", + __func__, strerror(ret)); + cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY; + return; + } + + memset(&resp, 0, sizeof(resp)); + resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO; + virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info); + virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp)); +} + +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g, + struct virtio_gpu_ctrl_command *cmd) +{ + struct virtio_gpu_resource_unmap_blob ublob; + struct virtio_gpu_simple_resource *res; + + VIRTIO_GPU_FILL_CMD(ublob); + virtio_gpu_unmap_blob_bswap(&ublob); + + res = virtio_gpu_find_resource(g, ublob.resource_id); + if (!res) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n", + __func__, ublob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + if (!res->mr) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n", + __func__, ublob.resource_id); + cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID; + return; + } + + virtio_gpu_virgl_unmap_resource_blob(g, res); +} + static void virgl_cmd_set_scanout_blob(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { @@ -661,6 +848,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g, virtio_gpu_get_edid(g, cmd); break; #ifdef HAVE_VIRGL_RESOURCE_BLOB + case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB: + virgl_cmd_resource_create_blob(g, cmd); + break; + case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB: + virgl_cmd_resource_map_blob(g, cmd); + break; + case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB: + virgl_cmd_resource_unmap_blob(g, cmd); + break; case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB: virgl_cmd_set_scanout_blob(g, cmd); break; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 1e57a53d346c..e6e09f4bf8da 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1478,10 +1478,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) return; } +#ifndef HAVE_VIRGL_RESOURCE_BLOB if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) { - error_setg(errp, "blobs and virgl are not compatible (yet)"); + error_setg(errp, "old virglrenderer, blob resources unsupported"); return; } +#endif } if (!virtio_gpu_base_device_realize(qdev, diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index 44c676c3ca4a..f3681476eaf6 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -60,6 +60,7 @@ struct virtio_gpu_simple_resource { void *blob; int dmabuf_fd; uint8_t *remapped; + MemoryRegion *mr; QTAILQ_ENTRY(virtio_gpu_simple_resource) next; };