Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-06 Thread Akihiko Odaki 

On 2023/09/06 16:56, Huang Rui wrote:

On Wed, Sep 06, 2023 at 11:39:09AM +0800, Akihiko Odaki wrote:

On 2023/09/06 12:09, Huang Rui wrote:

On Tue, Sep 05, 2023 at 05:20:43PM +0800, Akihiko Odaki wrote:

On 2023/09/05 18:08, Huang Rui wrote:

On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:

On 2023/08/31 18:32, Huang Rui wrote:

From: Antonio Caggiano 

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 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

v1->v2:
- 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 that will be
  released automatically once the memory region is unparented and all
  of its references have been released.
  Also, since the pointer can be used to indicate whether the blob
  is mapped, the explicit 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.

- Remove direct references to parent_obj.

- Separate declarations from code.

 hw/display/virtio-gpu-virgl.c  | 213 +
 hw/display/virtio-gpu.c|   4 +-
 include/hw/virtio/virtio-gpu.h |   5 +
 meson.build|   4 +
 4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 312953ec16..17b634d4ee 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"
 
@@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,

 virgl_renderer_resource_create(&args, NULL, 0);
 }
 
+static void virgl_resource_destroy(VirtIOGPU *g,

+   struct virtio_gpu_simple_resource *res)
+{
+if (!res)
+return;
+
+QTAILQ_REMOVE(&g->reslist, res, next);
+
+virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+g_free(res->addrs);
+
+g_free(res);
+}
+
 static void virgl_cmd_resource_unref(VirtIOGPU *g,
  struct virtio_gpu_ctrl_command *cmd)
 {
+struct virtio_gpu_simple_resource *res;
 struct virtio_gpu_resource_unref unref;
 struct iovec *res_iovs = NULL;
 int num_iovs = 0;
@@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
 VIRTIO_GPU_FILL_CMD(unref);
 trace_virtio_gpu_cmd_res_unref(unref.resource_id);
 
+res = virtio_gpu_find_resource(g, 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);
+if (res) {
+res->iov = NULL;
+res->iov_cnt = 0;
+}
 }
+
 virgl_renderer_resource_unref(unref.resource_id);
+
+virgl_resource_destroy(g, res);
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,

@@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
 g_free(resp);
 }
 
+#ifdef HAVE_VIRGL_RESOURCE_BLOB

