[PATCH v6 00/11] Support blob memory and venus on qemu

[PATCH v6 00/11] Support blob memory and venus on qemu

[Huang Rui]

Hi all,

Sorry to late for V6, I was occupied by other stuff last two months, and
right now resume the submission.

Antonio Caggiano made the venus with QEMU on KVM platform last
September[1]. This series are inherited from his original work to support
the features of context init, hostmem, resource uuid, and blob resources
for venus.
At March of this year, we sent out the V1 version[2] for the review. But
those series are included both xen and virtio gpu. Right now, we would like
to divide into two parts, one is to continue the Antonio's work to upstream
virtio-gpu support for blob memory and venus, and another is to upstream
xen specific patches. This series is focusing on virtio-gpu, so we are
marking as V4 version here to continue Antonio's patches[1]. And we will
send xen specific patches separately, because they are hypervisor specific.
Besides of QEMU, these supports also included virglrenderer[3][4] and
mesa[5][6] as well. Right now, virglrenderer and mesa parts are all
accepted by upstream. In this qemu version, we try to address the concerns
around not proper cleanup during blob resource unmap and unref. Appreciate
it if you have any commments.

[1] https://lore.kernel.org/qemu-devel/20220926142422.22325-1-antonio.caggiano@collabora.com/
[2] V1: https://lore.kernel.org/qemu-devel/20230312092244.451465-1-ray.huang@amd.com
[3] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1068
[4] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1180
[5] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22108
[6] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23680

Please note the first 4 patches 1 -> 4 are inlcuded in these series because
the series depends on them and not because we want them to be reviewed
since they are already in the process of review through the "rutabaga_gfx +
gfxstream" series.
- https://lore.kernel.org/qemu-devel/20230829003629.410-1-gurchetansingh@chromium.org/

V4: https://lore.kernel.org/qemu-devel/20230831093252.2461282-1-ray.huang@amd.com
V5: https://lore.kernel.org/qemu-devel/20230915111130.24064-1-ray.huang@amd.com

Changes from V5 to V6

- Move macros configurations under virgl.found() and rename
  HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS.

- Handle the case while context_init is disabled.

- Enable context_init by default.

- Move virtio_gpu_virgl_resource_unmap() into
  virgl_cmd_resource_unmap_blob().

- Introduce new struct virgl_gpu_resource to store virgl specific members.

- Remove erro handling of g_new0, because glib will abort() on OOM.

- Set resource uuid as option.

- Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
  for virtio live migration.

- Use g_int_hash/g_int_equal instead of the default

- Add scanout_blob function for virtio-gpu-virgl

- Resolve the memory leak on virtio-gpu-virgl

- Remove the unstable API flags check because virglrenderer is already 1.0

- Squash the render server flag support into "Initialize Venus"

Changes from V4 (virtio gpu V4) to V5

- Inverted patch 5 and 6 because we should configure
  HAVE_VIRGL_CONTEXT_INIT firstly.

- Validate owner of memory region to avoid slowing down DMA.

- Use memory_region_init_ram_ptr() instead of
  memory_region_init_ram_device_ptr().

- Adjust sequence to allocate gpu resource before virglrender resource
  creation

- Add virtio migration handling for uuid.

- Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS.
  https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/

- Add meson check to make sure unstable APIs defined from 0.9.0.

Changes from V1 to V2 (virtio gpu V4)

- Remove unused #include "hw/virtio/virtio-iommu.h"

- Add a local function, called virgl_resource_destroy(), that is used
  to release a vgpu resource on error paths and in resource_unref.

- Remove virtio_gpu_virgl_resource_unmap from
  virtio_gpu_cleanup_mapping(),
  since this function won't be called on blob resources and also because
  blob resources are unmapped via virgl_cmd_resource_unmap_blob().

- In virgl_cmd_resource_create_blob(), do proper cleanup in error paths
  and move QTAILQ_INSERT_HEAD(&g->reslist, res, next) after the resource
  has been fully initialized.

- Memory region has a different life-cycle from virtio gpu resources
  i.e. cannot be released synchronously along with the vgpu resource.
  So, here the field "region" was changed to a pointer and is allocated
  dynamically when the blob is mapped.
  Also, since the pointer can be used to indicate whether the blob
  is mapped, the explicite field "mapped" was removed.

- In virgl_cmd_resource_map_blob(), add check on the value of
  res->region, to prevent beeing called twice on the same resource.

- Add a patch to enable automatic deallocation of memory regions to resolve
  use-after-free memory corruption with a reference.

References

Demo with Venus:
- https://static.sched.com/hosted_files/xen2023/3f/xen_summit_2023_virtgpu_demo.mp4
QEMU repository:
- https://gitlab.freedesktop.org/rui/qemu-xen/-/commits/upstream-for-virtio-gpu

Thanks,
Ray

Antonio Caggiano (4):
  virtio-gpu: Handle resource blob commands
  virtio-gpu: Resource UUID
  virtio-gpu: Support Venus capset
  virtio-gpu: Initialize Venus

Dmitry Osipenko (1):
  virtio-gpu: Don't require udmabuf when blobs and virgl are enabled

Huang Rui (4):
  linux-headers: Update to kernel headers to add venus capset
  virtio-gpu: Configure new feature flag context_create_with_flags for
    virglrenderer
  virtio-gpu: Support context init feature with virglrenderer
  virtio-gpu: Introduce virgl_gpu_resource structure

Robert Beckett (1):
  virtio-gpu: make blob scanout use dmabuf fd

Xenia Ragiadakou (1):
  softmmu/memory: enable automatic deallocation of memory regions

 hw/display/trace-events                     |   1 +
 hw/display/virtio-gpu-base.c                |   4 +
 hw/display/virtio-gpu-virgl.c               | 495 ++++++++++++++++++--
 hw/display/virtio-gpu.c                     | 132 +++++-
 include/hw/virtio/virtio-gpu.h              |  13 +
 include/standard-headers/linux/virtio_gpu.h |   2 +
 meson.build                                 |   8 +
 system/memory.c                             |   2 +
 8 files changed, 627 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Huang Rui]

Sync up kernel headers to update venus macro till they are merged into
mainline.

Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Venus capset is applied in kernel, so update it in qemu for future use.

https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0

 include/standard-headers/linux/virtio_gpu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/virtio_gpu.h b/include/standard-headers/linux/virtio_gpu.h
index 2da48d3d4c..2db643ed8f 100644
--- a/include/standard-headers/linux/virtio_gpu.h
+++ b/include/standard-headers/linux/virtio_gpu.h
@@ -309,6 +309,8 @@ struct virtio_gpu_cmd_submit {
 
 #define VIRTIO_GPU_CAPSET_VIRGL 1
 #define VIRTIO_GPU_CAPSET_VIRGL2 2
+/* 3 is reserved for gfxstream */
+#define VIRTIO_GPU_CAPSET_VENUS 4
 
 /* VIRTIO_GPU_CMD_GET_CAPSET_INFO */
 struct virtio_gpu_get_capset_info {
-- 
2.25.1

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> Sync up kernel headers to update venus macro till they are merged into
> mainline.

Thanks for sorting things out with the kernel and spec.

> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Venus capset is applied in kernel, so update it in qemu for future use.
> 
> https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
Please include the link to the upstream commit in the commit message.

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Huang Rui]

On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> On 2023/12/19 16:53, Huang Rui wrote:
> > Sync up kernel headers to update venus macro till they are merged into
> > mainline.
> 
> Thanks for sorting things out with the kernel and spec.
> 
> > 
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > Changes in v6:
> > - Venus capset is applied in kernel, so update it in qemu for future use.
> > 
> > https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> > https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> Please include the link to the upstream commit in the commit message.

So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
need to wait it to be merged into kernel mainline?

Thanks,
Ray

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Peter Maydell]

On Tue, 19 Dec 2023 at 13:49, Huang Rui <ray.huang@amd.com> wrote:
>
> On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> > On 2023/12/19 16:53, Huang Rui wrote:
> > > Sync up kernel headers to update venus macro till they are merged into
> > > mainline.
> >
> > Thanks for sorting things out with the kernel and spec.
> >
> > >
> > > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > > ---
> > >
> > > Changes in v6:
> > > - Venus capset is applied in kernel, so update it in qemu for future use.
> > >
> > > https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> > > https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> > Please include the link to the upstream commit in the commit message.
>
> So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
> need to wait it to be merged into kernel mainline?

For an RFC patchset, no. For patches to be merged into QEMU
the headers change must be in the kernel mainline, and the
QEMU commit that updates our copy of the headers must be a
full-sync done with scripts/update-linux-headers.sh, not a
manual edit.

thanks
-- PMM

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Akihiko Odaki]

On 2023/12/19 23:14, Peter Maydell wrote:
> On Tue, 19 Dec 2023 at 13:49, Huang Rui <ray.huang@amd.com> wrote:
>>
>> On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
>>> On 2023/12/19 16:53, Huang Rui wrote:
>>>> Sync up kernel headers to update venus macro till they are merged into
>>>> mainline.
>>>
>>> Thanks for sorting things out with the kernel and spec.
>>>
>>>>
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> ---
>>>>
>>>> Changes in v6:
>>>> - Venus capset is applied in kernel, so update it in qemu for future use.
>>>>
>>>> https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
>>>> https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
>>> Please include the link to the upstream commit in the commit message.
>>
>> So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
>> need to wait it to be merged into kernel mainline?
> 
> For an RFC patchset, no. For patches to be merged into QEMU
> the headers change must be in the kernel mainline, and the
> QEMU commit that updates our copy of the headers must be a
> full-sync done with scripts/update-linux-headers.sh, not a
> manual edit.

Apparently the kernel change is unlikely to be merged to mainline before 
QEMU 9.0 so we need to come up with some idea to deal with this.

The release of Linux 6.7 is near and the development of 6.8 will start 
soon. So it was nice if the change made into 6.8, but unfortunately it 
missed the *probably last* drm-misc tree pull request for 6.8:
https://lore.kernel.org/all/aqpn5miejmkks7pbcfex7b6u63uwsruywxsnr3x5ljs45qatin@nbkkej2elk46/

It will still get into linux-next so we may retrieve headers from 
linux-next. Or we may add the definition to 
hw/display/virtio-gpu-virgl.c; the duplicate definition warning will 
tell when the definition becomes no longer necessary. I'm fine with 
either option.

Regards,
Akihiko Odaki

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Marc-André Lureau]

Hi

On Thu, Dec 21, 2023 at 10:55 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2023/12/19 23:14, Peter Maydell wrote:
> > On Tue, 19 Dec 2023 at 13:49, Huang Rui <ray.huang@amd.com> wrote:
> >>
> >> On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> >>> On 2023/12/19 16:53, Huang Rui wrote:
> >>>> Sync up kernel headers to update venus macro till they are merged into
> >>>> mainline.
> >>>
> >>> Thanks for sorting things out with the kernel and spec.
> >>>
> >>>>
> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >>>> ---
> >>>>
> >>>> Changes in v6:
> >>>> - Venus capset is applied in kernel, so update it in qemu for future use.
> >>>>
> >>>> https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> >>>> https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> >>> Please include the link to the upstream commit in the commit message.
> >>
> >> So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
> >> need to wait it to be merged into kernel mainline?
> >
> > For an RFC patchset, no. For patches to be merged into QEMU
> > the headers change must be in the kernel mainline, and the
> > QEMU commit that updates our copy of the headers must be a
> > full-sync done with scripts/update-linux-headers.sh, not a
> > manual edit.
>
> Apparently the kernel change is unlikely to be merged to mainline before
> QEMU 9.0 so we need to come up with some idea to deal with this.
>
> The release of Linux 6.7 is near and the development of 6.8 will start
> soon. So it was nice if the change made into 6.8, but unfortunately it
> missed the *probably last* drm-misc tree pull request for 6.8:
> https://lore.kernel.org/all/aqpn5miejmkks7pbcfex7b6u63uwsruywxsnr3x5ljs45qatin@nbkkej2elk46/
>
> It will still get into linux-next so we may retrieve headers from
> linux-next. Or we may add the definition to
> hw/display/virtio-gpu-virgl.c; the duplicate definition warning will
> tell when the definition becomes no longer necessary. I'm fine with
> either option.

The second option seems better to me, as it can avoid pulling unwanted changes.

thanks

-- 
Marc-André Lureau

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Huang Rui]

On Tue, Jan 02, 2024 at 06:42:44PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Dec 21, 2023 at 10:55 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > On 2023/12/19 23:14, Peter Maydell wrote:
> > > On Tue, 19 Dec 2023 at 13:49, Huang Rui <ray.huang@amd.com> wrote:
> > >>
> > >> On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> > >>> On 2023/12/19 16:53, Huang Rui wrote:
> > >>>> Sync up kernel headers to update venus macro till they are merged into
> > >>>> mainline.
> > >>>
> > >>> Thanks for sorting things out with the kernel and spec.
> > >>>
> > >>>>
> > >>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> > >>>> ---
> > >>>>
> > >>>> Changes in v6:
> > >>>> - Venus capset is applied in kernel, so update it in qemu for future use.
> > >>>>
> > >>>> https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> > >>>> https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> > >>> Please include the link to the upstream commit in the commit message.
> > >>
> > >> So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
> > >> need to wait it to be merged into kernel mainline?
> > >
> > > For an RFC patchset, no. For patches to be merged into QEMU
> > > the headers change must be in the kernel mainline, and the
> > > QEMU commit that updates our copy of the headers must be a
> > > full-sync done with scripts/update-linux-headers.sh, not a
> > > manual edit.
> >
> > Apparently the kernel change is unlikely to be merged to mainline before
> > QEMU 9.0 so we need to come up with some idea to deal with this.

May I know when QEMU 9.0 will be released?

> >
> > The release of Linux 6.7 is near and the development of 6.8 will start
> > soon. So it was nice if the change made into 6.8, but unfortunately it
> > missed the *probably last* drm-misc tree pull request for 6.8:
> > https://lore.kernel.org/all/aqpn5miejmkks7pbcfex7b6u63uwsruywxsnr3x5ljs45qatin@nbkkej2elk46/
> >
> > It will still get into linux-next so we may retrieve headers from
> > linux-next. Or we may add the definition to
> > hw/display/virtio-gpu-virgl.c; the duplicate definition warning will
> > tell when the definition becomes no longer necessary. I'm fine with
> > either option.
> 
> The second option seems better to me, as it can avoid pulling unwanted changes.
> 

I will keep an eye on dri-devel mailing list. And ok, I will add the
definition in virtio-gpu-virgl.c and remove it once kernel patch is merged
by mainline.

Thanks,
Ray

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Huang Rui]

On Tue, Dec 19, 2023 at 10:14:28PM +0800, Peter Maydell wrote:
> On Tue, 19 Dec 2023 at 13:49, Huang Rui <ray.huang@amd.com> wrote:
> >
> > On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> > > On 2023/12/19 16:53, Huang Rui wrote:
> > > > Sync up kernel headers to update venus macro till they are merged into
> > > > mainline.
> > >
> > > Thanks for sorting things out with the kernel and spec.
> > >
> > > >
> > > > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > > > ---
> > > >
> > > > Changes in v6:
> > > > - Venus capset is applied in kernel, so update it in qemu for future use.
> > > >
> > > > https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> > > > https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> > > Please include the link to the upstream commit in the commit message.
> >
> > So far, it's in drm maintainers' branch not in kernel mainline yet. Do I
> > need to wait it to be merged into kernel mainline?
> 
> For an RFC patchset, no. For patches to be merged into QEMU
> the headers change must be in the kernel mainline, and the
> QEMU commit that updates our copy of the headers must be a
> full-sync done with scripts/update-linux-headers.sh, not a
> manual edit.
> 

Yes, according to the comment in previous series, I am using
update-linux-headers.sh to generate the patch. But here, the patch is not
merged in mainline yet.

Thanks,
Ray

Re: [PATCH v6 01/11] linux-headers: Update to kernel headers to add venus capset

[Huang Rui]

On Tue, Dec 19, 2023 at 08:20:22PM +0800, Akihiko Odaki wrote:
> On 2023/12/19 16:53, Huang Rui wrote:
> > Sync up kernel headers to update venus macro till they are merged into
> > mainline.
> 
> Thanks for sorting things out with the kernel and spec.
> 
> > 
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > Changes in v6:
> > - Venus capset is applied in kernel, so update it in qemu for future use.
> > 
> > https://lore.kernel.org/lkml/b79dcf75-c9e8-490e-644f-3b97d95f7397@collabora.com/
> > https://cgit.freedesktop.org/drm-misc/commit/?id=216d86b9a430f3280e5b631c51e6fd1a7774cfa0
> Please include the link to the upstream commit in the commit message.

OK, I will add this info in qemu commit message.

Thanks,
Ray

[PATCH v6 02/11] virtio-gpu: Configure new feature flag context_create_with_flags for virglrenderer

[Huang Rui]

Configure a new feature flag (context_create_with_flags) for
virglrenderer.

Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Move macros configurations under virgl.found() and rename
  HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS.

 meson.build | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meson.build b/meson.build
index ec01f8b138..ea52ef1b9c 100644
--- a/meson.build
+++ b/meson.build
@@ -1050,6 +1050,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
                          cc.has_member('struct virgl_renderer_resource_info_ext', 'd3d_tex2d',
                                        prefix: '#include <virglrenderer.h>',
                                        dependencies: virgl))
+    config_host_data.set('HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS',
+                         cc.has_function('virgl_renderer_context_create_with_flags',
+                                         prefix: '#include <virglrenderer.h>',
+                                         dependencies: virgl))
   endif
 endif
 rutabaga = not_found
-- 
2.25.1

Re: [PATCH v6 02/11] virtio-gpu: Configure new feature flag context_create_with_flags for virglrenderer

[Antonio Caggiano]

Hi Huang Rui,

Thank you for this new version.

All patches which I did not sign off are reviewed by me :)

Cheers,
Antonio Caggiano

On 19/12/2023 08:53, Huang Rui wrote:
> Configure a new feature flag (context_create_with_flags) for
> virglrenderer.
> 
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Move macros configurations under virgl.found() and rename
>    HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS.
> 
>   meson.build | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index ec01f8b138..ea52ef1b9c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1050,6 +1050,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                            cc.has_member('struct virgl_renderer_resource_info_ext', 'd3d_tex2d',
>                                          prefix: '#include <virglrenderer.h>',
>                                          dependencies: virgl))
> +    config_host_data.set('HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS',
> +                         cc.has_function('virgl_renderer_context_create_with_flags',
> +                                         prefix: '#include <virglrenderer.h>',
> +                                         dependencies: virgl))
>     endif
>   endif
>   rutabaga = not_found

Re: [PATCH v6 02/11] virtio-gpu: Configure new feature flag context_create_with_flags for virglrenderer

[Huang Rui]

On Tue, Dec 19, 2023 at 05:09:27PM +0800, Antonio Caggiano wrote:
> Hi Huang Rui,
> 
> Thank you for this new version.
> 
> All patches which I did not sign off are reviewed by me :)

Thanks Antonio! May I have your RB in next version?

Best Regards,
Ray

> 
> Cheers,
> Antonio Caggiano
> 
> On 19/12/2023 08:53, Huang Rui wrote:
> > Configure a new feature flag (context_create_with_flags) for
> > virglrenderer.
> > 
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > Changes in v6:
> > - Move macros configurations under virgl.found() and rename
> >    HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS.
> > 
> >   meson.build | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index ec01f8b138..ea52ef1b9c 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1050,6 +1050,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
> >                            cc.has_member('struct virgl_renderer_resource_info_ext', 'd3d_tex2d',
> >                                          prefix: '#include <virglrenderer.h>',
> >                                          dependencies: virgl))
> > +    config_host_data.set('HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS',
> > +                         cc.has_function('virgl_renderer_context_create_with_flags',
> > +                                         prefix: '#include <virglrenderer.h>',
> > +                                         dependencies: virgl))
> >     endif
> >   endif
> >   rutabaga = not_found

Re: [PATCH v6 02/11] virtio-gpu: Configure new feature flag context_create_with_flags for virglrenderer

[Alex Bennée]

Huang Rui <ray.huang@amd.com> writes:

> Configure a new feature flag (context_create_with_flags) for
> virglrenderer.
>
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v6 03/11] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
feature flags.
We would like to enable the feature with virglrenderer, so add to create
virgl renderer context with flags using context_id when valid.

Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Handle the case while context_init is disabled.
- Enable context_init by default.

 hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
 hw/display/virtio-gpu.c       |  4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 8bb7a2c21f..5bbc8071b2 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);
 
-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
+    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+        return;
+    }
+#endif
+
+    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
 }
 
 static void virgl_cmd_context_destroy(VirtIOGPU *g,
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index b016d3bac8..8b2f4c6be3 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1619,6 +1619,10 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
+    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.25.1

Re: [PATCH v6 03/11] virtio-gpu: Support context init feature with virglrenderer

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2023 at 11:54 AM Huang Rui <ray.huang@amd.com> wrote:
>
> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> feature flags.
> We would like to enable the feature with virglrenderer, so add to create
> virgl renderer context with flags using context_id when valid.
>
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> Changes in v6:
> - Handle the case while context_init is disabled.
> - Enable context_init by default.
>
>  hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>  hw/display/virtio-gpu.c       |  4 ++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..5bbc8071b2 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>      trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                      cc.debug_name);
>
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +    }
> +#endif
> +
> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>  }
>
>  static void virgl_cmd_context_destroy(VirtIOGPU *g,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index b016d3bac8..8b2f4c6be3 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1619,6 +1619,10 @@ static Property virtio_gpu_properties[] = {
>      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> +#endif

Does it make sense to make this configurable? If the context to be
created asked for a capset id but the one created doesn't respect it,
what's the outcome?

It looks like it should instead set cmd->error.

-- 
Marc-André Lureau

Re: [PATCH v6 03/11] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Tue, Jan 02, 2024 at 07:43:20PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:54 AM Huang Rui <ray.huang@amd.com> wrote:
> >
> > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> > feature flags.
> > We would like to enable the feature with virglrenderer, so add to create
> > virgl renderer context with flags using context_id when valid.
> >
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > Changes in v6:
> > - Handle the case while context_init is disabled.
> > - Enable context_init by default.
> >
> >  hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >  hw/display/virtio-gpu.c       |  4 ++++
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 8bb7a2c21f..5bbc8071b2 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >      trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >                                      cc.debug_name);
> >
> > -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> > -                                  cc.debug_name);
> > +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> > +    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
> > +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> > +                                                 cc.context_init,
> > +                                                 cc.nlen,
> > +                                                 cc.debug_name);
> > +        return;
> > +    }
> > +#endif
> > +
> > +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
> >  }
> >
> >  static void virgl_cmd_context_destroy(VirtIOGPU *g,
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index b016d3bac8..8b2f4c6be3 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1619,6 +1619,10 @@ static Property virtio_gpu_properties[] = {
> >      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> > +#endif
> 
> Does it make sense to make this configurable? If the context to be
> created asked for a capset id but the one created doesn't respect it,
> what's the outcome?
> 
> It looks like it should instead set cmd->error.
> 

Hmm, how about setting VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED in
virtio_gpu_gl_device_realize(). And then drop context_init DEFINE_PROP_BIT
in virtio_gpu_properties. We treat all gl device as context_init enabled
and let virglrenderer report the error if command fails.

Thanks,
Ray

Re: [PATCH v6 03/11] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> feature flags.
> We would like to enable the feature with virglrenderer, so add to create
> virgl renderer context with flags using context_id when valid.
> 
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Handle the case while context_init is disabled.
> - Enable context_init by default.
> 
>   hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>   hw/display/virtio-gpu.c       |  4 ++++
>   2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..5bbc8071b2 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                       cc.debug_name);
>   
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +    }
> +#endif
> +
> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>   }
>   
>   static void virgl_cmd_context_destroy(VirtIOGPU *g,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index b016d3bac8..8b2f4c6be3 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1619,6 +1619,10 @@ static Property virtio_gpu_properties[] = {
>       DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                       VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>       DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,

The convention is to use "-" instead of "_" as delimiters. See comments 
for object_property_add() in: include/qom/object.h

Regards,
Akihiko Odaki

[PATCH v6 04/11] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled

[Huang Rui]

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

The udmabuf usage is mandatory when virgl is disabled and blobs feature
enabled in the Qemu machine configuration. If virgl and blobs are enabled,
then udmabuf requirement is optional. Since udmabuf isn't widely supported
by a popular Linux distros today, let's relax the udmabuf requirement for
blobs=on,virgl=on. Now, a full-featured virtio-gpu acceleration is
available to Qemu users without a need to have udmabuf available in the
system.

Reviewed-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

No change in v6.

 hw/display/virtio-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 8b2f4c6be3..4c3ec9d0ea 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1443,6 +1443,7 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 
     if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
         if (!virtio_gpu_rutabaga_enabled(g->parent_obj.conf) &&
+            !virtio_gpu_virgl_enabled(g->parent_obj.conf) &&
             !virtio_gpu_have_udmabuf()) {
             error_setg(errp, "need rutabaga or udmabuf for blob resources");
             return;
-- 
2.25.1

Re: [PATCH v6 04/11] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled

[Marc-André Lureau]

On Tue, Dec 19, 2023 at 11:54 AM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>
> The udmabuf usage is mandatory when virgl is disabled and blobs feature
> enabled in the Qemu machine configuration. If virgl and blobs are enabled,
> then udmabuf requirement is optional. Since udmabuf isn't widely supported
> by a popular Linux distros today, let's relax the udmabuf requirement for
> blobs=on,virgl=on. Now, a full-featured virtio-gpu acceleration is
> available to Qemu users without a need to have udmabuf available in the
> system.
>
> Reviewed-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>
> No change in v6.
>
>  hw/display/virtio-gpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 8b2f4c6be3..4c3ec9d0ea 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1443,6 +1443,7 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>
>      if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
>          if (!virtio_gpu_rutabaga_enabled(g->parent_obj.conf) &&
> +            !virtio_gpu_virgl_enabled(g->parent_obj.conf) &&
>              !virtio_gpu_have_udmabuf()) {
>              error_setg(errp, "need rutabaga or udmabuf for blob resources");
>              return;
> --
> 2.25.1
>


-- 
Marc-André Lureau

[PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Huang Rui]

Introduce a new virgl_gpu_resource data structure and helper functions
for virgl. It's used to add new member which is specific for virgl in
following patches of blob memory support.

Signed-off-by: Huang Rui <ray.huang@amd.com>
---

New patch:
- Introduce new struct virgl_gpu_resource to store virgl specific members.
- Move resource initialization from path "virtio-gpu: Resource UUID" here.
- Remove error handling of g_new0, because glib will abort() on OOM.
- Set iov and iov_cnt in struct virtio_gpu_simple_resource for all types
  of resources.

 hw/display/virtio-gpu-virgl.c | 84 ++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 20 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 5bbc8071b2..faab374336 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -22,6 +22,23 @@
 
 #include <virglrenderer.h>
 
+struct virgl_gpu_resource {
+    struct virtio_gpu_simple_resource res;
+};
+
+static struct virgl_gpu_resource *
+virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
+{
+    struct virtio_gpu_simple_resource *res;
+
+    res = virtio_gpu_find_resource(g, resource_id);
+    if (!res) {
+        return NULL;
+    }
+
+    return container_of(res, struct virgl_gpu_resource, res);
+}
+
 #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
 static void *
 virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
@@ -35,11 +52,19 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_2d c2d;
     struct virgl_renderer_resource_create_args args;
+    struct virgl_gpu_resource *vres;
 
     VIRTIO_GPU_FILL_CMD(c2d);
     trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
                                        c2d.width, c2d.height);
 
+    vres = g_new0(struct virgl_gpu_resource, 1);
+    vres->res.width = c2d.width;
+    vres->res.height = c2d.height;
+    vres->res.format = c2d.format;
+    vres->res.resource_id = c2d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
+
     args.handle = c2d.resource_id;
     args.target = 2;
     args.format = c2d.format;
@@ -59,11 +84,19 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_3d c3d;
     struct virgl_renderer_resource_create_args args;
+    struct virgl_gpu_resource *vres;
 
     VIRTIO_GPU_FILL_CMD(c3d);
     trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
                                        c3d.width, c3d.height, c3d.depth);
 
+    vres = g_new0(struct virgl_gpu_resource, 1);
+    vres->res.width = c3d.width;
+    vres->res.height = c3d.height;
+    vres->res.format = c3d.format;
+    vres->res.resource_id = c3d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
+
     args.handle = c3d.resource_id;
     args.target = c3d.target;
     args.format = c3d.format;
@@ -82,19 +115,23 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
                                      struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_resource_unref unref;
-    struct iovec *res_iovs = NULL;
-    int num_iovs = 0;
+    struct virgl_gpu_resource *vres;
 
     VIRTIO_GPU_FILL_CMD(unref);
     trace_virtio_gpu_cmd_res_unref(unref.resource_id);
 
-    virgl_renderer_resource_detach_iov(unref.resource_id,
-                                       &res_iovs,
-                                       &num_iovs);
-    if (res_iovs != NULL && num_iovs != 0) {
-        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
+    vres = virgl_gpu_find_resource(g, unref.resource_id);
+    if (!vres) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
     }
+
+    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
     virgl_renderer_resource_unref(unref.resource_id);
+
+    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
+    virtio_gpu_cleanup_mapping(g, &vres->res);
+    g_free(vres);
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -310,44 +347,51 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
                                           struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_resource_attach_backing att_rb;
-    struct iovec *res_iovs;
-    uint32_t res_niov;
+    struct virgl_gpu_resource *vres;
     int ret;
 
     VIRTIO_GPU_FILL_CMD(att_rb);
     trace_virtio_gpu_cmd_res_back_attach(att_rb.resource_id);
 
+    vres = virgl_gpu_find_resource(g, att_rb.resource_id);
+    if (!vres) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
     ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, sizeof(att_rb),
-                                        cmd, NULL, &res_iovs, &res_niov);
+                                        cmd, NULL, &vres->res.iov,
+                                        &vres->res.iov_cnt);
     if (ret != 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
 
     ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
-                                             res_iovs, res_niov);
+                                             vres->res.iov, vres->res.iov_cnt);
 
-    if (ret != 0)
-        virtio_gpu_cleanup_mapping_iov(g, res_iovs, res_niov);
+    if (ret != 0) {
+        virtio_gpu_cleanup_mapping(g, &vres->res);
+    }
 }
 
 static void virgl_resource_detach_backing(VirtIOGPU *g,
                                           struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_resource_detach_backing detach_rb;
-    struct iovec *res_iovs = NULL;
-    int num_iovs = 0;
+    struct virgl_gpu_resource *vres;
 
     VIRTIO_GPU_FILL_CMD(detach_rb);
     trace_virtio_gpu_cmd_res_back_detach(detach_rb.resource_id);
 
-    virgl_renderer_resource_detach_iov(detach_rb.resource_id,
-                                       &res_iovs,
-                                       &num_iovs);
-    if (res_iovs == NULL || num_iovs == 0) {
+    vres = virgl_gpu_find_resource(g, detach_rb.resource_id);
+    if (!vres) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
         return;
     }
-    virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
+
+    virgl_renderer_resource_detach_iov(detach_rb.resource_id, NULL, NULL);
+    virtio_gpu_cleanup_mapping(g, &vres->res);
 }
 
 
-- 
2.25.1

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> Introduce a new virgl_gpu_resource data structure and helper functions
> for virgl. It's used to add new member which is specific for virgl in
> following patches of blob memory support.

The name is ambigious. It should tell that it's specific for virgl.

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Huang Rui]

On Tue, Dec 19, 2023 at 08:35:27PM +0800, Akihiko Odaki wrote:
> On 2023/12/19 16:53, Huang Rui wrote:
> > Introduce a new virgl_gpu_resource data structure and helper functions
> > for virgl. It's used to add new member which is specific for virgl in
> > following patches of blob memory support.
> 
> The name is ambigious. It should tell that it's specific for virgl.

How about "virgl_resource" which inherits virtio_gpu_simple_resource? But
this name is exactly same with the name in virglrenderer.

Thanks,
Ray

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Akihiko Odaki]

On 2023/12/19 22:27, Huang Rui wrote:
> On Tue, Dec 19, 2023 at 08:35:27PM +0800, Akihiko Odaki wrote:
>> On 2023/12/19 16:53, Huang Rui wrote:
>>> Introduce a new virgl_gpu_resource data structure and helper functions
>>> for virgl. It's used to add new member which is specific for virgl in
>>> following patches of blob memory support.
>>
>> The name is ambigious. It should tell that it's specific for virgl.
> 
> How about "virgl_resource" which inherits virtio_gpu_simple_resource? But
> this name is exactly same with the name in virglrenderer.

You can prefix it with virtio_gpu_virgl as virtio_gpu_virgl_init() and 
other functions do.

Regards,
Akihiko Odaki

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Huang Rui]

On Thu, Dec 21, 2023 at 01:43:37PM +0800, Akihiko Odaki wrote:
> On 2023/12/19 22:27, Huang Rui wrote:
> > On Tue, Dec 19, 2023 at 08:35:27PM +0800, Akihiko Odaki wrote:
> >> On 2023/12/19 16:53, Huang Rui wrote:
> >>> Introduce a new virgl_gpu_resource data structure and helper functions
> >>> for virgl. It's used to add new member which is specific for virgl in
> >>> following patches of blob memory support.
> >>
> >> The name is ambigious. It should tell that it's specific for virgl.
> > 
> > How about "virgl_resource" which inherits virtio_gpu_simple_resource? But
> > this name is exactly same with the name in virglrenderer.
> 
> You can prefix it with virtio_gpu_virgl as virtio_gpu_virgl_init() and 
> other functions do.
> 

Thanks, will update it in V7.

Ray

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
>
> Introduce a new virgl_gpu_resource data structure and helper functions
> for virgl. It's used to add new member which is specific for virgl in
> following patches of blob memory support.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> New patch:
> - Introduce new struct virgl_gpu_resource to store virgl specific members.
> - Move resource initialization from path "virtio-gpu: Resource UUID" here.
> - Remove error handling of g_new0, because glib will abort() on OOM.
> - Set iov and iov_cnt in struct virtio_gpu_simple_resource for all types
>   of resources.
>
>  hw/display/virtio-gpu-virgl.c | 84 ++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 20 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 5bbc8071b2..faab374336 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -22,6 +22,23 @@
>
>  #include <virglrenderer.h>
>
> +struct virgl_gpu_resource {
> +    struct virtio_gpu_simple_resource res;
> +};
> +
> +static struct virgl_gpu_resource *
> +virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +
> +    res = virtio_gpu_find_resource(g, resource_id);
> +    if (!res) {
> +        return NULL;
> +    }
> +
> +    return container_of(res, struct virgl_gpu_resource, res);
> +}
> +
>  #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
>  static void *
>  virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
> @@ -35,11 +52,19 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>  {
>      struct virtio_gpu_resource_create_2d c2d;
>      struct virgl_renderer_resource_create_args args;
> +    struct virgl_gpu_resource *vres;
>
>      VIRTIO_GPU_FILL_CMD(c2d);
>      trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
>                                         c2d.width, c2d.height);
>

It should check the resource doesn't already exist (similar to 2d code)

> +    vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres->res.width = c2d.width;
> +    vres->res.height = c2d.height;
> +    vres->res.format = c2d.format;
> +    vres->res.resource_id = c2d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
> +
>      args.handle = c2d.resource_id;
>      args.target = 2;
>      args.format = c2d.format;
> @@ -59,11 +84,19 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>  {
>      struct virtio_gpu_resource_create_3d c3d;
>      struct virgl_renderer_resource_create_args args;
> +    struct virgl_gpu_resource *vres;
>
>      VIRTIO_GPU_FILL_CMD(c3d);
>      trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
>                                         c3d.width, c3d.height, c3d.depth);
>

same

> +    vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres->res.width = c3d.width;
> +    vres->res.height = c3d.height;
> +    vres->res.format = c3d.format;
> +    vres->res.resource_id = c3d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
> +
>      args.handle = c3d.resource_id;
>      args.target = c3d.target;
>      args.format = c3d.format;
> @@ -82,19 +115,23 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>                                       struct virtio_gpu_ctrl_command *cmd)
>  {
>      struct virtio_gpu_resource_unref unref;
> -    struct iovec *res_iovs = NULL;
> -    int num_iovs = 0;
> +    struct virgl_gpu_resource *vres;
>
>      VIRTIO_GPU_FILL_CMD(unref);
>      trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>
> -    virgl_renderer_resource_detach_iov(unref.resource_id,
> -                                       &res_iovs,
> -                                       &num_iovs);
> -    if (res_iovs != NULL && num_iovs != 0) {
> -        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> +    vres = virgl_gpu_find_resource(g, unref.resource_id);
> +    if (!vres) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
>      }
> +
> +    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
>      virgl_renderer_resource_unref(unref.resource_id);
> +
> +    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> +    virtio_gpu_cleanup_mapping(g, &vres->res);
> +    g_free(vres);
>  }
>
>  static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -310,44 +347,51 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
>                                            struct virtio_gpu_ctrl_command *cmd)
>  {
>      struct virtio_gpu_resource_attach_backing att_rb;
> -    struct iovec *res_iovs;
> -    uint32_t res_niov;
> +    struct virgl_gpu_resource *vres;
>      int ret;
>
>      VIRTIO_GPU_FILL_CMD(att_rb);
>      trace_virtio_gpu_cmd_res_back_attach(att_rb.resource_id);
>
> +    vres = virgl_gpu_find_resource(g, att_rb.resource_id);
> +    if (!vres) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
>      ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, sizeof(att_rb),
> -                                        cmd, NULL, &res_iovs, &res_niov);
> +                                        cmd, NULL, &vres->res.iov,
> +                                        &vres->res.iov_cnt);
>      if (ret != 0) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }
>
>      ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> -                                             res_iovs, res_niov);
> +                                             vres->res.iov, vres->res.iov_cnt);
>
> -    if (ret != 0)
> -        virtio_gpu_cleanup_mapping_iov(g, res_iovs, res_niov);
> +    if (ret != 0) {
> +        virtio_gpu_cleanup_mapping(g, &vres->res);
> +    }
>  }
>
>  static void virgl_resource_detach_backing(VirtIOGPU *g,
>                                            struct virtio_gpu_ctrl_command *cmd)
>  {
>      struct virtio_gpu_resource_detach_backing detach_rb;
> -    struct iovec *res_iovs = NULL;
> -    int num_iovs = 0;
> +    struct virgl_gpu_resource *vres;
>
>      VIRTIO_GPU_FILL_CMD(detach_rb);
>      trace_virtio_gpu_cmd_res_back_detach(detach_rb.resource_id);
>
> -    virgl_renderer_resource_detach_iov(detach_rb.resource_id,
> -                                       &res_iovs,
> -                                       &num_iovs);
> -    if (res_iovs == NULL || num_iovs == 0) {
> +    vres = virgl_gpu_find_resource(g, detach_rb.resource_id);
> +    if (!vres) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>          return;
>      }
> -    virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> +
> +    virgl_renderer_resource_detach_iov(detach_rb.resource_id, NULL, NULL);
> +    virtio_gpu_cleanup_mapping(g, &vres->res);
>  }
>
>
> --
> 2.25.1
>
>


-- 
Marc-André Lureau

Re: [PATCH v6 05/11] virtio-gpu: Introduce virgl_gpu_resource structure

[Huang Rui]