+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+   struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_simple_resource *res;
+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

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-06 Thread Huang Rui 
On Wed, Sep 06, 2023 at 11:39:09AM +0800, Akihiko Odaki wrote:
> On 2023/09/06 12:09, Huang Rui wrote:
> > On Tue, Sep 05, 2023 at 05:20:43PM +0800, Akihiko Odaki wrote:
> >> On 2023/09/05 18:08, Huang Rui wrote:
> >>> On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:
>  On 2023/08/31 18:32, Huang Rui wrote:
> > From: Antonio Caggiano 
> >
> > 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 
> > Signed-off-by: Dmitry Osipenko 
> > Signed-off-by: Xenia Ragiadakou 
> > Signed-off-by: Huang Rui 
> > ---
> >
> > v1->v2:
> >- 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 that will 
> > be
> >  released automatically once the memory region is unparented 
> > and all
> >  of its references have been released.
> >  Also, since the pointer can be used to indicate whether the 
> > blob
> >  is mapped, the explicit 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.
> >
> >- Remove direct references to parent_obj.
> >
> >- Separate declarations from code.
> >
> > hw/display/virtio-gpu-virgl.c  | 213 
> > +
> > hw/display/virtio-gpu.c|   4 +-
> > include/hw/virtio/virtio-gpu.h |   5 +
> > meson.build|   4 +
> > 4 files changed, 225 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c 
> > b/hw/display/virtio-gpu-virgl.c
> > index 312953ec16..17b634d4ee 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"
> > 
> > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU 
> > *g,
> > virgl_renderer_resource_create(&args, NULL, 0);
> > }
> > 
> > +static void virgl_resource_destroy(VirtIOGPU *g,
> > +   struct virtio_gpu_simple_resource 
> > *res)
> > +{
> > +if (!res)
> > +return;
> > +
> > +QTAILQ_REMOVE(&g->reslist, res, next);
> > +
> > +virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> > +g_free(res->addrs);
> > +
> > +g_free(res);
> > +}
> > +
> > static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >  struct virtio_gpu_ctrl_command 
> > *cmd)
> > {
> > +struct virtio_gpu_simple_resource *res;
> > struct virtio_gpu_resource_unref unref;
> > struct iovec *res_iovs = NULL;
> > int num_iovs = 0;
> > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> > VIRTIO_GPU_FILL_CMD(unref);
> > trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> > 
> > +res = virtio_gpu_find_resource(g, 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);
> > +if (res) {
> > +res->iov = NULL;
> > +res->iov_cnt = 0;

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-05 Thread Akihiko Odaki 

On 2023/09/06 12:09, Huang Rui wrote:

On Tue, Sep 05, 2023 at 05:20:43PM +0800, Akihiko Odaki wrote:

On 2023/09/05 18:08, Huang Rui wrote:

On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:

On 2023/08/31 18:32, Huang Rui wrote:

From: Antonio Caggiano 

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 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

v1->v2:
   - 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 that will be
 released automatically once the memory region is unparented and all
 of its references have been released.
 Also, since the pointer can be used to indicate whether the blob
 is mapped, the explicit 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.

   - Remove direct references to parent_obj.

   - Separate declarations from code.

hw/display/virtio-gpu-virgl.c  | 213 +
hw/display/virtio-gpu.c|   4 +-
include/hw/virtio/virtio-gpu.h |   5 +
meson.build|   4 +
4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 312953ec16..17b634d4ee 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"

@@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,

virgl_renderer_resource_create(&args, NULL, 0);
}

+static void virgl_resource_destroy(VirtIOGPU *g,

+   struct virtio_gpu_simple_resource *res)
+{
+if (!res)
+return;
+
+QTAILQ_REMOVE(&g->reslist, res, next);
+
+virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+g_free(res->addrs);
+
+g_free(res);
+}
+
static void virgl_cmd_resource_unref(VirtIOGPU *g,
 struct virtio_gpu_ctrl_command *cmd)
{
+struct virtio_gpu_simple_resource *res;
struct virtio_gpu_resource_unref unref;
struct iovec *res_iovs = NULL;
int num_iovs = 0;
@@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
VIRTIO_GPU_FILL_CMD(unref);
trace_virtio_gpu_cmd_res_unref(unref.resource_id);

+res = virtio_gpu_find_resource(g, 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);
+if (res) {
+res->iov = NULL;
+res->iov_cnt = 0;
+}
}
+
virgl_renderer_resource_unref(unref.resource_id);
+
+virgl_resource_destroy(g, res);
}

static void virgl_cmd_context_create(VirtIOGPU *g,

@@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
g_free(resp);
}

+#ifdef HAVE_VIRGL_RESOURCE_BLOB