On Tue, Jan 02, 2024 at 07:52:04PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
> >
> > Introduce a new virgl_gpu_resource data structure and helper functions
> > for virgl. It's used to add new member which is specific for virgl in
> > following patches of blob memory support.
> >
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > New patch:
> > - Introduce new struct virgl_gpu_resource to store virgl specific members.
> > - Move resource initialization from path "virtio-gpu: Resource UUID" here.
> > - Remove error handling of g_new0, because glib will abort() on OOM.
> > - Set iov and iov_cnt in struct virtio_gpu_simple_resource for all types
> >   of resources.
> >
> >  hw/display/virtio-gpu-virgl.c | 84 ++++++++++++++++++++++++++---------
> >  1 file changed, 64 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 5bbc8071b2..faab374336 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -22,6 +22,23 @@
> >
> >  #include <virglrenderer.h>
> >
> > +struct virgl_gpu_resource {
> > +    struct virtio_gpu_simple_resource res;
> > +};
> > +
> > +static struct virgl_gpu_resource *
> > +virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +
> > +    res = virtio_gpu_find_resource(g, resource_id);
> > +    if (!res) {
> > +        return NULL;
> > +    }
> > +
> > +    return container_of(res, struct virgl_gpu_resource, res);
> > +}
> > +
> >  #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
> >  static void *
> >  virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
> > @@ -35,11 +52,19 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >  {
> >      struct virtio_gpu_resource_create_2d c2d;
> >      struct virgl_renderer_resource_create_args args;
> > +    struct virgl_gpu_resource *vres;
> >
> >      VIRTIO_GPU_FILL_CMD(c2d);
> >      trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
> >                                         c2d.width, c2d.height);
> >
> 
> It should check the resource doesn't already exist (similar to 2d code)
> 

Will add the resource check here in V7.

Thanks,
Ray

> > +    vres = g_new0(struct virgl_gpu_resource, 1);
> > +    vres->res.width = c2d.width;
> > +    vres->res.height = c2d.height;
> > +    vres->res.format = c2d.format;
> > +    vres->res.resource_id = c2d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
> > +
> >      args.handle = c2d.resource_id;
> >      args.target = 2;
> >      args.format = c2d.format;
> > @@ -59,11 +84,19 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >  {
> >      struct virtio_gpu_resource_create_3d c3d;
> >      struct virgl_renderer_resource_create_args args;
> > +    struct virgl_gpu_resource *vres;
> >
> >      VIRTIO_GPU_FILL_CMD(c3d);
> >      trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
> >                                         c3d.width, c3d.height, c3d.depth);
> >
> 
> same
> 
> > +    vres = g_new0(struct virgl_gpu_resource, 1);
> > +    vres->res.width = c3d.width;
> > +    vres->res.height = c3d.height;
> > +    vres->res.format = c3d.format;
> > +    vres->res.resource_id = c3d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->res, next);
> > +
> >      args.handle = c3d.resource_id;
> >      args.target = c3d.target;
> >      args.format = c3d.format;
> > @@ -82,19 +115,23 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >                                       struct virtio_gpu_ctrl_command *cmd)
> >  {
> >      struct virtio_gpu_resource_unref unref;
> > -    struct iovec *res_iovs = NULL;
> > -    int num_iovs = 0;
> > +    struct virgl_gpu_resource *vres;
> >
> >      VIRTIO_GPU_FILL_CMD(unref);
> >      trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >
> > -    virgl_renderer_resource_detach_iov(unref.resource_id,
> > -                                       &res_iovs,
> > -                                       &num_iovs);
> > -    if (res_iovs != NULL && num_iovs != 0) {
> > -        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> > +    vres = virgl_gpu_find_resource(g, unref.resource_id);
> > +    if (!vres) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> >      }
> > +
> > +    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> >      virgl_renderer_resource_unref(unref.resource_id);
> > +
> > +    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> > +    virtio_gpu_cleanup_mapping(g, &vres->res);
> > +    g_free(vres);
> >  }
> >
> >  static void virgl_cmd_context_create(VirtIOGPU *g,
> > @@ -310,44 +347,51 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
> >                                            struct virtio_gpu_ctrl_command *cmd)
> >  {
> >      struct virtio_gpu_resource_attach_backing att_rb;
> > -    struct iovec *res_iovs;
> > -    uint32_t res_niov;
> > +    struct virgl_gpu_resource *vres;
> >      int ret;
> >
> >      VIRTIO_GPU_FILL_CMD(att_rb);
> >      trace_virtio_gpu_cmd_res_back_attach(att_rb.resource_id);
> >
> > +    vres = virgl_gpu_find_resource(g, att_rb.resource_id);
> > +    if (!vres) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> >      ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, sizeof(att_rb),
> > -                                        cmd, NULL, &res_iovs, &res_niov);
> > +                                        cmd, NULL, &vres->res.iov,
> > +                                        &vres->res.iov_cnt);
> >      if (ret != 0) {
> >          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >          return;
> >      }
> >
> >      ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> > -                                             res_iovs, res_niov);
> > +                                             vres->res.iov, vres->res.iov_cnt);
> >
> > -    if (ret != 0)
> > -        virtio_gpu_cleanup_mapping_iov(g, res_iovs, res_niov);
> > +    if (ret != 0) {
> > +        virtio_gpu_cleanup_mapping(g, &vres->res);
> > +    }
> >  }
> >
> >  static void virgl_resource_detach_backing(VirtIOGPU *g,
> >                                            struct virtio_gpu_ctrl_command *cmd)
> >  {
> >      struct virtio_gpu_resource_detach_backing detach_rb;
> > -    struct iovec *res_iovs = NULL;
> > -    int num_iovs = 0;
> > +    struct virgl_gpu_resource *vres;
> >
> >      VIRTIO_GPU_FILL_CMD(detach_rb);
> >      trace_virtio_gpu_cmd_res_back_detach(detach_rb.resource_id);
> >
> > -    virgl_renderer_resource_detach_iov(detach_rb.resource_id,
> > -                                       &res_iovs,
> > -                                       &num_iovs);
> > -    if (res_iovs == NULL || num_iovs == 0) {
> > +    vres = virgl_gpu_find_resource(g, detach_rb.resource_id);
> > +    if (!vres) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> >          return;
> >      }
> > -    virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> > +
> > +    virgl_renderer_resource_detach_iov(detach_rb.resource_id, NULL, NULL);
> > +    virtio_gpu_cleanup_mapping(g, &vres->res);
> >  }
> >
> >
> > --
> > 2.25.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

[PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

[Huang Rui]

From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>

When the memory region has a different life-cycle from that of her parent,
could be automatically released, once has been unparent and once all of her
references have gone away, via the object's free callback.

However, currently, the address space subsystem keeps references to the
memory region without first incrementing its object's reference count.
As a result, the automatic deallocation of the object, not taking into
account those references, results in use-after-free memory corruption.

More specifically, reference to the memory region is kept in flatview
ranges. If the reference count of the memory region is not incremented,
flatview_destroy(), that is asynchronous, may be called after memory
region's destruction. If the reference count of the memory region is
incremented, memory region's destruction will take place after
flatview_destroy() has released its references.

This patch increases the reference count of an owned memory region object
on each memory_region_ref() and decreases it on each memory_region_unref().

Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- remove in-code comment because it is confusing and explain the issue,
  that the patch attempts to fix, with more details in commit message

 system/memory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/system/memory.c b/system/memory.c
index 304fa843ea..4d5e7e7a4c 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1824,6 +1824,7 @@ void memory_region_ref(MemoryRegion *mr)
      * we do not ref/unref them because it slows down DMA sensibly.
      */
     if (mr && mr->owner) {
+        object_ref(OBJECT(mr));
         object_ref(mr->owner);
     }
 }
@@ -1832,6 +1833,7 @@ void memory_region_unref(MemoryRegion *mr)
 {
     if (mr && mr->owner) {
         object_unref(mr->owner);
+        object_unref(OBJECT(mr));
     }
 }
 
-- 
2.25.1

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> When the memory region has a different life-cycle from that of her parent,
> could be automatically released, once has been unparent and once all of her
> references have gone away, via the object's free callback.
> 
> However, currently, the address space subsystem keeps references to the
> memory region without first incrementing its object's reference count.
> As a result, the automatic deallocation of the object, not taking into
> account those references, results in use-after-free memory corruption.
> 
> More specifically, reference to the memory region is kept in flatview
> ranges. If the reference count of the memory region is not incremented,
> flatview_destroy(), that is asynchronous, may be called after memory
> region's destruction. If the reference count of the memory region is
> incremented, memory region's destruction will take place after
> flatview_destroy() has released its references.
> 
> This patch increases the reference count of an owned memory region object
> on each memory_region_ref() and decreases it on each memory_region_unref().

Why not pass the memory region itself as the owner parameter of 
memory_region_init_ram_ptr()?

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

[Xenia Ragiadakou]

On 21/12/23 07:45, Akihiko Odaki wrote:
> On 2023/12/19 16:53, Huang Rui wrote:
>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>
>> When the memory region has a different life-cycle from that of her 
>> parent,
>> could be automatically released, once has been unparent and once all 
>> of her
>> references have gone away, via the object's free callback.
>>
>> However, currently, the address space subsystem keeps references to the
>> memory region without first incrementing its object's reference count.
>> As a result, the automatic deallocation of the object, not taking into
>> account those references, results in use-after-free memory corruption.
>>
>> More specifically, reference to the memory region is kept in flatview
>> ranges. If the reference count of the memory region is not incremented,
>> flatview_destroy(), that is asynchronous, may be called after memory
>> region's destruction. If the reference count of the memory region is
>> incremented, memory region's destruction will take place after
>> flatview_destroy() has released its references.
>>
>> This patch increases the reference count of an owned memory region object
>> on each memory_region_ref() and decreases it on each 
>> memory_region_unref().
> 
> Why not pass the memory region itself as the owner parameter of 
> memory_region_init_ram_ptr()?

Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't 
disappear while the memory region is still in use?

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/12/21 16:35, Xenia Ragiadakou wrote:
> 
> On 21/12/23 07:45, Akihiko Odaki wrote:
>> On 2023/12/19 16:53, Huang Rui wrote:
>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>
>>> When the memory region has a different life-cycle from that of her 
>>> parent,
>>> could be automatically released, once has been unparent and once all 
>>> of her
>>> references have gone away, via the object's free callback.
>>>
>>> However, currently, the address space subsystem keeps references to the
>>> memory region without first incrementing its object's reference count.
>>> As a result, the automatic deallocation of the object, not taking into
>>> account those references, results in use-after-free memory corruption.
>>>
>>> More specifically, reference to the memory region is kept in flatview
>>> ranges. If the reference count of the memory region is not incremented,
>>> flatview_destroy(), that is asynchronous, may be called after memory
>>> region's destruction. If the reference count of the memory region is
>>> incremented, memory region's destruction will take place after
>>> flatview_destroy() has released its references.
>>>
>>> This patch increases the reference count of an owned memory region 
>>> object
>>> on each memory_region_ref() and decreases it on each 
>>> memory_region_unref().
>>
>> Why not pass the memory region itself as the owner parameter of 
>> memory_region_init_ram_ptr()?
> 
> Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't 
> disappear while the memory region is still in use?

You can object_ref() when you do memory_region_init_ram_ptr() and 
object_unref() when the memory region is being destroyed.

Re: [PATCH v6 06/11] softmmu/memory: enable automatic deallocation of memory regions

[Xenia Ragiadakou]

On 21/12/23 09:50, Akihiko Odaki wrote:
> On 2023/12/21 16:35, Xenia Ragiadakou wrote:
>>
>> On 21/12/23 07:45, Akihiko Odaki wrote:
>>> On 2023/12/19 16:53, Huang Rui wrote:
>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>
>>>> When the memory region has a different life-cycle from that of her 
>>>> parent,
>>>> could be automatically released, once has been unparent and once all 
>>>> of her
>>>> references have gone away, via the object's free callback.
>>>>
>>>> However, currently, the address space subsystem keeps references to the
>>>> memory region without first incrementing its object's reference count.
>>>> As a result, the automatic deallocation of the object, not taking into
>>>> account those references, results in use-after-free memory corruption.
>>>>
>>>> More specifically, reference to the memory region is kept in flatview
>>>> ranges. If the reference count of the memory region is not incremented,
>>>> flatview_destroy(), that is asynchronous, may be called after memory
>>>> region's destruction. If the reference count of the memory region is
>>>> incremented, memory region's destruction will take place after
>>>> flatview_destroy() has released its references.
>>>>
>>>> This patch increases the reference count of an owned memory region 
>>>> object
>>>> on each memory_region_ref() and decreases it on each 
>>>> memory_region_unref().
>>>
>>> Why not pass the memory region itself as the owner parameter of 
>>> memory_region_init_ram_ptr()?
>>
>> Hmm, in that case, how will it be guaranteed that the VirtIOGPU won't 
>> disappear while the memory region is still in use?
> 
> You can object_ref() when you do memory_region_init_ram_ptr() and 
> object_unref() when the memory region is being destroyed.

It is not very intuitive but I see your point. This change is quite 
intrusive and has little use. I think it can be worked around in the way 
you suggest.

[PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Huang Rui]

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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Use new struct virgl_gpu_resource.
- Unmap, unref and destroy the resource only after the memory region
  has been completely removed.
- In unref check whether the resource is still mapped.
- In unmap_blob check whether the resource has been already unmapped.
- Fix coding style

 hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
 hw/display/virtio-gpu.c       |   4 +-
 meson.build                   |   4 +
 3 files changed, 276 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index faab374336..5a3a292f79 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
 #include "trace.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
 
 #include "ui/egl-helpers.h"
 
@@ -24,8 +25,62 @@
 
 struct virgl_gpu_resource {
     struct virtio_gpu_simple_resource res;
+    uint32_t ref;
+    VirtIOGPU *g;
+
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    /* only blob resource needs this region to be mapped as guest mmio */
+    MemoryRegion *region;
+#endif
 };
 
+static void vres_get_ref(struct virgl_gpu_resource *vres)
+{
+    uint32_t ref;
+
+    ref = qatomic_fetch_inc(&vres->ref);
+    g_assert(ref < INT_MAX);
+}
+
+static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+    VirtIOGPU *g;
+
+    if (!vres) {
+        return;
+    }
+
+    g = vres->g;
+    res = &vres->res;
+    QTAILQ_REMOVE(&g->reslist, res, next);
+    virtio_gpu_cleanup_mapping(g, res);
+    g_free(vres);
+}
+
+static void virgl_resource_unref(struct virgl_gpu_resource *vres)
+{
+    struct virtio_gpu_simple_resource *res;
+
+    if (!vres) {
+        return;
+    }
+
+    res = &vres->res;
+    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
+    virgl_renderer_resource_unref(res->resource_id);
+}
+
+static void vres_put_ref(struct virgl_gpu_resource *vres)
+{
+    g_assert(vres->ref > 0);
+
+    if (qatomic_fetch_dec(&vres->ref) == 1) {
+        virgl_resource_unref(vres);
+        virgl_resource_destroy(vres);
+    }
+}
+
 static struct virgl_gpu_resource *
 virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
 {
@@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
                                        c2d.width, c2d.height);
 
     vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
     vres->res.width = c2d.width;
     vres->res.height = c2d.height;
     vres->res.format = c2d.format;
@@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
                                        c3d.width, c3d.height, c3d.depth);
 
     vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
     vres->res.width = c3d.width;
     vres->res.height = c3d.height;
     vres->res.format = c3d.format;
@@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
         return;
     }
 
-    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
-    virgl_renderer_resource_unref(unref.resource_id);
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    if (vres->region) {
+        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+        MemoryRegion *mr = vres->region;
+
+        warn_report("%s: blob resource %d not unmapped",
+                    __func__, unref.resource_id);
+        vres->region = NULL;
+        memory_region_set_enabled(mr, false);
+        memory_region_del_subregion(&b->hostmem, mr);
+        object_unparent(OBJECT(mr));
+    }
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
 
-    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
-    virtio_gpu_cleanup_mapping(g, &vres->res);
-    g_free(vres);
+    vres_put_ref(vres);
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
     g_free(resp);
 }
 
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+
+static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
+{
+    if (!vres) {
+        return;
+    }
+
+    virgl_renderer_resource_unmap(vres->res.resource_id);
+
+    vres_put_ref(vres);
+}
+
+static void virgl_resource_blob_async_unmap(void *obj)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    struct virgl_gpu_resource *vres = mr->opaque;
+
+    virgl_resource_unmap(vres);
+
+    g_free(obj);
+}
+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+                                           struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virgl_gpu_resource *vres;
+    struct virtio_gpu_resource_create_blob cblob;
+    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
+    int ret;
+
+    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;
+    }
+
+    vres = virgl_gpu_find_resource(g, cblob.resource_id);
+    if (vres) {
+        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;
+    }
+
+    vres = g_new0(struct virgl_gpu_resource, 1);
+    vres_get_ref(vres);
+    vres->g = g;
+    vres->res.resource_id = cblob.resource_id;
+    vres->res.blob_size = cblob.size;
+
+    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
+        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
+                                            cmd, &vres->res.addrs,
+                                            &vres->res.iov, &vres->res.iov_cnt);
+        if (!ret) {
+            g_free(vres);
+            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+            return;
+        }
+    }
+
+    QTAILQ_INSERT_HEAD(&g->reslist, &vres->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 = vres->res.iov;
+    virgl_args.num_iovs = vres->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 virgl_gpu_resource *vres;
+    struct virtio_gpu_resource_map_blob mblob;
+    int ret;
+    void *data;
+    uint64_t size;
+    struct virtio_gpu_resp_map_info resp;
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+
+    VIRTIO_GPU_FILL_CMD(mblob);
+    virtio_gpu_map_blob_bswap(&mblob);
+
+    if (mblob.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;
+    }
+
+    vres = virgl_gpu_find_resource(g, mblob.resource_id);
+    if (!vres) {
+        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 (vres->region) {
+        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 = virgl_renderer_resource_map(vres->res.resource_id, &data, &size);
+    if (ret) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
+                      __func__, strerror(-ret));
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    vres_get_ref(vres);
+    vres->region = g_new0(MemoryRegion, 1);
+    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
+    vres->region->opaque = vres;
+    OBJECT(vres->region)->free = virgl_resource_blob_async_unmap;
+    memory_region_add_subregion(&b->hostmem, mblob.offset, vres->region);
+    memory_region_set_enabled(vres->region, true);
+
+    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 virgl_gpu_resource *vres;
+    struct virtio_gpu_resource_unmap_blob ublob;
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+    MemoryRegion *mr;
+
+    VIRTIO_GPU_FILL_CMD(ublob);
+    virtio_gpu_unmap_blob_bswap(&ublob);
+
+    if (ublob.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;
+    }
+
+    vres = virgl_gpu_find_resource(g, ublob.resource_id);
+    if (!vres) {
+        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 (!vres->region) {
+        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;
+    }
+
+    mr = vres->region;
+    vres->region = NULL;
+    memory_region_set_enabled(mr, false);
+    memory_region_del_subregion(&b->hostmem, mr);
+    object_unparent(OBJECT(mr));
+}
+
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
+
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                       struct virtio_gpu_ctrl_command *cmd)
 {
@@ -536,6 +789,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     case VIRTIO_GPU_CMD_GET_EDID:
         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;
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         break;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4c3ec9d0ea..8189c392dc 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1449,10 +1449,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, "Linked virglrenderer does not support blob resources");
             return;
         }
+#endif
     }
 
     if (!virtio_gpu_base_device_realize(qdev,
diff --git a/meson.build b/meson.build
index ea52ef1b9c..629407128e 100644
--- a/meson.build
+++ b/meson.build
@@ -1054,6 +1054,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
                          cc.has_function('virgl_renderer_context_create_with_flags',
                                          prefix: '#include <virglrenderer.h>',
                                          dependencies: virgl))
+    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
+                         cc.has_function('virgl_renderer_resource_create_blob',
+                                         prefix: '#include <virglrenderer.h>',
+                                         dependencies: virgl))
   endif
 endif
 rutabaga = not_found
-- 
2.25.1

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Use new struct virgl_gpu_resource.
> - Unmap, unref and destroy the resource only after the memory region
>    has been completely removed.
> - In unref check whether the resource is still mapped.
> - In unmap_blob check whether the resource has been already unmapped.
> - Fix coding style
> 
>   hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
>   hw/display/virtio-gpu.c       |   4 +-
>   meson.build                   |   4 +
>   3 files changed, 276 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index faab374336..5a3a292f79 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -17,6 +17,7 @@
>   #include "trace.h"
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-gpu.h"
> +#include "hw/virtio/virtio-gpu-bswap.h"
>   
>   #include "ui/egl-helpers.h"
>   
> @@ -24,8 +25,62 @@
>   
>   struct virgl_gpu_resource {
>       struct virtio_gpu_simple_resource res;
> +    uint32_t ref;
> +    VirtIOGPU *g;
> +
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    /* only blob resource needs this region to be mapped as guest mmio */
> +    MemoryRegion *region;

Why not just embed MemoryRegion into struct virgl_gpu_resource instead 
of having a pointer?

> +#endif
>   };
>   
> +static void vres_get_ref(struct virgl_gpu_resource *vres)
> +{
> +    uint32_t ref;
> +
> +    ref = qatomic_fetch_inc(&vres->ref);
> +    g_assert(ref < INT_MAX);
> +}
> +
> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    VirtIOGPU *g;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    g = vres->g;
> +    res = &vres->res;
> +    QTAILQ_REMOVE(&g->reslist, res, next);
> +    virtio_gpu_cleanup_mapping(g, res);
> +    g_free(vres);
> +}
> +
> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    res = &vres->res;
> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
> +    virgl_renderer_resource_unref(res->resource_id);
> +}
> +
> +static void vres_put_ref(struct virgl_gpu_resource *vres)
> +{
> +    g_assert(vres->ref > 0);
> +
> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
> +        virgl_resource_unref(vres);
> +        virgl_resource_destroy(vres);
> +    }
> +}
> +
>   static struct virgl_gpu_resource *
>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
>   {
> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>                                          c2d.width, c2d.height);
>   
>       vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>       vres->res.width = c2d.width;
>       vres->res.height = c2d.height;
>       vres->res.format = c2d.format;
> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>                                          c3d.width, c3d.height, c3d.depth);
>   
>       vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>       vres->res.width = c3d.width;
>       vres->res.height = c3d.height;
>       vres->res.format = c3d.format;
> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>           return;
>       }
>   
> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> -    virgl_renderer_resource_unref(unref.resource_id);
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    if (vres->region) {
> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +        MemoryRegion *mr = vres->region;
> +
> +        warn_report("%s: blob resource %d not unmapped",
> +                    __func__, unref.resource_id);
> +        vres->region = NULL;
> +        memory_region_set_enabled(mr, false);
> +        memory_region_del_subregion(&b->hostmem, mr);
> +        object_unparent(OBJECT(mr));
> +    }
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>   
> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> -    virtio_gpu_cleanup_mapping(g, &vres->res);
> -    g_free(vres);
> +    vres_put_ref(vres);

What will happen if the guest consecutively requests 
VIRTIO_GPU_CMD_RESOURCE_UNREF twice for a mapped resource?

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Xenia Ragiadakou]

On 21/12/23 07:57, Akihiko Odaki wrote:
> On 2023/12/19 16:53, Huang Rui 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>
>> Changes in v6:
>> - Use new struct virgl_gpu_resource.
>> - Unmap, unref and destroy the resource only after the memory region
>>    has been completely removed.
>> - In unref check whether the resource is still mapped.
>> - In unmap_blob check whether the resource has been already unmapped.
>> - Fix coding style
>>
>>   hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
>>   hw/display/virtio-gpu.c       |   4 +-
>>   meson.build                   |   4 +
>>   3 files changed, 276 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c 
>> b/hw/display/virtio-gpu-virgl.c
>> index faab374336..5a3a292f79 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -17,6 +17,7 @@
>>   #include "trace.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "hw/virtio/virtio-gpu.h"
>> +#include "hw/virtio/virtio-gpu-bswap.h"
>>   #include "ui/egl-helpers.h"
>> @@ -24,8 +25,62 @@
>>   struct virgl_gpu_resource {
>>       struct virtio_gpu_simple_resource res;
>> +    uint32_t ref;
>> +    VirtIOGPU *g;
>> +
>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>> +    /* only blob resource needs this region to be mapped as guest 
>> mmio */
>> +    MemoryRegion *region;
> 
> Why not just embed MemoryRegion into struct virgl_gpu_resource instead 
> of having a pointer?

To decouple memory region from the resource. Since there are different 
commands for creating and mapping maybe there is the intention to map 
the same resource multiple times. If the lifecycle of the resource is 
tightly coupled to the lifecycle of the memory region then the memory 
region could be embedded into the struct. Ray could give more insight.

> 
>> +#endif
>>   };
>> +static void vres_get_ref(struct virgl_gpu_resource *vres)
>> +{
>> +    uint32_t ref;
>> +
>> +    ref = qatomic_fetch_inc(&vres->ref);
>> +    g_assert(ref < INT_MAX);
>> +}
>> +
>> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
>> +{
>> +    struct virtio_gpu_simple_resource *res;
>> +    VirtIOGPU *g;
>> +
>> +    if (!vres) {
>> +        return;
>> +    }
>> +
>> +    g = vres->g;
>> +    res = &vres->res;
>> +    QTAILQ_REMOVE(&g->reslist, res, next);
>> +    virtio_gpu_cleanup_mapping(g, res);
>> +    g_free(vres);
>> +}
>> +
>> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
>> +{
>> +    struct virtio_gpu_simple_resource *res;
>> +
>> +    if (!vres) {
>> +        return;
>> +    }
>> +
>> +    res = &vres->res;
>> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
>> +    virgl_renderer_resource_unref(res->resource_id);
>> +}
>> +
>> +static void vres_put_ref(struct virgl_gpu_resource *vres)
>> +{
>> +    g_assert(vres->ref > 0);
>> +
>> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
>> +        virgl_resource_unref(vres);
>> +        virgl_resource_destroy(vres);
>> +    }
>> +}
>> +
>>   static struct virgl_gpu_resource *
>>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
>>   {
>> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>>                                          c2d.width, c2d.height);
>>       vres = g_new0(struct virgl_gpu_resource, 1);
>> +    vres_get_ref(vres);
>> +    vres->g = g;
>>       vres->res.width = c2d.width;
>>       vres->res.height = c2d.height;
>>       vres->res.format = c2d.format;
>> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>>                                          c3d.width, c3d.height, 
>> c3d.depth);
>>       vres = g_new0(struct virgl_gpu_resource, 1);
>> +    vres_get_ref(vres);
>> +    vres->g = g;
>>       vres->res.width = c3d.width;
>>       vres->res.height = c3d.height;
>>       vres->res.format = c3d.format;
>> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>           return;
>>       }
>> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
>> -    virgl_renderer_resource_unref(unref.resource_id);
>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>> +    if (vres->region) {
>> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +        MemoryRegion *mr = vres->region;
>> +
>> +        warn_report("%s: blob resource %d not unmapped",
>> +                    __func__, unref.resource_id);
>> +        vres->region = NULL;
>> +        memory_region_set_enabled(mr, false);
>> +        memory_region_del_subregion(&b->hostmem, mr);
>> +        object_unparent(OBJECT(mr));
>> +    }
>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
>> -    virtio_gpu_cleanup_mapping(g, &vres->res);
>> -    g_free(vres);
>> +    vres_put_ref(vres);
> 
> What will happen if the guest consecutively requests 
> VIRTIO_GPU_CMD_RESOURCE_UNREF twice for a mapped resource?

You are right. I think the resource needs to be removed from the list 
synchronously on first unref.

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui 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

I have another concern about delaying virgl_renderer_resource_unref() 
until the resource gets unmapped; the guest will expect the resource ID 
will be available for a new resource immediately after 
VIRTIO_GPU_CMD_RESOURCE_UNREF, but it will break the assumption and may 
corrupt things.

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Pierre-Eric Pelloux-Prayer]

Le 21/12/2023 à 09:09, Akihiko Odaki a écrit :
> On 2023/12/19 16:53, Huang Rui 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
> 
> I have another concern about delaying virgl_renderer_resource_unref() until the resource gets unmapped; the guest will expect the resource ID will be available for a new resource immediately after VIRTIO_GPU_CMD_RESOURCE_UNREF, but it will break the assumption and may corrupt things.
> 

Yes this is a problem.

And another one is virglrenderer is not really thread-safe, so this callstack:

#0  virgl_resource_blob_async_unmap ()
#1  object_finalize ()
#2  object_unref ()
#3  memory_region_unref ()
#4  flatview_destroy ()
#5  call_rcu_thread ()
#6  qemu_thread_start ()

Will call into virgl_renderer_ctx_resource_unmap which in turn uses virgl_resource_lookup
without any multithreading considerations.


Regards,
Pierre-Eric

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> Changes in v6:
> - Use new struct virgl_gpu_resource.
> - Unmap, unref and destroy the resource only after the memory region
>   has been completely removed.
> - In unref check whether the resource is still mapped.
> - In unmap_blob check whether the resource has been already unmapped.
> - Fix coding style
>
>  hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
>  hw/display/virtio-gpu.c       |   4 +-
>  meson.build                   |   4 +
>  3 files changed, 276 insertions(+), 6 deletions(-)

Could you split this patch to introduce the new resource
ref/get/put/destroy functions first, before adding the blob commands.
Please explain the rationale of the changes, why they are safe or
equivalent to current code. I'd also suggest documentation and better
naming for the functions, or inlined code as appropriate, as it's
confusing to understand together what should be used and when.

>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index faab374336..5a3a292f79 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -17,6 +17,7 @@
>  #include "trace.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-gpu.h"
> +#include "hw/virtio/virtio-gpu-bswap.h"
>
>  #include "ui/egl-helpers.h"
>
> @@ -24,8 +25,62 @@
>
>  struct virgl_gpu_resource {
>      struct virtio_gpu_simple_resource res;
> +    uint32_t ref;
> +    VirtIOGPU *g;
> +
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    /* only blob resource needs this region to be mapped as guest mmio */
> +    MemoryRegion *region;
> +#endif
>  };
>
> +static void vres_get_ref(struct virgl_gpu_resource *vres)
> +{
> +    uint32_t ref;
> +
> +    ref = qatomic_fetch_inc(&vres->ref);
> +    g_assert(ref < INT_MAX);
> +}
> +
> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    VirtIOGPU *g;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    g = vres->g;
> +    res = &vres->res;
> +    QTAILQ_REMOVE(&g->reslist, res, next);
> +    virtio_gpu_cleanup_mapping(g, res);
> +    g_free(vres);
> +}
> +
> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    res = &vres->res;
> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
> +    virgl_renderer_resource_unref(res->resource_id);
> +}
> +
> +static void vres_put_ref(struct virgl_gpu_resource *vres)
> +{
> +    g_assert(vres->ref > 0);
> +
> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
> +        virgl_resource_unref(vres);
> +        virgl_resource_destroy(vres);
> +    }
> +}
> +
>  static struct virgl_gpu_resource *
>  virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
>  {
> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>                                         c2d.width, c2d.height);
>
>      vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>      vres->res.width = c2d.width;
>      vres->res.height = c2d.height;
>      vres->res.format = c2d.format;
> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>                                         c3d.width, c3d.height, c3d.depth);
>
>      vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>      vres->res.width = c3d.width;
>      vres->res.height = c3d.height;
>      vres->res.format = c3d.format;
> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>          return;
>      }
>
> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> -    virgl_renderer_resource_unref(unref.resource_id);
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    if (vres->region) {
> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +        MemoryRegion *mr = vres->region;
> +
> +        warn_report("%s: blob resource %d not unmapped",
> +                    __func__, unref.resource_id);
> +        vres->region = NULL;
> +        memory_region_set_enabled(mr, false);
> +        memory_region_del_subregion(&b->hostmem, mr);
> +        object_unparent(OBJECT(mr));
> +    }
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>
> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> -    virtio_gpu_cleanup_mapping(g, &vres->res);
> -    g_free(vres);
> +    vres_put_ref(vres);
>  }
>
>  static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>      g_free(resp);
>  }
>
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +
> +static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
> +{
> +    if (!vres) {
> +        return;
> +    }
> +
> +    virgl_renderer_resource_unmap(vres->res.resource_id);
> +
> +    vres_put_ref(vres);
> +}
> +
> +static void virgl_resource_blob_async_unmap(void *obj)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    struct virgl_gpu_resource *vres = mr->opaque;
> +
> +    virgl_resource_unmap(vres);
> +
> +    g_free(obj);
> +}
> +
> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> +                                           struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_create_blob cblob;
> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> +    int ret;
> +
> +    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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, cblob.resource_id);
> +    if (vres) {
> +        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;
> +    }
> +
> +    vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
> +    vres->res.resource_id = cblob.resource_id;
> +    vres->res.blob_size = cblob.size;
> +
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> +                                            cmd, &vres->res.addrs,
> +                                            &vres->res.iov, &vres->res.iov_cnt);
> +        if (!ret) {
> +            g_free(vres);
> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +            return;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->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 = vres->res.iov;
> +    virgl_args.num_iovs = vres->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 virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_map_blob mblob;
> +    int ret;
> +    void *data;
> +    uint64_t size;
> +    struct virtio_gpu_resp_map_info resp;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    VIRTIO_GPU_FILL_CMD(mblob);
> +    virtio_gpu_map_blob_bswap(&mblob);
> +
> +    if (mblob.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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, mblob.resource_id);
> +    if (!vres) {
> +        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 (vres->region) {
> +        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 = virgl_renderer_resource_map(vres->res.resource_id, &data, &size);
> +    if (ret) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    vres_get_ref(vres);
> +    vres->region = g_new0(MemoryRegion, 1);
> +    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
> +    vres->region->opaque = vres;
> +    OBJECT(vres->region)->free = virgl_resource_blob_async_unmap;
> +    memory_region_add_subregion(&b->hostmem, mblob.offset, vres->region);
> +    memory_region_set_enabled(vres->region, true);
> +
> +    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 virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_unmap_blob ublob;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +    MemoryRegion *mr;
> +
> +    VIRTIO_GPU_FILL_CMD(ublob);
> +    virtio_gpu_unmap_blob_bswap(&ublob);
> +
> +    if (ublob.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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, ublob.resource_id);
> +    if (!vres) {
> +        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 (!vres->region) {
> +        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;
> +    }
> +
> +    mr = vres->region;
> +    vres->region = NULL;
> +    memory_region_set_enabled(mr, false);
> +    memory_region_del_subregion(&b->hostmem, mr);
> +    object_unparent(OBJECT(mr));
> +}
> +
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> +
>  void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                        struct virtio_gpu_ctrl_command *cmd)
>  {
> @@ -536,6 +789,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_GET_EDID:
>          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;
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4c3ec9d0ea..8189c392dc 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1449,10 +1449,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, "Linked virglrenderer does not support blob resources");
>              return;
>          }
> +#endif
>      }
>
>      if (!virtio_gpu_base_device_realize(qdev,
> diff --git a/meson.build b/meson.build
> index ea52ef1b9c..629407128e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1054,6 +1054,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                           cc.has_function('virgl_renderer_context_create_with_flags',
>                                           prefix: '#include <virglrenderer.h>',
>                                           dependencies: virgl))
> +    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
> +                         cc.has_function('virgl_renderer_resource_create_blob',
> +                                         prefix: '#include <virglrenderer.h>',
> +                                         dependencies: virgl))
>    endif
>  endif
>  rutabaga = not_found
> --
> 2.25.1
>


-- 
Marc-André Lureau

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Pierre-Eric Pelloux-Prayer]

Le 19/12/2023 à 08:53, Huang Rui a écrit :
> 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Use new struct virgl_gpu_resource.
> - Unmap, unref and destroy the resource only after the memory region
>    has been completely removed.
> - In unref check whether the resource is still mapped.
> - In unmap_blob check whether the resource has been already unmapped.
> - Fix coding style
> 
>   hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
>   hw/display/virtio-gpu.c       |   4 +-
>   meson.build                   |   4 +
>   3 files changed, 276 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index faab374336..5a3a292f79 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -17,6 +17,7 @@
>   #include "trace.h"
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-gpu.h"
> +#include "hw/virtio/virtio-gpu-bswap.h"
>   
>   #include "ui/egl-helpers.h"
>   
> @@ -24,8 +25,62 @@
>   
>   struct virgl_gpu_resource {
>       struct virtio_gpu_simple_resource res;
> +    uint32_t ref;
> +    VirtIOGPU *g;
> +
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    /* only blob resource needs this region to be mapped as guest mmio */
> +    MemoryRegion *region;
> +#endif
>   };
>   
> +static void vres_get_ref(struct virgl_gpu_resource *vres)
> +{
> +    uint32_t ref;
> +
> +    ref = qatomic_fetch_inc(&vres->ref);
> +    g_assert(ref < INT_MAX);
> +}
> +
> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    VirtIOGPU *g;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    g = vres->g;
> +    res = &vres->res;
> +    QTAILQ_REMOVE(&g->reslist, res, next);
> +    virtio_gpu_cleanup_mapping(g, res);
> +    g_free(vres);
> +}
> +
> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +
> +    if (!vres) {
> +        return;
> +    }
> +
> +    res = &vres->res;
> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
> +    virgl_renderer_resource_unref(res->resource_id);
> +}
> +
> +static void vres_put_ref(struct virgl_gpu_resource *vres)
> +{
> +    g_assert(vres->ref > 0);
> +
> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
> +        virgl_resource_unref(vres);
> +        virgl_resource_destroy(vres);
> +    }
> +}
> +
>   static struct virgl_gpu_resource *
>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
>   {
> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>                                          c2d.width, c2d.height);
>   
>       vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>       vres->res.width = c2d.width;
>       vres->res.height = c2d.height;
>       vres->res.format = c2d.format;
> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>                                          c3d.width, c3d.height, c3d.depth);
>   
>       vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
>       vres->res.width = c3d.width;
>       vres->res.height = c3d.height;
>       vres->res.format = c3d.format;
> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>           return;
>       }
>   
> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> -    virgl_renderer_resource_unref(unref.resource_id);
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    if (vres->region) {
> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +        MemoryRegion *mr = vres->region;
> +
> +        warn_report("%s: blob resource %d not unmapped",
> +                    __func__, unref.resource_id);
> +        vres->region = NULL;

Shouldn't there be a call to memory_region_unref(mr)?

> +        memory_region_set_enabled(mr, false);
> +        memory_region_del_subregion(&b->hostmem, mr);
> +        object_unparent(OBJECT(mr));
> +    }
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>   
> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> -    virtio_gpu_cleanup_mapping(g, &vres->res);
> -    g_free(vres);
> +    vres_put_ref(vres);
>   }
>   
>   static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>       g_free(resp);
>   }
>   
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +
> +static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
> +{
> +    if (!vres) {
> +        return;
> +    }
> +
> +    virgl_renderer_resource_unmap(vres->res.resource_id);
> +
> +    vres_put_ref(vres);
> +}
> +
> +static void virgl_resource_blob_async_unmap(void *obj)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    struct virgl_gpu_resource *vres = mr->opaque;
> +
> +    virgl_resource_unmap(vres);
> +
> +    g_free(obj);
> +}
> +
> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> +                                           struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_create_blob cblob;
> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> +    int ret;
> +
> +    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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, cblob.resource_id);
> +    if (vres) {
> +        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;
> +    }
> +
> +    vres = g_new0(struct virgl_gpu_resource, 1);
> +    vres_get_ref(vres);
> +    vres->g = g;
> +    vres->res.resource_id = cblob.resource_id;
> +    vres->res.blob_size = cblob.size;
> +
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> +                                            cmd, &vres->res.addrs,
> +                                            &vres->res.iov, &vres->res.iov_cnt);
> +        if (!ret) {
> +            g_free(vres);
> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +            return;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->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 = vres->res.iov;
> +    virgl_args.num_iovs = vres->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 virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_map_blob mblob;
> +    int ret;
> +    void *data;
> +    uint64_t size;
> +    struct virtio_gpu_resp_map_info resp;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    VIRTIO_GPU_FILL_CMD(mblob);
> +    virtio_gpu_map_blob_bswap(&mblob);
> +
> +    if (mblob.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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, mblob.resource_id);
> +    if (!vres) {
> +        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 (vres->region) {
> +        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 = virgl_renderer_resource_map(vres->res.resource_id, &data, &size);
> +    if (ret) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    vres_get_ref(vres);

Why is this needed? And if it is, shouldn't virgl_cmd_resource_unmap_blob
call "vres_put_ref(vres)" ?

> +    vres->region = g_new0(MemoryRegion, 1);
> +    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
> +    vres->region->opaque = vres;
> +    OBJECT(vres->region)->free = virgl_resource_blob_async_unmap;
> +    memory_region_add_subregion(&b->hostmem, mblob.offset, vres->region);
> +    memory_region_set_enabled(vres->region, true);
> +
> +    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 virgl_gpu_resource *vres;
> +    struct virtio_gpu_resource_unmap_blob ublob;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +    MemoryRegion *mr;
> +
> +    VIRTIO_GPU_FILL_CMD(ublob);
> +    virtio_gpu_unmap_blob_bswap(&ublob);
> +
> +    if (ublob.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;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, ublob.resource_id);
> +    if (!vres) {
> +        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 (!vres->region) {
> +        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;
> +    }
> +
> +    mr = vres->region;
> +    vres->region = NULL;

memory_region_unref(mr)?

Note that AFAICT without the added memory_region_unref() calls virgl_resource_unmap()
was never called.

Regards,
Pierre-Eric

> +    memory_region_set_enabled(mr, false);
> +    memory_region_del_subregion(&b->hostmem, mr);
> +    object_unparent(OBJECT(mr));
> +}
> +
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> +
>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                         struct virtio_gpu_ctrl_command *cmd)
>   {
> @@ -536,6 +789,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>       case VIRTIO_GPU_CMD_GET_EDID:
>           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;
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>       default:
>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>           break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4c3ec9d0ea..8189c392dc 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1449,10 +1449,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, "Linked virglrenderer does not support blob resources");
>               return;
>           }
> +#endif
>       }
>   
>       if (!virtio_gpu_base_device_realize(qdev,
> diff --git a/meson.build b/meson.build
> index ea52ef1b9c..629407128e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1054,6 +1054,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                            cc.has_function('virgl_renderer_context_create_with_flags',
>                                            prefix: '#include <virglrenderer.h>',
>                                            dependencies: virgl))
> +    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
> +                         cc.has_function('virgl_renderer_resource_create_blob',
> +                                         prefix: '#include <virglrenderer.h>',
> +                                         dependencies: virgl))
>     endif
>   endif
>   rutabaga = not_found

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Pierre-Eric Pelloux-Prayer]