+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+   struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_simple_resource *res;
+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_I

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-05 Thread Huang Rui 
On Tue, Sep 05, 2023 at 05:20:43PM +0800, Akihiko Odaki wrote:
> On 2023/09/05 18:08, Huang Rui wrote:
> > On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:
> >> On 2023/08/31 18:32, Huang Rui wrote:
> >>> From: Antonio Caggiano 
> >>>
> >>> 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 
> >>> Signed-off-by: Dmitry Osipenko 
> >>> Signed-off-by: Xenia Ragiadakou 
> >>> Signed-off-by: Huang Rui 
> >>> ---
> >>>
> >>> v1->v2:
> >>>   - 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 that will be
> >>> released automatically once the memory region is unparented and 
> >>> all
> >>> of its references have been released.
> >>> Also, since the pointer can be used to indicate whether the blob
> >>> is mapped, the explicit 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.
> >>>
> >>>   - Remove direct references to parent_obj.
> >>>
> >>>   - Separate declarations from code.
> >>>
> >>>hw/display/virtio-gpu-virgl.c  | 213 +
> >>>hw/display/virtio-gpu.c|   4 +-
> >>>include/hw/virtio/virtio-gpu.h |   5 +
> >>>meson.build|   4 +
> >>>4 files changed, 225 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>> index 312953ec16..17b634d4ee 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"
> >>>
> >>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >>>virgl_renderer_resource_create(&args, NULL, 0);
> >>>}
> >>>
> >>> +static void virgl_resource_destroy(VirtIOGPU *g,
> >>> +   struct virtio_gpu_simple_resource 
> >>> *res)
> >>> +{
> >>> +if (!res)
> >>> +return;
> >>> +
> >>> +QTAILQ_REMOVE(&g->reslist, res, next);
> >>> +
> >>> +virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> >>> +g_free(res->addrs);
> >>> +
> >>> +g_free(res);
> >>> +}
> >>> +
> >>>static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>> struct virtio_gpu_ctrl_command 
> >>> *cmd)
> >>>{
> >>> +struct virtio_gpu_simple_resource *res;
> >>>struct virtio_gpu_resource_unref unref;
> >>>struct iovec *res_iovs = NULL;
> >>>int num_iovs = 0;
> >>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>>VIRTIO_GPU_FILL_CMD(unref);
> >>>trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >>>
> >>> +res = virtio_gpu_find_resource(g, 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);
> >>> +if (res) {
> >>> +res->iov = NULL;
> >>> +res->iov_cnt = 0;
> >>> +}
> >>>}
> >>> +
> >>>virgl_renderer_resource_unref(unref.resource_id);
> >>> +
> >>> +virgl_resource_destroy(g, res);
> >>>}
> >>>
> >>>static void virgl_cmd_context_create(VirtIOGPU *g,
> >>> @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >>>g_free(resp);
> >>>}
> >>>
> >>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> >>> +
> >>> +static void virgl_cmd_resource_create_blob(Vi

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-05 Thread Akihiko Odaki 

On 2023/09/05 18:08, Huang Rui wrote:

On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:

On 2023/08/31 18:32, Huang Rui wrote:

From: Antonio Caggiano 

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 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

v1->v2:
  - 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 that will be
released automatically once the memory region is unparented and all
of its references have been released.
Also, since the pointer can be used to indicate whether the blob
is mapped, the explicit 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.

  - Remove direct references to parent_obj.

  - Separate declarations from code.

   hw/display/virtio-gpu-virgl.c  | 213 +
   hw/display/virtio-gpu.c|   4 +-
   include/hw/virtio/virtio-gpu.h |   5 +
   meson.build|   4 +
   4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 312953ec16..17b634d4ee 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"
   
@@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,

   virgl_renderer_resource_create(&args, NULL, 0);
   }
   
+static void virgl_resource_destroy(VirtIOGPU *g,

+   struct virtio_gpu_simple_resource *res)
+{
+if (!res)
+return;
+
+QTAILQ_REMOVE(&g->reslist, res, next);
+
+virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+g_free(res->addrs);
+
+g_free(res);
+}
+
   static void virgl_cmd_resource_unref(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
   {
+struct virtio_gpu_simple_resource *res;
   struct virtio_gpu_resource_unref unref;
   struct iovec *res_iovs = NULL;
   int num_iovs = 0;
@@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
   VIRTIO_GPU_FILL_CMD(unref);
   trace_virtio_gpu_cmd_res_unref(unref.resource_id);
   
+res = virtio_gpu_find_resource(g, 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);
+if (res) {
+res->iov = NULL;
+res->iov_cnt = 0;
+}
   }
+
   virgl_renderer_resource_unref(unref.resource_id);
+
+virgl_resource_destroy(g, res);
   }
   
   static void virgl_cmd_context_create(VirtIOGPU *g,

@@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
   g_free(resp);
   }
   
+#ifdef HAVE_VIRGL_RESOURCE_BLOB