Le 09/01/2024 à 17:50, Pierre-Eric Pelloux-Prayer a écrit :
> 
> 
> Le 19/12/2023 à 08:53, Huang Rui a écrit :
>> 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>
>> Changes in v6:
>> - Use new struct virgl_gpu_resource.
>> - Unmap, unref and destroy the resource only after the memory region
>>    has been completely removed.
>> - In unref check whether the resource is still mapped.
>> - In unmap_blob check whether the resource has been already unmapped.
>> - Fix coding style
>>
>>   hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
>>   hw/display/virtio-gpu.c       |   4 +-
>>   meson.build                   |   4 +
>>   3 files changed, 276 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index faab374336..5a3a292f79 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -17,6 +17,7 @@
>>   #include "trace.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "hw/virtio/virtio-gpu.h"
>> +#include "hw/virtio/virtio-gpu-bswap.h"
>>   #include "ui/egl-helpers.h"
>> @@ -24,8 +25,62 @@
>>   struct virgl_gpu_resource {
>>       struct virtio_gpu_simple_resource res;
>> +    uint32_t ref;
>> +    VirtIOGPU *g;
>> +
>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>> +    /* only blob resource needs this region to be mapped as guest mmio */
>> +    MemoryRegion *region;
>> +#endif
>>   };
>> +static void vres_get_ref(struct virgl_gpu_resource *vres)
>> +{
>> +    uint32_t ref;
>> +
>> +    ref = qatomic_fetch_inc(&vres->ref);
>> +    g_assert(ref < INT_MAX);
>> +}
>> +
>> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
>> +{
>> +    struct virtio_gpu_simple_resource *res;
>> +    VirtIOGPU *g;
>> +
>> +    if (!vres) {
>> +        return;
>> +    }
>> +
>> +    g = vres->g;
>> +    res = &vres->res;
>> +    QTAILQ_REMOVE(&g->reslist, res, next);
>> +    virtio_gpu_cleanup_mapping(g, res);
>> +    g_free(vres);
>> +}
>> +
>> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
>> +{
>> +    struct virtio_gpu_simple_resource *res;
>> +
>> +    if (!vres) {
>> +        return;
>> +    }
>> +
>> +    res = &vres->res;
>> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
>> +    virgl_renderer_resource_unref(res->resource_id);
>> +}
>> +
>> +static void vres_put_ref(struct virgl_gpu_resource *vres)
>> +{
>> +    g_assert(vres->ref > 0);
>> +
>> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
>> +        virgl_resource_unref(vres);
>> +        virgl_resource_destroy(vres);
>> +    }
>> +}
>> +
>>   static struct virgl_gpu_resource *
>>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
>>   {
>> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>>                                          c2d.width, c2d.height);
>>       vres = g_new0(struct virgl_gpu_resource, 1);
>> +    vres_get_ref(vres);
>> +    vres->g = g;
>>       vres->res.width = c2d.width;
>>       vres->res.height = c2d.height;
>>       vres->res.format = c2d.format;
>> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>>                                          c3d.width, c3d.height, c3d.depth);
>>       vres = g_new0(struct virgl_gpu_resource, 1);
>> +    vres_get_ref(vres);
>> +    vres->g = g;
>>       vres->res.width = c3d.width;
>>       vres->res.height = c3d.height;
>>       vres->res.format = c3d.format;
>> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>           return;
>>       }
>> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
>> -    virgl_renderer_resource_unref(unref.resource_id);
>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>> +    if (vres->region) {
>> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +        MemoryRegion *mr = vres->region;
>> +
>> +        warn_report("%s: blob resource %d not unmapped",
>> +                    __func__, unref.resource_id);
>> +        vres->region = NULL;
> 
> Shouldn't there be a call to memory_region_unref(mr)?
> 
>> +        memory_region_set_enabled(mr, false);
>> +        memory_region_del_subregion(&b->hostmem, mr);
>> +        object_unparent(OBJECT(mr));
>> +    }
>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
>> -    virtio_gpu_cleanup_mapping(g, &vres->res);
>> -    g_free(vres);
>> +    vres_put_ref(vres);
>>   }
>>   static void virgl_cmd_context_create(VirtIOGPU *g,
>> @@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>>       g_free(resp);
>>   }
>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>> +
>> +static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
>> +{
>> +    if (!vres) {
>> +        return;
>> +    }
>> +
>> +    virgl_renderer_resource_unmap(vres->res.resource_id);
>> +
>> +    vres_put_ref(vres);
>> +}
>> +
>> +static void virgl_resource_blob_async_unmap(void *obj)
>> +{
>> +    MemoryRegion *mr = MEMORY_REGION(obj);
>> +    struct virgl_gpu_resource *vres = mr->opaque;
>> +
>> +    virgl_resource_unmap(vres);
>> +
>> +    g_free(obj);
>> +}
>> +
>> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>> +                                           struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virgl_gpu_resource *vres;
>> +    struct virtio_gpu_resource_create_blob cblob;
>> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
>> +    int ret;
>> +
>> +    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;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, cblob.resource_id);
>> +    if (vres) {
>> +        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;
>> +    }
>> +
>> +    vres = g_new0(struct virgl_gpu_resource, 1);
>> +    vres_get_ref(vres);
>> +    vres->g = g;
>> +    vres->res.resource_id = cblob.resource_id;
>> +    vres->res.blob_size = cblob.size;
>> +
>> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
>> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
>> +                                            cmd, &vres->res.addrs,
>> +                                            &vres->res.iov, &vres->res.iov_cnt);
>> +        if (!ret) {
>> +            g_free(vres);
>> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>> +            return;
>> +        }
>> +    }
>> +
>> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->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 = vres->res.iov;
>> +    virgl_args.num_iovs = vres->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 virgl_gpu_resource *vres;
>> +    struct virtio_gpu_resource_map_blob mblob;
>> +    int ret;
>> +    void *data;
>> +    uint64_t size;
>> +    struct virtio_gpu_resp_map_info resp;
>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +
>> +    VIRTIO_GPU_FILL_CMD(mblob);
>> +    virtio_gpu_map_blob_bswap(&mblob);
>> +
>> +    if (mblob.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;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, mblob.resource_id);
>> +    if (!vres) {
>> +        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 (vres->region) {
>> +        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 = virgl_renderer_resource_map(vres->res.resource_id, &data, &size);
>> +    if (ret) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
>> +                      __func__, strerror(-ret));
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>> +
>> +    vres_get_ref(vres);
> 
> Why is this needed? And if it is, shouldn't virgl_cmd_resource_unmap_blob
> call "vres_put_ref(vres)" ?
> 
>> +    vres->region = g_new0(MemoryRegion, 1);
>> +    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
>> +    vres->region->opaque = vres;
>> +    OBJECT(vres->region)->free = virgl_resource_blob_async_unmap;
>> +    memory_region_add_subregion(&b->hostmem, mblob.offset, vres->region);
>> +    memory_region_set_enabled(vres->region, true);
>> +
>> +    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 virgl_gpu_resource *vres;
>> +    struct virtio_gpu_resource_unmap_blob ublob;
>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>> +    MemoryRegion *mr;
>> +
>> +    VIRTIO_GPU_FILL_CMD(ublob);
>> +    virtio_gpu_unmap_blob_bswap(&ublob);
>> +
>> +    if (ublob.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;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, ublob.resource_id);
>> +    if (!vres) {
>> +        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 (!vres->region) {
>> +        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;
>> +    }
>> +
>> +    mr = vres->region;
>> +    vres->region = NULL;
> 
> memory_region_unref(mr)?
> 
> Note that AFAICT without the added memory_region_unref() calls virgl_resource_unmap()
> was never called.


Xenia and I figured out the refcounting issue: this code is written based on the
assumption that:
  
    object_unparent(OBJECT(mr));


Will decrement the refcount. But this assumption is only true if mr->parent_obj.parent
is non-NULL.

The map_blob function uses the following arguments:

    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);

Since name is NULL, mr won't be added as a child of 'g' and thus object_unparent()
does nothing.

I'd suggest 2 changes:
    * use a name ("blob_memory"?) to so mr can be a child of g
    * increment mr's refcount when setting vres->region and decrement it when clearing it.
      This change is not needed technically but when a variable is refcounted it seems
      clearer to increment/decrement the refcount in these situations.


Pierre-Eric


> 
>> +    memory_region_set_enabled(mr, false);
>> +    memory_region_del_subregion(&b->hostmem, mr);
>> +    object_unparent(OBJECT(mr));
>> +}
>> +
>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>> +
>>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>                                         struct virtio_gpu_ctrl_command *cmd)
>>   {
>> @@ -536,6 +789,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>       case VIRTIO_GPU_CMD_GET_EDID:
>>           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;
>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>>       default:
>>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>           break;
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 4c3ec9d0ea..8189c392dc 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -1449,10 +1449,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, "Linked virglrenderer does not support blob resources");
>>               return;
>>           }
>> +#endif
>>       }
>>       if (!virtio_gpu_base_device_realize(qdev,
>> diff --git a/meson.build b/meson.build
>> index ea52ef1b9c..629407128e 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1054,6 +1054,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>>                            cc.has_function('virgl_renderer_context_create_with_flags',
>>                                            prefix: '#include <virglrenderer.h>',
>>                                            dependencies: virgl))
>> +    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
>> +                         cc.has_function('virgl_renderer_resource_create_blob',
>> +                                         prefix: '#include <virglrenderer.h>',
>> +                                         dependencies: virgl))
>>     endif
>>   endif
>>   rutabaga = not_found
> 

Re: [PATCH v6 07/11] virtio-gpu: Handle resource blob commands

[Huang Rui via]

On Wed, Jan 10, 2024 at 04:51:31PM +0800, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> Le 09/01/2024 à 17:50, Pierre-Eric Pelloux-Prayer a écrit :
> > 
> > 
> > Le 19/12/2023 à 08:53, Huang Rui a écrit :
> >> 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> >> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >> ---
> >>
> >> Changes in v6:
> >> - Use new struct virgl_gpu_resource.
> >> - Unmap, unref and destroy the resource only after the memory region
> >>    has been completely removed.
> >> - In unref check whether the resource is still mapped.
> >> - In unmap_blob check whether the resource has been already unmapped.
> >> - Fix coding style
> >>
> >>   hw/display/virtio-gpu-virgl.c | 274 +++++++++++++++++++++++++++++++++-
> >>   hw/display/virtio-gpu.c       |   4 +-
> >>   meson.build                   |   4 +
> >>   3 files changed, 276 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >> index faab374336..5a3a292f79 100644
> >> --- a/hw/display/virtio-gpu-virgl.c
> >> +++ b/hw/display/virtio-gpu-virgl.c
> >> @@ -17,6 +17,7 @@
> >>   #include "trace.h"
> >>   #include "hw/virtio/virtio.h"
> >>   #include "hw/virtio/virtio-gpu.h"
> >> +#include "hw/virtio/virtio-gpu-bswap.h"
> >>   #include "ui/egl-helpers.h"
> >> @@ -24,8 +25,62 @@
> >>   struct virgl_gpu_resource {
> >>       struct virtio_gpu_simple_resource res;
> >> +    uint32_t ref;
> >> +    VirtIOGPU *g;
> >> +
> >> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >> +    /* only blob resource needs this region to be mapped as guest mmio */
> >> +    MemoryRegion *region;
> >> +#endif
> >>   };
> >> +static void vres_get_ref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    uint32_t ref;
> >> +
> >> +    ref = qatomic_fetch_inc(&vres->ref);
> >> +    g_assert(ref < INT_MAX);
> >> +}
> >> +
> >> +static void virgl_resource_destroy(struct virgl_gpu_resource *vres)
> >> +{
> >> +    struct virtio_gpu_simple_resource *res;
> >> +    VirtIOGPU *g;
> >> +
> >> +    if (!vres) {
> >> +        return;
> >> +    }
> >> +
> >> +    g = vres->g;
> >> +    res = &vres->res;
> >> +    QTAILQ_REMOVE(&g->reslist, res, next);
> >> +    virtio_gpu_cleanup_mapping(g, res);
> >> +    g_free(vres);
> >> +}
> >> +
> >> +static void virgl_resource_unref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    struct virtio_gpu_simple_resource *res;
> >> +
> >> +    if (!vres) {
> >> +        return;
> >> +    }
> >> +
> >> +    res = &vres->res;
> >> +    virgl_renderer_resource_detach_iov(res->resource_id, NULL, NULL);
> >> +    virgl_renderer_resource_unref(res->resource_id);
> >> +}
> >> +
> >> +static void vres_put_ref(struct virgl_gpu_resource *vres)
> >> +{
> >> +    g_assert(vres->ref > 0);
> >> +
> >> +    if (qatomic_fetch_dec(&vres->ref) == 1) {
> >> +        virgl_resource_unref(vres);
> >> +        virgl_resource_destroy(vres);
> >> +    }
> >> +}
> >> +
> >>   static struct virgl_gpu_resource *
> >>   virgl_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> >>   {
> >> @@ -59,6 +114,8 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >>                                          c2d.width, c2d.height);
> >>       vres = g_new0(struct virgl_gpu_resource, 1);
> >> +    vres_get_ref(vres);
> >> +    vres->g = g;
> >>       vres->res.width = c2d.width;
> >>       vres->res.height = c2d.height;
> >>       vres->res.format = c2d.format;
> >> @@ -91,6 +148,8 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >>                                          c3d.width, c3d.height, c3d.depth);
> >>       vres = g_new0(struct virgl_gpu_resource, 1);
> >> +    vres_get_ref(vres);
> >> +    vres->g = g;
> >>       vres->res.width = c3d.width;
> >>       vres->res.height = c3d.height;
> >>       vres->res.format = c3d.format;
> >> @@ -126,12 +185,21 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>           return;
> >>       }
> >> -    virgl_renderer_resource_detach_iov(unref.resource_id, NULL, NULL);
> >> -    virgl_renderer_resource_unref(unref.resource_id);
> >> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >> +    if (vres->region) {
> >> +        VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> >> +        MemoryRegion *mr = vres->region;
> >> +
> >> +        warn_report("%s: blob resource %d not unmapped",
> >> +                    __func__, unref.resource_id);
> >> +        vres->region = NULL;
> > 
> > Shouldn't there be a call to memory_region_unref(mr)?
> > 
> >> +        memory_region_set_enabled(mr, false);
> >> +        memory_region_del_subregion(&b->hostmem, mr);
> >> +        object_unparent(OBJECT(mr));
> >> +    }
> >> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >> -    QTAILQ_REMOVE(&g->reslist, &vres->res, next);
> >> -    virtio_gpu_cleanup_mapping(g, &vres->res);
> >> -    g_free(vres);
> >> +    vres_put_ref(vres);
> >>   }
> >>   static void virgl_cmd_context_create(VirtIOGPU *g,
> >> @@ -470,6 +538,191 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >>       g_free(resp);
> >>   }
> >> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >> +
> >> +static void virgl_resource_unmap(struct virgl_gpu_resource *vres)
> >> +{
> >> +    if (!vres) {
> >> +        return;
> >> +    }
> >> +
> >> +    virgl_renderer_resource_unmap(vres->res.resource_id);
> >> +
> >> +    vres_put_ref(vres);
> >> +}
> >> +
> >> +static void virgl_resource_blob_async_unmap(void *obj)
> >> +{
> >> +    MemoryRegion *mr = MEMORY_REGION(obj);
> >> +    struct virgl_gpu_resource *vres = mr->opaque;
> >> +
> >> +    virgl_resource_unmap(vres);
> >> +
> >> +    g_free(obj);
> >> +}
> >> +
> >> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> >> +                                           struct virtio_gpu_ctrl_command *cmd)
> >> +{
> >> +    struct virgl_gpu_resource *vres;
> >> +    struct virtio_gpu_resource_create_blob cblob;
> >> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> >> +    int ret;
> >> +
> >> +    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;
> >> +    }
> >> +
> >> +    vres = virgl_gpu_find_resource(g, cblob.resource_id);
> >> +    if (vres) {
> >> +        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;
> >> +    }
> >> +
> >> +    vres = g_new0(struct virgl_gpu_resource, 1);
> >> +    vres_get_ref(vres);
> >> +    vres->g = g;
> >> +    vres->res.resource_id = cblob.resource_id;
> >> +    vres->res.blob_size = cblob.size;
> >> +
> >> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> >> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> >> +                                            cmd, &vres->res.addrs,
> >> +                                            &vres->res.iov, &vres->res.iov_cnt);
> >> +        if (!ret) {
> >> +            g_free(vres);
> >> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    QTAILQ_INSERT_HEAD(&g->reslist, &vres->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 = vres->res.iov;
> >> +    virgl_args.num_iovs = vres->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 virgl_gpu_resource *vres;
> >> +    struct virtio_gpu_resource_map_blob mblob;
> >> +    int ret;
> >> +    void *data;
> >> +    uint64_t size;
> >> +    struct virtio_gpu_resp_map_info resp;
> >> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> >> +
> >> +    VIRTIO_GPU_FILL_CMD(mblob);
> >> +    virtio_gpu_map_blob_bswap(&mblob);
> >> +
> >> +    if (mblob.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;
> >> +    }
> >> +
> >> +    vres = virgl_gpu_find_resource(g, mblob.resource_id);
> >> +    if (!vres) {
> >> +        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 (vres->region) {
> >> +        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 = virgl_renderer_resource_map(vres->res.resource_id, &data, &size);
> >> +    if (ret) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> >> +                      __func__, strerror(-ret));
> >> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> >> +        return;
> >> +    }
> >> +
> >> +    vres_get_ref(vres);
> > 
> > Why is this needed? And if it is, shouldn't virgl_cmd_resource_unmap_blob
> > call "vres_put_ref(vres)" ?
> > 
> >> +    vres->region = g_new0(MemoryRegion, 1);
> >> +    memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
> >> +    vres->region->opaque = vres;
> >> +    OBJECT(vres->region)->free = virgl_resource_blob_async_unmap;
> >> +    memory_region_add_subregion(&b->hostmem, mblob.offset, vres->region);
> >> +    memory_region_set_enabled(vres->region, true);
> >> +
> >> +    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 virgl_gpu_resource *vres;
> >> +    struct virtio_gpu_resource_unmap_blob ublob;
> >> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> >> +    MemoryRegion *mr;
> >> +
> >> +    VIRTIO_GPU_FILL_CMD(ublob);
> >> +    virtio_gpu_unmap_blob_bswap(&ublob);
> >> +
> >> +    if (ublob.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;
> >> +    }
> >> +
> >> +    vres = virgl_gpu_find_resource(g, ublob.resource_id);
> >> +    if (!vres) {
> >> +        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 (!vres->region) {
> >> +        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;
> >> +    }
> >> +
> >> +    mr = vres->region;
> >> +    vres->region = NULL;
> > 
> > memory_region_unref(mr)?
> > 
> > Note that AFAICT without the added memory_region_unref() calls virgl_resource_unmap()
> > was never called.
> 
> 
> Xenia and I figured out the refcounting issue: this code is written based on the
> assumption that:
>   
>     object_unparent(OBJECT(mr));
> 
> 
> Will decrement the refcount. But this assumption is only true if mr->parent_obj.parent
> is non-NULL.
> 
> The map_blob function uses the following arguments:
> 
>     memory_region_init_ram_ptr(vres->region, OBJECT(g), NULL, size, data);
> 
> Since name is NULL, mr won't be added as a child of 'g' and thus object_unparent()
> does nothing.
> 
> I'd suggest 2 changes:
>     * use a name ("blob_memory"?) to so mr can be a child of g
>     * increment mr's refcount when setting vres->region and decrement it when clearing it.
>       This change is not needed technically but when a variable is refcounted it seems
>       clearer to increment/decrement the refcount in these situations.
> 

Yes, issue is caused by NULL in name field, I will update according to your
suggestions in V7.

Thanks,
Ray

> 
> Pierre-Eric
> 
> 
> > 
> >> +    memory_region_set_enabled(mr, false);
> >> +    memory_region_del_subregion(&b->hostmem, mr);
> >> +    object_unparent(OBJECT(mr));
> >> +}
> >> +
> >> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >> +
> >>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >>                                         struct virtio_gpu_ctrl_command *cmd)
> >>   {
> >> @@ -536,6 +789,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >>       case VIRTIO_GPU_CMD_GET_EDID:
> >>           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;
> >> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >>       default:
> >>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >>           break;
> >> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >> index 4c3ec9d0ea..8189c392dc 100644
> >> --- a/hw/display/virtio-gpu.c
> >> +++ b/hw/display/virtio-gpu.c
> >> @@ -1449,10 +1449,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, "Linked virglrenderer does not support blob resources");
> >>               return;
> >>           }
> >> +#endif
> >>       }
> >>       if (!virtio_gpu_base_device_realize(qdev,
> >> diff --git a/meson.build b/meson.build
> >> index ea52ef1b9c..629407128e 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -1054,6 +1054,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
> >>                            cc.has_function('virgl_renderer_context_create_with_flags',
> >>                                            prefix: '#include <virglrenderer.h>',
> >>                                            dependencies: virgl))
> >> +    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
> >> +                         cc.has_function('virgl_renderer_resource_create_blob',
> >> +                                         prefix: '#include <virglrenderer.h>',
> >> +                                         dependencies: virgl))
> >>     endif
> >>   endif
> >>   rutabaga = not_found
> > 

[PATCH v6 08/11] virtio-gpu: Resource UUID

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Enable resource UUID feature and implement command resource assign UUID.
This is done by introducing a hash table to map resource IDs to their
UUIDs.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Set resource uuid as option.
- Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
  or virtio live migration.
- Use g_int_hash/g_int_equal instead of the default.
- Move virtio_vgpu_simple_resource initialization in the earlier new patch
  "virtio-gpu: Introduce virgl_gpu_resource structure"

 hw/display/trace-events        |   1 +
 hw/display/virtio-gpu-base.c   |   4 ++
 hw/display/virtio-gpu-virgl.c  |   3 +
 hw/display/virtio-gpu.c        | 119 +++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-gpu.h |   7 ++
 5 files changed, 134 insertions(+)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index 2336a0ca15..54d6894c59 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
 virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
+virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 37af256219..6bcee3882f 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -236,6 +236,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
         features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
     }
 
+    if (virtio_gpu_resource_uuid_enabled(g->conf)) {
+        features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
+    }
+
     return features;
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 5a3a292f79..be9da6e780 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -777,6 +777,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
         /* TODO add security */
         virgl_cmd_ctx_detach_resource(g, cmd);
         break;
+    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
+        virtio_gpu_resource_assign_uuid(g, cmd);
+        break;
     case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
         virgl_cmd_get_capset_info(g, cmd);
         break;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 8189c392dc..466debb256 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -958,6 +958,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
     virtio_gpu_cleanup_mapping(g, res);
 }
 
+void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
+                                     struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_simple_resource *res;
+    struct virtio_gpu_resource_assign_uuid assign;
+    struct virtio_gpu_resp_resource_uuid resp;
+    QemuUUID *uuid;
+
+    VIRTIO_GPU_FILL_CMD(assign);
+    virtio_gpu_bswap_32(&assign, sizeof(assign));
+    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
+
+    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
+    if (!res) {
+        return;
+    }
+
+    memset(&resp, 0, sizeof(resp));
+    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
+
+    uuid = g_hash_table_lookup(g->resource_uuids, &assign.resource_id);
+    if (!uuid) {
+        uuid = g_new(QemuUUID, 1);
+        qemu_uuid_generate(uuid);
+        g_hash_table_insert(g->resource_uuids, &assign.resource_id, uuid);
+    }
+
+    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
+    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+}
+
 void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
                                    struct virtio_gpu_ctrl_command *cmd)
 {
@@ -1006,6 +1037,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
     case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
         virtio_gpu_resource_detach_backing(g, cmd);
         break;
+    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
+        virtio_gpu_resource_assign_uuid(g, cmd);
+        break;
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         break;
@@ -1400,6 +1434,57 @@ static int virtio_gpu_blob_load(QEMUFile *f, void *opaque, size_t size,
     return 0;
 }
 
+static int virtio_gpu_resource_uuid_save(QEMUFile *f, void *opaque, size_t size,
+                                         const VMStateField *field,
+                                         JSONWriter *vmdesc)
+{
+    VirtIOGPU *g = opaque;
+    struct virtio_gpu_simple_resource *res;
+    QemuUUID *uuid;
+
+    /* in 2d mode we should never find unprocessed commands here */
+    assert(QTAILQ_EMPTY(&g->cmdq));
+
+    QTAILQ_FOREACH(res, &g->reslist, next) {
+        qemu_put_be32(f, res->resource_id);
+        uuid = g_hash_table_lookup(g->resource_uuids, &res->resource_id);
+        qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
+    }
+    qemu_put_be32(f, 0); /* end of list */
+
+    g_hash_table_destroy(g->resource_uuids);
+
+    return 0;
+}
+
+static int virtio_gpu_resource_uuid_load(QEMUFile *f, void *opaque, size_t size,
+                                         const VMStateField *field)
+{
+    VirtIOGPU *g = opaque;
+    struct virtio_gpu_simple_resource *res;
+    uint32_t resource_id;
+    QemuUUID *uuid = NULL;
+
+    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
+    resource_id = qemu_get_be32(f);
+    while (resource_id != 0) {
+        res = virtio_gpu_find_resource(g, resource_id);
+        if (res) {
+            return -EINVAL;
+        }
+
+        res = g_new0(struct virtio_gpu_simple_resource, 1);
+        res->resource_id = resource_id;
+
+        qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
+        g_hash_table_insert(g->resource_uuids, &res->resource_id, uuid);
+
+        resource_id = qemu_get_be32(f);
+    }
+
+    return 0;
+}
+
 static int virtio_gpu_post_load(void *opaque, int version_id)
 {
     VirtIOGPU *g = opaque;
@@ -1475,12 +1560,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     QTAILQ_INIT(&g->reslist);
     QTAILQ_INIT(&g->cmdq);
     QTAILQ_INIT(&g->fenceq);
+
+    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
 }
 
 static void virtio_gpu_device_unrealize(DeviceState *qdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(qdev);
 
+    g_hash_table_destroy(g->resource_uuids);
     g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
     g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
     g_clear_pointer(&g->reset_bh, qemu_bh_delete);
@@ -1534,6 +1622,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
         g_free(cmd);
     }
 
+    g_hash_table_remove_all(g->resource_uuids);
+
     virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
 }
 
@@ -1583,6 +1673,32 @@ const VMStateDescription vmstate_virtio_gpu_blob_state = {
     },
 };
 