+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+   struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_simple_resource *res;
+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;
+}
+
+res = virtio_gpu_find_resource(g, cblob.resource_id);
+if (res) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource a

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-09-05 Thread Huang Rui 
On Thu, Aug 31, 2023 at 06:24:32PM +0800, Akihiko Odaki wrote:
> On 2023/08/31 18:32, Huang Rui wrote:
> > From: Antonio Caggiano 
> > 
> > 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 
> > Signed-off-by: Dmitry Osipenko 
> > Signed-off-by: Xenia Ragiadakou 
> > Signed-off-by: Huang Rui 
> > ---
> > 
> > v1->v2:
> >  - 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 that will be
> >released automatically once the memory region is unparented and all
> >of its references have been released.
> >Also, since the pointer can be used to indicate whether the blob
> >is mapped, the explicit 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.
> > 
> >  - Remove direct references to parent_obj.
> > 
> >  - Separate declarations from code.
> > 
> >   hw/display/virtio-gpu-virgl.c  | 213 +
> >   hw/display/virtio-gpu.c|   4 +-
> >   include/hw/virtio/virtio-gpu.h |   5 +
> >   meson.build|   4 +
> >   4 files changed, 225 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 312953ec16..17b634d4ee 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"
> >   
> > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >   virgl_renderer_resource_create(&args, NULL, 0);
> >   }
> >   
> > +static void virgl_resource_destroy(VirtIOGPU *g,
> > +   struct virtio_gpu_simple_resource *res)
> > +{
> > +if (!res)
> > +return;
> > +
> > +QTAILQ_REMOVE(&g->reslist, res, next);
> > +
> > +virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> > +g_free(res->addrs);
> > +
> > +g_free(res);
> > +}
> > +
> >   static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >struct virtio_gpu_ctrl_command *cmd)
> >   {
> > +struct virtio_gpu_simple_resource *res;
> >   struct virtio_gpu_resource_unref unref;
> >   struct iovec *res_iovs = NULL;
> >   int num_iovs = 0;
> > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >   VIRTIO_GPU_FILL_CMD(unref);
> >   trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >   
> > +res = virtio_gpu_find_resource(g, 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);
> > +if (res) {
> > +res->iov = NULL;
> > +res->iov_cnt = 0;
> > +}
> >   }
> > +
> >   virgl_renderer_resource_unref(unref.resource_id);
> > +
> > +virgl_resource_destroy(g, res);
> >   }
> >   
> >   static void virgl_cmd_context_create(VirtIOGPU *g,
> > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >   g_free(resp);
> >   }
> >   
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +
> > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> > +   struct virtio_gpu_ctrl_command 
> > *cmd)
> > +{
> > +struct virtio_gpu_simple_resource *res;
> > +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_re

Re: [QEMU PATCH v4 09/13] virtio-gpu: Handle resource blob commands

2023-08-31 Thread Akihiko Odaki 

On 2023/08/31 18:32, Huang Rui wrote:

From: Antonio Caggiano 

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 
Signed-off-by: Dmitry Osipenko 
Signed-off-by: Xenia Ragiadakou 
Signed-off-by: Huang Rui 
---

v1->v2:
 - 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 that will be
   released automatically once the memory region is unparented and all
   of its references have been released.
   Also, since the pointer can be used to indicate whether the blob
   is mapped, the explicit 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.

 - Remove direct references to parent_obj.

 - Separate declarations from code.

  hw/display/virtio-gpu-virgl.c  | 213 +
  hw/display/virtio-gpu.c|   4 +-
  include/hw/virtio/virtio-gpu.h |   5 +
  meson.build|   4 +
  4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 312953ec16..17b634d4ee 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"
  
@@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,

  virgl_renderer_resource_create(&args, NULL, 0);
  }
  
+static void virgl_resource_destroy(VirtIOGPU *g,

+   struct virtio_gpu_simple_resource *res)
+{
+if (!res)
+return;
+
+QTAILQ_REMOVE(&g->reslist, res, next);
+
+virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+g_free(res->addrs);
+
+g_free(res);
+}
+
  static void virgl_cmd_resource_unref(VirtIOGPU *g,
   struct virtio_gpu_ctrl_command *cmd)
  {
+struct virtio_gpu_simple_resource *res;
  struct virtio_gpu_resource_unref unref;
  struct iovec *res_iovs = NULL;
  int num_iovs = 0;
@@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
  VIRTIO_GPU_FILL_CMD(unref);
  trace_virtio_gpu_cmd_res_unref(unref.resource_id);
  
+res = virtio_gpu_find_resource(g, 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);
+if (res) {
+res->iov = NULL;
+res->iov_cnt = 0;
+}
  }
+
  virgl_renderer_resource_unref(unref.resource_id);
+
+virgl_resource_destroy(g, res);
  }
  
  static void virgl_cmd_context_create(VirtIOGPU *g,

@@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
  g_free(resp);
  }
  
+#ifdef HAVE_VIRGL_RESOURCE_BLOB

+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+   struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_simple_resource *res;
+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;
+}
+
+res = virtio_gpu_find_resource(g, cblob.resource_id);
+if (res) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+  __func__, cblob.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+return;
+}