+static bool virtio_gpu_resource_uuid_state_needed(void *opaque)
+{
+    VirtIOGPU *g = VIRTIO_GPU(opaque);
+
+    return virtio_gpu_resource_uuid_enabled(g->parent_obj.conf);
+}
+
+const VMStateDescription vmstate_virtio_gpu_resource_uuid_state = {
+    .name = "virtio-gpu/resource_uuid",
+    .minimum_version_id = VIRTIO_GPU_VM_VERSION,
+    .version_id = VIRTIO_GPU_VM_VERSION,
+    .needed = virtio_gpu_resource_uuid_state_needed,
+    .fields = (const VMStateField[]){
+        {
+            .name = "virtio-gpu/resource_uuid",
+            .info = &(const VMStateInfo) {
+                .name = "resource_uuid",
+                .get = virtio_gpu_resource_uuid_load,
+                .put = virtio_gpu_resource_uuid_save,
+            },
+            .flags = VMS_SINGLE,
+        } /* device */,
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 /*
  * For historical reasons virtio_gpu does not adhere to virtio migration
  * scheme as described in doc/virtio-migration.txt, in a sense that no
@@ -1610,6 +1726,7 @@ static const VMStateDescription vmstate_virtio_gpu = {
     },
     .subsections = (const VMStateDescription * []) {
         &vmstate_virtio_gpu_blob_state,
+        &vmstate_virtio_gpu_resource_uuid_state,
         NULL
     },
     .post_load = virtio_gpu_post_load,
@@ -1622,6 +1739,8 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+    DEFINE_PROP_BIT("resource_uuid", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED, false),
 #ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
     DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 584ba2ed73..76b410fe91 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -98,6 +98,7 @@ enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
     VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
     VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
+    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -114,6 +115,8 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
 #define virtio_gpu_rutabaga_enabled(_cfg) \
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
+#define virtio_gpu_resource_uuid_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED))
 #define virtio_gpu_hostmem_enabled(_cfg) \
     (_cfg.hostmem > 0)
 
@@ -209,6 +212,8 @@ struct VirtIOGPU {
         QTAILQ_HEAD(, VGPUDMABuf) bufs;
         VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
     } dmabuf;
+
+    GHashTable *resource_uuids;
 };
 
 struct VirtIOGPUClass {
@@ -307,6 +312,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
                                     struct iovec *iov, uint32_t count);
 void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
                                 struct virtio_gpu_simple_resource *res);
+void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
+                                     struct virtio_gpu_ctrl_command *cmd);
 void virtio_gpu_process_cmdq(VirtIOGPU *g);
 void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
 void virtio_gpu_reset(VirtIODevice *vdev);
-- 
2.25.1

Re: [PATCH v6 08/11] virtio-gpu: Resource UUID

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> Enable resource UUID feature and implement command resource assign UUID.
> This is done by introducing a hash table to map resource IDs to their
> UUIDs.

What about putting QemuUUID in struct virtio_gpu_simple_resource?

Re: [PATCH v6 08/11] virtio-gpu: Resource UUID

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> Enable resource UUID feature and implement command resource assign UUID.
> This is done by introducing a hash table to map resource IDs to their
> UUIDs.

I agree with Akihiko, what about putting QemuUUID in struct
virtio_gpu_simple_resource?

(I also doubt about the hash table usefulness, but I don't know
how/why the UUID is used)

>
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> Changes in v6:
> - Set resource uuid as option.
> - Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
>   or virtio live migration.
> - Use g_int_hash/g_int_equal instead of the default.
> - Move virtio_vgpu_simple_resource initialization in the earlier new patch
>   "virtio-gpu: Introduce virgl_gpu_resource structure"
>
>  hw/display/trace-events        |   1 +
>  hw/display/virtio-gpu-base.c   |   4 ++
>  hw/display/virtio-gpu-virgl.c  |   3 +
>  hw/display/virtio-gpu.c        | 119 +++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-gpu.h |   7 ++
>  5 files changed, 134 insertions(+)
>
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 2336a0ca15..54d6894c59 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
>  virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 37af256219..6bcee3882f 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -236,6 +236,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>          features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
>      }
>
> +    if (virtio_gpu_resource_uuid_enabled(g->conf)) {
> +        features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> +    }
> +
>      return features;
>  }
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 5a3a292f79..be9da6e780 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -777,6 +777,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>          /* TODO add security */
>          virgl_cmd_ctx_detach_resource(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>      case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
>          virgl_cmd_get_capset_info(g, cmd);
>          break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 8189c392dc..466debb256 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -958,6 +958,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
>      virtio_gpu_cleanup_mapping(g, res);
>  }
>
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_assign_uuid assign;
> +    struct virtio_gpu_resp_resource_uuid resp;
> +    QemuUUID *uuid;
> +
> +    VIRTIO_GPU_FILL_CMD(assign);
> +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> +
> +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> +    if (!res) {
> +        return;
> +    }
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> +
> +    uuid = g_hash_table_lookup(g->resource_uuids, &assign.resource_id);
> +    if (!uuid) {
> +        uuid = g_new(QemuUUID, 1);
> +        qemu_uuid_generate(uuid);
> +        g_hash_table_insert(g->resource_uuids, &assign.resource_id, uuid);
> +    }
> +
> +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +}
> +
>  void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>                                     struct virtio_gpu_ctrl_command *cmd)
>  {
> @@ -1006,6 +1037,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>          virtio_gpu_resource_detach_backing(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> @@ -1400,6 +1434,57 @@ static int virtio_gpu_blob_load(QEMUFile *f, void *opaque, size_t size,
>      return 0;
>  }
>
> +static int virtio_gpu_resource_uuid_save(QEMUFile *f, void *opaque, size_t size,
> +                                         const VMStateField *field,
> +                                         JSONWriter *vmdesc)
> +{
> +    VirtIOGPU *g = opaque;
> +    struct virtio_gpu_simple_resource *res;
> +    QemuUUID *uuid;
> +
> +    /* in 2d mode we should never find unprocessed commands here */
> +    assert(QTAILQ_EMPTY(&g->cmdq));
> +
> +    QTAILQ_FOREACH(res, &g->reslist, next) {
> +        qemu_put_be32(f, res->resource_id);
> +        uuid = g_hash_table_lookup(g->resource_uuids, &res->resource_id);
> +        qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
> +    }
> +    qemu_put_be32(f, 0); /* end of list */
> +
> +    g_hash_table_destroy(g->resource_uuids);
> +
> +    return 0;
> +}
> +
> +static int virtio_gpu_resource_uuid_load(QEMUFile *f, void *opaque, size_t size,
> +                                         const VMStateField *field)
> +{
> +    VirtIOGPU *g = opaque;
> +    struct virtio_gpu_simple_resource *res;
> +    uint32_t resource_id;
> +    QemuUUID *uuid = NULL;
> +
> +    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
> +    resource_id = qemu_get_be32(f);
> +    while (resource_id != 0) {
> +        res = virtio_gpu_find_resource(g, resource_id);
> +        if (res) {
> +            return -EINVAL;
> +        }
> +
> +        res = g_new0(struct virtio_gpu_simple_resource, 1);
> +        res->resource_id = resource_id;
> +
> +        qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
> +        g_hash_table_insert(g->resource_uuids, &res->resource_id, uuid);
> +
> +        resource_id = qemu_get_be32(f);
> +    }
> +
> +    return 0;
> +}
> +
>  static int virtio_gpu_post_load(void *opaque, int version_id)
>  {
>      VirtIOGPU *g = opaque;
> @@ -1475,12 +1560,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>      QTAILQ_INIT(&g->reslist);
>      QTAILQ_INIT(&g->cmdq);
>      QTAILQ_INIT(&g->fenceq);
> +
> +    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
>  }
>
>  static void virtio_gpu_device_unrealize(DeviceState *qdev)
>  {
>      VirtIOGPU *g = VIRTIO_GPU(qdev);
>
> +    g_hash_table_destroy(g->resource_uuids);

better:
g_clear_pointer(&g->resource_uuids, g_hash_table_unref);

>      g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
>      g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
>      g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> @@ -1534,6 +1622,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>          g_free(cmd);
>      }
>
> +    g_hash_table_remove_all(g->resource_uuids);
> +
>      virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
>  }
>
> @@ -1583,6 +1673,32 @@ const VMStateDescription vmstate_virtio_gpu_blob_state = {
>      },
>  };
>
> +static bool virtio_gpu_resource_uuid_state_needed(void *opaque)
> +{
> +    VirtIOGPU *g = VIRTIO_GPU(opaque);
> +
> +    return virtio_gpu_resource_uuid_enabled(g->parent_obj.conf);
> +}
> +
> +const VMStateDescription vmstate_virtio_gpu_resource_uuid_state = {
> +    .name = "virtio-gpu/resource_uuid",
> +    .minimum_version_id = VIRTIO_GPU_VM_VERSION,
> +    .version_id = VIRTIO_GPU_VM_VERSION,
> +    .needed = virtio_gpu_resource_uuid_state_needed,
> +    .fields = (const VMStateField[]){
> +        {
> +            .name = "virtio-gpu/resource_uuid",
> +            .info = &(const VMStateInfo) {
> +                .name = "resource_uuid",
> +                .get = virtio_gpu_resource_uuid_load,
> +                .put = virtio_gpu_resource_uuid_save,
> +            },
> +            .flags = VMS_SINGLE,
> +        } /* device */,
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  /*
>   * For historical reasons virtio_gpu does not adhere to virtio migration
>   * scheme as described in doc/virtio-migration.txt, in a sense that no
> @@ -1610,6 +1726,7 @@ static const VMStateDescription vmstate_virtio_gpu = {
>      },
>      .subsections = (const VMStateDescription * []) {
>          &vmstate_virtio_gpu_blob_state,
> +        &vmstate_virtio_gpu_resource_uuid_state,
>          NULL
>      },
>      .post_load = virtio_gpu_post_load,
> @@ -1622,6 +1739,8 @@ static Property virtio_gpu_properties[] = {
>      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +    DEFINE_PROP_BIT("resource_uuid", VirtIOGPU, parent_obj.conf.flags,
> +                    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED, false),

why not enable it by default? (and set it to false for machine < 9.0

>  #ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
>      DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>                      VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 584ba2ed73..76b410fe91 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -98,6 +98,7 @@ enum virtio_gpu_base_conf_flags {
>      VIRTIO_GPU_FLAG_BLOB_ENABLED,
>      VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
>      VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
> +    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED,
>  };
>
>  #define virtio_gpu_virgl_enabled(_cfg) \
> @@ -114,6 +115,8 @@ enum virtio_gpu_base_conf_flags {
>      (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
>  #define virtio_gpu_rutabaga_enabled(_cfg) \
>      (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
> +#define virtio_gpu_resource_uuid_enabled(_cfg) \
> +    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED))
>  #define virtio_gpu_hostmem_enabled(_cfg) \
>      (_cfg.hostmem > 0)
>
> @@ -209,6 +212,8 @@ struct VirtIOGPU {
>          QTAILQ_HEAD(, VGPUDMABuf) bufs;
>          VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
>      } dmabuf;
> +
> +    GHashTable *resource_uuids;
>  };
>
>  struct VirtIOGPUClass {
> @@ -307,6 +312,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
>                                      struct iovec *iov, uint32_t count);
>  void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>                                  struct virtio_gpu_simple_resource *res);
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd);
>  void virtio_gpu_process_cmdq(VirtIOGPU *g);
>  void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
>  void virtio_gpu_reset(VirtIODevice *vdev);
> --
> 2.25.1
>


-- 
Marc-André Lureau

Re: [PATCH v6 08/11] virtio-gpu: Resource UUID

[Huang Rui]

On Tue, Jan 02, 2024 at 08:49:54PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
> >
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >
> > Enable resource UUID feature and implement command resource assign UUID.
> > This is done by introducing a hash table to map resource IDs to their
> > UUIDs.
> 
> I agree with Akihiko, what about putting QemuUUID in struct
> virtio_gpu_simple_resource?

OK, I will add a member of UUID in simple resource structure.

> 
> (I also doubt about the hash table usefulness, but I don't know
> how/why the UUID is used)
> 

The system cannot be without this patch, let me figure it out the reason.

Thanks,
Ray

> >
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > Changes in v6:
> > - Set resource uuid as option.
> > - Implement optional subsection of vmstate_virtio_gpu_resource_uuid_state
> >   or virtio live migration.
> > - Use g_int_hash/g_int_equal instead of the default.
> > - Move virtio_vgpu_simple_resource initialization in the earlier new patch
> >   "virtio-gpu: Introduce virgl_gpu_resource structure"
> >
> >  hw/display/trace-events        |   1 +
> >  hw/display/virtio-gpu-base.c   |   4 ++
> >  hw/display/virtio-gpu-virgl.c  |   3 +
> >  hw/display/virtio-gpu.c        | 119 +++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio-gpu.h |   7 ++
> >  5 files changed, 134 insertions(+)
> >
> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 2336a0ca15..54d6894c59 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
> >  virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> > +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index 37af256219..6bcee3882f 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -236,6 +236,10 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> >          features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> >      }
> >
> > +    if (virtio_gpu_resource_uuid_enabled(g->conf)) {
> > +        features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> > +    }
> > +
> >      return features;
> >  }
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 5a3a292f79..be9da6e780 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -777,6 +777,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >          /* TODO add security */
> >          virgl_cmd_ctx_detach_resource(g, cmd);
> >          break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >      case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
> >          virgl_cmd_get_capset_info(g, cmd);
> >          break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 8189c392dc..466debb256 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -958,6 +958,37 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
> >      virtio_gpu_cleanup_mapping(g, res);
> >  }
> >
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_assign_uuid assign;
> > +    struct virtio_gpu_resp_resource_uuid resp;
> > +    QemuUUID *uuid;
> > +
> > +    VIRTIO_GPU_FILL_CMD(assign);
> > +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> > +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> > +
> > +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> > +    if (!res) {
> > +        return;
> > +    }
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> > +
> > +    uuid = g_hash_table_lookup(g->resource_uuids, &assign.resource_id);
> > +    if (!uuid) {
> > +        uuid = g_new(QemuUUID, 1);
> > +        qemu_uuid_generate(uuid);
> > +        g_hash_table_insert(g->resource_uuids, &assign.resource_id, uuid);
> > +    }
> > +
> > +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> > +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> > +}
> > +
> >  void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >                                     struct virtio_gpu_ctrl_command *cmd)
> >  {
> > @@ -1006,6 +1037,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
> >          virtio_gpu_resource_detach_backing(g, cmd);
> >          break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >      default:
> >          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >          break;
> > @@ -1400,6 +1434,57 @@ static int virtio_gpu_blob_load(QEMUFile *f, void *opaque, size_t size,
> >      return 0;
> >  }
> >
> > +static int virtio_gpu_resource_uuid_save(QEMUFile *f, void *opaque, size_t size,
> > +                                         const VMStateField *field,
> > +                                         JSONWriter *vmdesc)
> > +{
> > +    VirtIOGPU *g = opaque;
> > +    struct virtio_gpu_simple_resource *res;
> > +    QemuUUID *uuid;
> > +
> > +    /* in 2d mode we should never find unprocessed commands here */
> > +    assert(QTAILQ_EMPTY(&g->cmdq));
> > +
> > +    QTAILQ_FOREACH(res, &g->reslist, next) {
> > +        qemu_put_be32(f, res->resource_id);
> > +        uuid = g_hash_table_lookup(g->resource_uuids, &res->resource_id);
> > +        qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
> > +    }
> > +    qemu_put_be32(f, 0); /* end of list */
> > +
> > +    g_hash_table_destroy(g->resource_uuids);
> > +
> > +    return 0;
> > +}
> > +
> > +static int virtio_gpu_resource_uuid_load(QEMUFile *f, void *opaque, size_t size,
> > +                                         const VMStateField *field)
> > +{
> > +    VirtIOGPU *g = opaque;
> > +    struct virtio_gpu_simple_resource *res;
> > +    uint32_t resource_id;
> > +    QemuUUID *uuid = NULL;
> > +
> > +    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
> > +    resource_id = qemu_get_be32(f);
> > +    while (resource_id != 0) {
> > +        res = virtio_gpu_find_resource(g, resource_id);
> > +        if (res) {
> > +            return -EINVAL;
> > +        }
> > +
> > +        res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +        res->resource_id = resource_id;
> > +
> > +        qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
> > +        g_hash_table_insert(g->resource_uuids, &res->resource_id, uuid);
> > +
> > +        resource_id = qemu_get_be32(f);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int virtio_gpu_post_load(void *opaque, int version_id)
> >  {
> >      VirtIOGPU *g = opaque;
> > @@ -1475,12 +1560,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >      QTAILQ_INIT(&g->reslist);
> >      QTAILQ_INIT(&g->cmdq);
> >      QTAILQ_INIT(&g->fenceq);
> > +
> > +    g->resource_uuids = g_hash_table_new_full(g_int_hash, g_int_equal, NULL, g_free);
> >  }
> >
> >  static void virtio_gpu_device_unrealize(DeviceState *qdev)
> >  {
> >      VirtIOGPU *g = VIRTIO_GPU(qdev);
> >
> > +    g_hash_table_destroy(g->resource_uuids);
> 
> better:
> g_clear_pointer(&g->resource_uuids, g_hash_table_unref);
> 
> >      g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
> >      g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
> >      g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> > @@ -1534,6 +1622,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
> >          g_free(cmd);
> >      }
> >
> > +    g_hash_table_remove_all(g->resource_uuids);
> > +
> >      virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
> >  }
> >
> > @@ -1583,6 +1673,32 @@ const VMStateDescription vmstate_virtio_gpu_blob_state = {
> >      },
> >  };
> >
> > +static bool virtio_gpu_resource_uuid_state_needed(void *opaque)
> > +{
> > +    VirtIOGPU *g = VIRTIO_GPU(opaque);
> > +
> > +    return virtio_gpu_resource_uuid_enabled(g->parent_obj.conf);
> > +}
> > +
> > +const VMStateDescription vmstate_virtio_gpu_resource_uuid_state = {
> > +    .name = "virtio-gpu/resource_uuid",
> > +    .minimum_version_id = VIRTIO_GPU_VM_VERSION,
> > +    .version_id = VIRTIO_GPU_VM_VERSION,
> > +    .needed = virtio_gpu_resource_uuid_state_needed,
> > +    .fields = (const VMStateField[]){
> > +        {
> > +            .name = "virtio-gpu/resource_uuid",
> > +            .info = &(const VMStateInfo) {
> > +                .name = "resource_uuid",
> > +                .get = virtio_gpu_resource_uuid_load,
> > +                .put = virtio_gpu_resource_uuid_save,
> > +            },
> > +            .flags = VMS_SINGLE,
> > +        } /* device */,
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  /*
> >   * For historical reasons virtio_gpu does not adhere to virtio migration
> >   * scheme as described in doc/virtio-migration.txt, in a sense that no
> > @@ -1610,6 +1726,7 @@ static const VMStateDescription vmstate_virtio_gpu = {
> >      },
> >      .subsections = (const VMStateDescription * []) {
> >          &vmstate_virtio_gpu_blob_state,
> > +        &vmstate_virtio_gpu_resource_uuid_state,
> >          NULL
> >      },
> >      .post_load = virtio_gpu_post_load,
> > @@ -1622,6 +1739,8 @@ static Property virtio_gpu_properties[] = {
> >      DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >                      VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >      DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > +    DEFINE_PROP_BIT("resource_uuid", VirtIOGPU, parent_obj.conf.flags,
> > +                    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED, false),
> 
> why not enable it by default? (and set it to false for machine < 9.0
> 
> >  #ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
> >      DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> >                      VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index 584ba2ed73..76b410fe91 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -98,6 +98,7 @@ enum virtio_gpu_base_conf_flags {
> >      VIRTIO_GPU_FLAG_BLOB_ENABLED,
> >      VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
> >      VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
> > +    VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED,
> >  };
> >
> >  #define virtio_gpu_virgl_enabled(_cfg) \
> > @@ -114,6 +115,8 @@ enum virtio_gpu_base_conf_flags {
> >      (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
> >  #define virtio_gpu_rutabaga_enabled(_cfg) \
> >      (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
> > +#define virtio_gpu_resource_uuid_enabled(_cfg) \
> > +    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RESOURCE_UUID_ENABLED))
> >  #define virtio_gpu_hostmem_enabled(_cfg) \
> >      (_cfg.hostmem > 0)
> >
> > @@ -209,6 +212,8 @@ struct VirtIOGPU {
> >          QTAILQ_HEAD(, VGPUDMABuf) bufs;
> >          VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
> >      } dmabuf;
> > +
> > +    GHashTable *resource_uuids;
> >  };
> >
> >  struct VirtIOGPUClass {
> > @@ -307,6 +312,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
> >                                      struct iovec *iov, uint32_t count);
> >  void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
> >                                  struct virtio_gpu_simple_resource *res);
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd);
> >  void virtio_gpu_process_cmdq(VirtIOGPU *g);
> >  void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
> >  void virtio_gpu_reset(VirtIODevice *vdev);
> > --
> > 2.25.1
> >
> 
> 
> -- 
> Marc-André Lureau

[PATCH v6 09/11] virtio-gpu: Support Venus capset

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Add support for the Venus capset, which enables Vulkan support through
the Venus Vulkan driver for virtio-gpu.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

No change in v6.

 hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index be9da6e780..f35a751824 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -506,6 +506,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
         virgl_renderer_get_cap_set(resp.capset_id,
                                    &resp.capset_max_version,
                                    &resp.capset_max_size);
+    } else if (info.capset_index == 2) {
+        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
+        virgl_renderer_get_cap_set(resp.capset_id,
+                                   &resp.capset_max_version,
+                                   &resp.capset_max_size);
     } else {
         resp.capset_max_version = 0;
         resp.capset_max_size = 0;
@@ -978,10 +983,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
 
 int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
 {
-    uint32_t capset2_max_ver, capset2_max_size;
+    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
+    num_capsets = 1;
+
     virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
-                              &capset2_max_ver,
-                              &capset2_max_size);
+                               &capset2_max_ver,
+                               &capset2_max_size);
+    num_capsets += capset2_max_ver ? 1 : 0;
+
+    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+                               &capset2_max_ver,
+                               &capset2_max_size);
+    num_capsets += capset2_max_size ? 1 : 0;
 
-    return capset2_max_ver ? 2 : 1;
+    return num_capsets;
 }
-- 
2.25.1

Re: [PATCH v6 09/11] virtio-gpu: Support Venus capset

[Pierre-Eric Pelloux-Prayer]

Hi Ray, Antonio,


Le 19/12/2023 à 08:53, Huang Rui a écrit :
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> Add support for the Venus capset, which enables Vulkan support through
> the Venus Vulkan driver for virtio-gpu.
> 
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> No change in v6.
> 
>   hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index be9da6e780..f35a751824 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -506,6 +506,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
>           virgl_renderer_get_cap_set(resp.capset_id,
>                                      &resp.capset_max_version,
>                                      &resp.capset_max_size);
> +    } else if (info.capset_index == 2) {
> +        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
> +        virgl_renderer_get_cap_set(resp.capset_id,
> +                                   &resp.capset_max_version,
> +                                   &resp.capset_max_size);
>       } else {
>           resp.capset_max_version = 0;
>           resp.capset_max_size = 0;
> @@ -978,10 +983,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>   
>   int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>   {
> -    uint32_t capset2_max_ver, capset2_max_size;
> +    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
> +    num_capsets = 1;
> +
>       virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
> -                              &capset2_max_ver,
> -                              &capset2_max_size);
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_ver ? 1 : 0;
> +
> +    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_size ? 1 : 0;

IMHO the logic here doesn't work.

The kernel will use num_capset like this:

	for (i = 0; i < num_capsets; i++) {
		virtio_gpu_cmd_get_capset_info(vgdev, i);

So if num_capset = 2, it will query the capset info of index 0 and 1.
Capset 0 is alway VIRGL so it's fine. But since VIRL2 support is optional,
QEMU has no way to know if index 1 is VIRGL2 (if it's supported)
or VENUS (if VIRGL2 support is missing).
And it'll get worse when we will want to support CAPSET_DRM.

Ray: we have a patch internally for this (virtio-gpu: fix capset query), you
may want to add it to this series, before this patch.

Regards,
Pierre-Eric



>   
> -    return capset2_max_ver ? 2 : 1;
> +    return num_capsets;
>   }

[PATCH v6 10/11] virtio-gpu: Initialize Venus

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Request Venus when initializing VirGL.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Remove the unstable API flags check because virglrenderer is already 1.0.
- Squash the render server flag support into "Initialize Venus".

 hw/display/virtio-gpu-virgl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index f35a751824..c523a6717a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     }
 #endif
 
+#ifdef VIRGL_RENDERER_VENUS
+    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
+#endif
+
     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
     if (ret != 0) {
         error_report("virgl could not be initialized: %d", ret);
-- 
2.25.1

Re: [PATCH v6 10/11] virtio-gpu: Initialize Venus

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> Request Venus when initializing VirGL.
>
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> Changes in v6:
> - Remove the unstable API flags check because virglrenderer is already 1.0.
> - Squash the render server flag support into "Initialize Venus".
>
>  hw/display/virtio-gpu-virgl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index f35a751824..c523a6717a 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>      }
>  #endif
>
> +#ifdef VIRGL_RENDERER_VENUS
> +    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
> +#endif
> +

I wonder if it's a good idea to initialize venus by default. It
doesn't seem to require vulkan during initialization, but this may
evolve. Make it optional?

>      ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
>      if (ret != 0) {
>          error_report("virgl could not be initialized: %d", ret);
> --
> 2.25.1
>


-- 
Marc-André Lureau

Re: [PATCH v6 10/11] virtio-gpu: Initialize Venus

[Huang Rui]

On Tue, Jan 02, 2024 at 09:33:11PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
> >
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >
> > Request Venus when initializing VirGL.
> >
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > Changes in v6:
> > - Remove the unstable API flags check because virglrenderer is already 1.0.
> > - Squash the render server flag support into "Initialize Venus".
> >
> >  hw/display/virtio-gpu-virgl.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index f35a751824..c523a6717a 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >      }
> >  #endif
> >
> > +#ifdef VIRGL_RENDERER_VENUS
> > +    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
> > +#endif
> > +
> 
> I wonder if it's a good idea to initialize venus by default. It
> doesn't seem to require vulkan during initialization, but this may
> evolve. Make it optional?
> 

I am fine. In fact, vulkan is widely used for graphic area such as gaming,
compute, VR/AR, etc.

Thanks,
Ray

> >      ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
> >      if (ret != 0) {
> >          error_report("virgl could not be initialized: %d", ret);
> > --
> > 2.25.1
> >
> 
> 
> -- 
> Marc-André Lureau

Re: [PATCH v6 10/11] virtio-gpu: Initialize Venus

[Pierre-Eric Pelloux-Prayer]

Le 23/02/2024 à 10:15, Huang Rui a écrit :
> On Tue, Jan 02, 2024 at 09:33:11PM +0800, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Dec 19, 2023 at 11:55 AM Huang Rui <ray.huang@amd.com> wrote:
>>>
>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>
>>> Request Venus when initializing VirGL.
>>>
>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>
>>> Changes in v6:
>>> - Remove the unstable API flags check because virglrenderer is already 1.0.
>>> - Squash the render server flag support into "Initialize Venus".
>>>
>>>   hw/display/virtio-gpu-virgl.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index f35a751824..c523a6717a 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -964,6 +964,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>>>       }
>>>   #endif
>>>
>>> +#ifdef VIRGL_RENDERER_VENUS
>>> +    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
>>> +#endif
>>> +
>>
>> I wonder if it's a good idea to initialize venus by default. It
>> doesn't seem to require vulkan during initialization, but this may
>> evolve. Make it optional?
>>
> 
> I am fine. In fact, vulkan is widely used for graphic area such as gaming,
> compute, VR/AR, etc.

Actually, making it optional is useful because Venus support is optional in
virglrenderer (= having VIRGL_RENDERER_VENUS defined doesn't mean that
Venus is supported).

Thanks,
Pierre-Eric


> 
> Thanks,
> Ray
> 
>>>       ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
>>>       if (ret != 0) {
>>>           error_report("virgl could not be initialized: %d", ret);
>>> --
>>> 2.25.1
>>>
>>
>>
>> -- 
>> Marc-André Lureau
> 

[PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Huang Rui]

From: Robert Beckett <bob.beckett@collabora.com>

This relies on a virglrenderer change to include the dmabuf fd when
returning resource info.

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

Changes in v6:
- Add scanout_blob function for virtio-gpu-virgl.
- Update for new virgl_gpu_resource.

 hw/display/virtio-gpu-virgl.c  | 104 +++++++++++++++++++++++++++++++++
 hw/display/virtio-gpu.c        |   4 +-
 include/hw/virtio/virtio-gpu.h |   6 ++
 3 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index c523a6717a..c384225a98 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -18,6 +18,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
 #include "hw/virtio/virtio-gpu-bswap.h"
+#include "hw/virtio/virtio-gpu-pixman.h"
 
 #include "ui/egl-helpers.h"
 
@@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
     object_unparent(OBJECT(mr));
 }
 
+static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
+                                       struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virgl_gpu_resource *vres;
+    struct virtio_gpu_framebuffer fb = { 0 };
+    struct virtio_gpu_set_scanout_blob ss;
+    struct virgl_renderer_resource_info info;
+    uint64_t fbend;
+
+    VIRTIO_GPU_FILL_CMD(ss);
+    virtio_gpu_scanout_blob_bswap(&ss);
+    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
+                                          ss.r.width, ss.r.height, ss.r.x,
+                                          ss.r.y);
+
+    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+                      __func__, ss.scanout_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+        return;
+    }
+
+    if (ss.resource_id == 0) {
+        virtio_gpu_disable_scanout(g, ss.scanout_id);
+        return;
+    }
+
+    if (ss.width < 16 ||
+        ss.height < 16 ||
+        ss.r.x + ss.r.width > ss.width ||
+        ss.r.y + ss.r.height > ss.height) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
+                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
+                      __func__, ss.scanout_id, ss.resource_id,
+                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
+                      ss.width, ss.height);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without GL!\n", __func__);
+        return;
+    }
+
+    vres = virgl_gpu_find_resource(g, ss.resource_id);
+    if (!vres) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: illegal resource specified %d\n",
+                      __func__, ss.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: illegal virgl resource specified %d\n",
+                      __func__, ss.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+    if (!vres->res.dmabuf_fd && info.fd)
+        vres->res.dmabuf_fd = info.fd;
+
+    fb.format = virtio_gpu_get_pixman_format(ss.format);
+    if (!fb.format) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: host couldn't handle guest format %d\n",
+                      __func__, ss.format);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
+    fb.width = ss.width;
+    fb.height = ss.height;
+    fb.stride = ss.strides[0];
+    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
+
+    fbend = fb.offset;
+    fbend += fb.stride * (ss.r.height - 1);
+    fbend += fb.bytes_pp * ss.r.width;
+    if (fbend > vres->res.blob_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: fb end out of range\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    g->parent_obj.enable = 1;
+    if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res,
+                                 &fb, &ss.r)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: failed to update dmabuf\n", __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+    virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r);
+}
+
 #endif /* HAVE_VIRGL_RESOURCE_BLOB */
 
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
@@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     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;
 #endif /* HAVE_VIRGL_RESOURCE_BLOB */
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 466debb256..492f578b4b 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -380,7 +380,7 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
     QTAILQ_INSERT_HEAD(&g->reslist, res, next);
 }
 
-static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
+void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
 {
     struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id];
     struct virtio_gpu_simple_resource *res;
@@ -592,7 +592,7 @@ static void virtio_unref_resource(pixman_image_t *image, void *data)
     pixman_image_unref(data);
 }
 
-static void virtio_gpu_update_scanout(VirtIOGPU *g,
+void virtio_gpu_update_scanout(VirtIOGPU *g,
                                       uint32_t scanout_id,
                                       struct virtio_gpu_simple_resource *res,
                                       struct virtio_gpu_rect *r)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 76b410fe91..ac2adfb607 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -332,6 +332,12 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
                              struct virtio_gpu_framebuffer *fb,
                              struct virtio_gpu_rect *r);
 
+void virtio_gpu_update_scanout(VirtIOGPU *g,
+                               uint32_t scanout_id,
+                               struct virtio_gpu_simple_resource *res,
+                               struct virtio_gpu_rect *r);
+void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id);
+
 /* virtio-gpu-3d.c */
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                   struct virtio_gpu_ctrl_command *cmd);
-- 
2.25.1

Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Akihiko Odaki]

On 2023/12/19 16:53, Huang Rui wrote:
> From: Robert Beckett <bob.beckett@collabora.com>
> 
> This relies on a virglrenderer change to include the dmabuf fd when
> returning resource info.
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> Changes in v6:
> - Add scanout_blob function for virtio-gpu-virgl.
> - Update for new virgl_gpu_resource.
> 
>   hw/display/virtio-gpu-virgl.c  | 104 +++++++++++++++++++++++++++++++++
>   hw/display/virtio-gpu.c        |   4 +-
>   include/hw/virtio/virtio-gpu.h |   6 ++
>   3 files changed, 112 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index c523a6717a..c384225a98 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -18,6 +18,7 @@
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-gpu.h"
>   #include "hw/virtio/virtio-gpu-bswap.h"
> +#include "hw/virtio/virtio-gpu-pixman.h"
>   
>   #include "ui/egl-helpers.h"
>   
> @@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
>       object_unparent(OBJECT(mr));
>   }
>   
> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
> +                                       struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virgl_gpu_resource *vres;
> +    struct virtio_gpu_framebuffer fb = { 0 };
> +    struct virtio_gpu_set_scanout_blob ss;
> +    struct virgl_renderer_resource_info info;
> +    uint64_t fbend;
> +
> +    VIRTIO_GPU_FILL_CMD(ss);
> +    virtio_gpu_scanout_blob_bswap(&ss);
> +    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
> +                                          ss.r.width, ss.r.height, ss.r.x,
> +                                          ss.r.y);
> +
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
> +    if (ss.resource_id == 0) {
> +        virtio_gpu_disable_scanout(g, ss.scanout_id);
> +        return;
> +    }
> +
> +    if (ss.width < 16 ||
> +        ss.height < 16 ||
> +        ss.r.x + ss.r.width > ss.width ||
> +        ss.r.y + ss.r.height > ss.height) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
> +                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
> +                      __func__, ss.scanout_id, ss.resource_id,
> +                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
> +                      ss.width, ss.height);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {

Shouldn't OpenGL always be available for virgl?

> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without GL!\n", __func__);
> +        return;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, ss.resource_id);
> +    if (!vres) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: illegal resource specified %d\n",
> +                      __func__, ss.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: illegal virgl resource specified %d\n",
> +                      __func__, ss.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +    if (!vres->res.dmabuf_fd && info.fd)
> +        vres->res.dmabuf_fd = info.fd;
> +
> +    fb.format = virtio_gpu_get_pixman_format(ss.format);
> +    if (!fb.format) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: host couldn't handle guest format %d\n",
> +                      __func__, ss.format);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
> +    fb.width = ss.width;
> +    fb.height = ss.height;
> +    fb.stride = ss.strides[0];
> +    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
> +
> +    fbend = fb.offset;
> +    fbend += fb.stride * (ss.r.height - 1);
> +    fbend += fb.bytes_pp * ss.r.width;
> +    if (fbend > vres->res.blob_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fb end out of range\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    g->parent_obj.enable = 1;
> +    if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res,
> +                                 &fb, &ss.r)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: failed to update dmabuf\n", __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +    virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r);
> +}
> +
>   #endif /* HAVE_VIRGL_RESOURCE_BLOB */
>   
>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> @@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>       case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
>           virgl_cmd_resource_unmap_blob(g, cmd);
>           break;
> +    case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB:

VIRTIO_GPU_CMD_SET_SCANOUT_BLOB support should be added before allowing 
the user to enable the resource blob support.

You should also check if virtio_vdev_has_feature(VIRTIO_DEVICE(g), 
VIRTIO_GPU_F_RESOURCE_BLOB). It also applies to other resource blob 
commands though I failed to note that for the earlier patch.

Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Pierre-Eric Pelloux-Prayer]

Le 21/12/2023 à 07:25, Akihiko Odaki a écrit :
> On 2023/12/19 16:53, Huang Rui wrote:
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> This relies on a virglrenderer change to include the dmabuf fd when
>> returning resource info.
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>
>> Changes in v6:
>> - Add scanout_blob function for virtio-gpu-virgl.
>> - Update for new virgl_gpu_resource.
>>
>>   hw/display/virtio-gpu-virgl.c  | 104 +++++++++++++++++++++++++++++++++
>>   hw/display/virtio-gpu.c        |   4 +-
>>   include/hw/virtio/virtio-gpu.h |   6 ++
>>   3 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index c523a6717a..c384225a98 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/virtio/virtio.h"
>>   #include "hw/virtio/virtio-gpu.h"
>>   #include "hw/virtio/virtio-gpu-bswap.h"
>> +#include "hw/virtio/virtio-gpu-pixman.h"
>>   #include "ui/egl-helpers.h"
>> @@ -726,6 +727,106 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
>>       object_unparent(OBJECT(mr));
>>   }
>> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>> +                                       struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virgl_gpu_resource *vres;
>> +    struct virtio_gpu_framebuffer fb = { 0 };
>> +    struct virtio_gpu_set_scanout_blob ss;
>> +    struct virgl_renderer_resource_info info;
>> +    uint64_t fbend;
>> +
>> +    VIRTIO_GPU_FILL_CMD(ss);
>> +    virtio_gpu_scanout_blob_bswap(&ss);
>> +    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
>> +                                          ss.r.width, ss.r.height, ss.r.x,
>> +                                          ss.r.y);
>> +
>> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
>> +                      __func__, ss.scanout_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
>> +        return;
>> +    }
>> +
>> +    if (ss.resource_id == 0) {
>> +        virtio_gpu_disable_scanout(g, ss.scanout_id);
>> +        return;
>> +    }
>> +
>> +    if (ss.width < 16 ||
>> +        ss.height < 16 ||
>> +        ss.r.x + ss.r.width > ss.width ||
>> +        ss.r.y + ss.r.height > ss.height) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
>> +                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
>> +                      __func__, ss.scanout_id, ss.resource_id,
>> +                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
>> +                      ss.width, ss.height);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
> 
> Shouldn't OpenGL always be available for virgl?
> 
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without GL!\n", __func__);
>> +        return;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, ss.resource_id);
>> +    if (!vres) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>> +    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal virgl resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>> +    if (!vres->res.dmabuf_fd && info.fd)
>> +        vres->res.dmabuf_fd = info.fd;
>> +
>> +    fb.format = virtio_gpu_get_pixman_format(ss.format);
>> +    if (!fb.format) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: host couldn't handle guest format %d\n",
>> +                      __func__, ss.format);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
>> +    fb.width = ss.width;
>> +    fb.height = ss.height;
>> +    fb.stride = ss.strides[0];
>> +    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
>> +
>> +    fbend = fb.offset;
>> +    fbend += fb.stride * (ss.r.height - 1);
>> +    fbend += fb.bytes_pp * ss.r.width;
>> +    if (fbend > vres->res.blob_size) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: fb end out of range\n",
>> +                      __func__);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    g->parent_obj.enable = 1;
>> +    if (virtio_gpu_update_dmabuf(g, ss.scanout_id, &vres->res,
>> +                                 &fb, &ss.r)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: failed to update dmabuf\n", __func__);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +    virtio_gpu_update_scanout(g, ss.scanout_id, &vres->res, &ss.r);
>> +}
>> +
>>   #endif /* HAVE_VIRGL_RESOURCE_BLOB */
>>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>> @@ -807,6 +908,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>       case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
>>           virgl_cmd_resource_unmap_blob(g, cmd);
>>           break;
>> +    case VIRTIO_GPU_CMD_SET_SCANOUT_BLOB:
> 
> VIRTIO_GPU_CMD_SET_SCANOUT_BLOB support should be added before allowing the user to enable the resource blob support.

It seems that two patches were squashed together:

- "virtio-gpu: Handle set scanout blob command" by Antonio Caggiano
- "virtio-gpu: Handle set scanout blob command" by Bob Beckett

Restoring the original patches with the appropriate S-o-b tags seems the right thing to do.

Thanks,
Pierre-Eric
> 
> You should also check if virtio_vdev_has_feature(VIRTIO_DEVICE(g), VIRTIO_GPU_F_RESOURCE_BLOB). It also applies to other resource blob commands though I failed to note that for the earlier patch.
> 

Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Alex Bennée]

Huang Rui <ray.huang@amd.com> writes:

> From: Robert Beckett <bob.beckett@collabora.com>
>
> This relies on a virglrenderer change to include the dmabuf fd when
> returning resource info.
>
<snip>
> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
> +                                       struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virgl_gpu_resource *vres;
> +    struct virtio_gpu_framebuffer fb = { 0 };
> +    struct virtio_gpu_set_scanout_blob ss;
> +    struct virgl_renderer_resource_info info;
> +    uint64_t fbend;
> +
> +    VIRTIO_GPU_FILL_CMD(ss);
> +    virtio_gpu_scanout_blob_bswap(&ss);
> +    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
> +                                          ss.r.width, ss.r.height, ss.r.x,
> +                                          ss.r.y);
> +
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
> +    if (ss.resource_id == 0) {
> +        virtio_gpu_disable_scanout(g, ss.scanout_id);
> +        return;
> +    }
> +
> +    if (ss.width < 16 ||
> +        ss.height < 16 ||
> +        ss.r.x + ss.r.width > ss.width ||
> +        ss.r.y + ss.r.height > ss.height) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
> +                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
> +                      __func__, ss.scanout_id, ss.resource_id,
> +                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
> +                      ss.width, ss.height);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without GL!\n", __func__);
> +        return;
> +    }
> +
> +    vres = virgl_gpu_find_resource(g, ss.resource_id);
> +    if (!vres) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: illegal resource specified %d\n",
> +                      __func__, ss.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: illegal virgl resource specified %d\n",
> +                      __func__, ss.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }

Minor nit, the format of the following needs to include braces.

> +    if (!vres->res.dmabuf_fd && info.fd)
> +        vres->res.dmabuf_fd = info.fd;

However I'm seeing:

  cc -m64 -mcx16 -Ilibcommon.fa.p -I../../common-user/host/x86_64 -I../../linux-user/include/host/x86_64 -I../../linux-user/include -Iui -I../../ui -I/usr/include/capstone -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/fribidi -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/x86_64-linux-gnu -I/usr/include/atk-1.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/vte-2.91 -I/usr/include/virgl -I/home/alex/lsrc/qemu.git/builds/extra.libs/install/include -I/usr/include/cacard -I/usr/include/nss -I/usr/include/nspr -I/usr/include/PCSC -I/usr/include/pipewire-0.3 -I/usr/include/spa-0.2 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -Wshadow=local -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/host/include/x86_64 -iquote /home/alex/lsrc/qemu.git/host/include/generic -iquote /home/alex/lsrc/qemu.git/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -DNCURSES_WIDECHAR=1 -D_REENTRANT -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -MF libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o.d -o libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -c ../../hw/display/virtio-gpu-virgl.c
  ../../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_set_scanout_blob’:
  ../../hw/display/virtio-gpu-virgl.c:790:37: error: ‘struct virgl_renderer_resource_info’ has no member named ‘fd’
    790 |     if (!vres->res.dmabuf_fd && info.fd)
        |                                     ^
  ../../hw/display/virtio-gpu-virgl.c:791:35: error: ‘struct virgl_renderer_resource_info’ has no member named ‘fd’
    791 |         vres->res.dmabuf_fd = info.fd;
        |                                   ^

But searching my extra libs (for aemu/gfstream/rutabaga_ffi) I can see
the bindings.rs but nothing generated a header:

  $ ag -r "virgl_renderer_resource_info" 
  crosvm.git/rutabaga_gfx/src/generated/virgl_renderer_bindings.rs
  33:pub const VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION: u32 = 0;
  337:pub struct virgl_renderer_resource_info {
  351:pub struct virgl_renderer_resource_info_ext {
  353:    pub base: virgl_renderer_resource_info,
  359:impl Default for virgl_renderer_resource_info_ext {
  373:        info: *mut virgl_renderer_resource_info,
  379:        info: *mut virgl_renderer_resource_info_ext,

Which makes me think a) its picked up the older virgl headers and b) the
crosvm/rutabaf_gfx install needs a fix.


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v6 11/11] virtio-gpu: make blob scanout use dmabuf fd

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Huang Rui <ray.huang@amd.com> writes:
>
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> This relies on a virglrenderer change to include the dmabuf fd when
>> returning resource info.
>>
> <snip>
>> +static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>> +                                       struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virgl_gpu_resource *vres;
>> +    struct virtio_gpu_framebuffer fb = { 0 };
>> +    struct virtio_gpu_set_scanout_blob ss;
>> +    struct virgl_renderer_resource_info info;
>> +    uint64_t fbend;
>> +
>> +    VIRTIO_GPU_FILL_CMD(ss);
>> +    virtio_gpu_scanout_blob_bswap(&ss);
>> +    trace_virtio_gpu_cmd_set_scanout_blob(ss.scanout_id, ss.resource_id,
>> +                                          ss.r.width, ss.r.height, ss.r.x,
>> +                                          ss.r.y);
>> +
>> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
>> +                      __func__, ss.scanout_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
>> +        return;
>> +    }
>> +
>> +    if (ss.resource_id == 0) {
>> +        virtio_gpu_disable_scanout(g, ss.scanout_id);
>> +        return;
>> +    }
>> +
>> +    if (ss.width < 16 ||
>> +        ss.height < 16 ||
>> +        ss.r.x + ss.r.width > ss.width ||
>> +        ss.r.y + ss.r.height > ss.height) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d bounds for"
>> +                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
>> +                      __func__, ss.scanout_id, ss.resource_id,
>> +                      ss.r.x, ss.r.y, ss.r.width, ss.r.height,
>> +                      ss.width, ss.height);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    if (!console_has_gl(g->parent_obj.scanout[ss.scanout_id].con)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: unable to scanout blot without GL!\n", __func__);
>> +        return;
>> +    }
>> +
>> +    vres = virgl_gpu_find_resource(g, ss.resource_id);
>> +    if (!vres) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>> +    if (virgl_renderer_resource_get_info(ss.resource_id, &info)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: illegal virgl resource specified %d\n",
>> +                      __func__, ss.resource_id);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> +        return;
>> +    }
>
> Minor nit, the format of the following needs to include braces.
>
>> +    if (!vres->res.dmabuf_fd && info.fd)
>> +        vres->res.dmabuf_fd = info.fd;
>
> However I'm seeing:
>
>   cc -m64 -mcx16 -Ilibcommon.fa.p -I../../common-user/host/x86_64 -I../../linux-user/include/host/x86_64 -I../../linux-user/include -Iui -I../../ui -I/usr/include/capstone -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/fribidi -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/x86_64-linux-gnu -I/usr/include/atk-1.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/vte-2.91 -I/usr/include/virgl -I/home/alex/lsrc/qemu.git/builds/extra.libs/install/include -I/usr/include/cacard -I/usr/include/nss -I/usr/include/nspr -I/usr/include/PCSC -I/usr/include/pipewire-0.3 -I/usr/include/spa-0.2 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -Wshadow=local -isystem /home/alex/lsrc/qemu.git/linux-headers -isystem linux-headers -iquote . -iquote /home/alex/lsrc/qemu.git -iquote /home/alex/lsrc/qemu.git/include -iquote /home/alex/lsrc/qemu.git/host/include/x86_64 -iquote /home/alex/lsrc/qemu.git/host/include/generic -iquote /home/alex/lsrc/qemu.git/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -fPIE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -DNCURSES_WIDECHAR=1 -D_REENTRANT -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -MF libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o.d -o libcommon.fa.p/hw_display_virtio-gpu-virgl.c.o -c ../../hw/display/virtio-gpu-virgl.c
>   ../../hw/display/virtio-gpu-virgl.c: In function ‘virgl_cmd_set_scanout_blob’:
>   ../../hw/display/virtio-gpu-virgl.c:790:37: error: ‘struct virgl_renderer_resource_info’ has no member named ‘fd’
>     790 |     if (!vres->res.dmabuf_fd && info.fd)
>         |                                     ^
>   ../../hw/display/virtio-gpu-virgl.c:791:35: error: ‘struct virgl_renderer_resource_info’ has no member named ‘fd’
>     791 |         vres->res.dmabuf_fd = info.fd;
>         |                                   ^
>
> But searching my extra libs (for aemu/gfstream/rutabaga_ffi) I can see
> the bindings.rs but nothing generated a header:
>
>   $ ag -r "virgl_renderer_resource_info" 
>   crosvm.git/rutabaga_gfx/src/generated/virgl_renderer_bindings.rs
>   33:pub const VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION: u32 = 0;
>   337:pub struct virgl_renderer_resource_info {
>   351:pub struct virgl_renderer_resource_info_ext {
>   353:    pub base: virgl_renderer_resource_info,
>   359:impl Default for virgl_renderer_resource_info_ext {
>   373:        info: *mut virgl_renderer_resource_info,
>   379:        info: *mut virgl_renderer_resource_info_ext,
>
> Which makes me think a) its picked up the older virgl headers and b) the
> crosvm/rutabaf_gfx install needs a fix.

Actually it was libvirglrenderer was too old (I got it the wrong way
round, the rust bindings come from libvirglrenderer). As we want to
build with older libvirglrenderers on older systems I think this needs a
tweak to meson.build, maybe something like:

    config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
                         cc.has_function('virgl_renderer_resource_create_blob',
                                          prefix: '#include <virglrenderer.h>',
                                          dependencies: virgl)
                         and
                         cc.has_member('struct virgl_renderer_resource_info', 'fd',
                                       prefix: '#include <virglrenderer.h>',
                                       dependencies: virgl))

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro