[Mesa-dev] [virglrenderer-devel] [PATCH] virgl/vtest: add support to vtest for new cap getting.

[Jakob Bornecrantz]

On 2018-06-08 07:22, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> The vtest protocol is pretty simple but also pretty dumb, and
> the v1 caps query was fixed size, with no nice way to expand it,
> however the server also ignores any command it doesn't understand.
> 
> So we can query v2 caps by sending a v2 followed by a v1, if the
> v2 is ignored we know it's an old vtest server, and the we get
> a v2 answer then we can just read the v1 answer and discard it.
> ---
>   .../winsys/virgl/vtest/virgl_vtest_socket.c        | 30 +++++++++++++++++++---
>   src/gallium/winsys/virgl/vtest/vtest_protocol.h    |  2 ++
>   2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c b/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
> index adec26b66b8..d25f9a3bd9e 100644
> --- a/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
> +++ b/src/gallium/winsys/virgl/vtest/virgl_vtest_socket.c
> @@ -129,12 +129,14 @@ int virgl_vtest_connect(struct virgl_vtest_winsys *vws)
>   int virgl_vtest_send_get_caps(struct virgl_vtest_winsys *vws,
>                                 struct virgl_drm_caps *caps)
>   {
> -   uint32_t get_caps_buf[VTEST_HDR_SIZE];
> +   uint32_t get_caps_buf[VTEST_HDR_SIZE * 2];
>      uint32_t resp_buf[VTEST_HDR_SIZE];
> -
> +   uint32_t caps_size = sizeof(struct virgl_caps_v2);
>      int ret;
>      get_caps_buf[VTEST_CMD_LEN] = 0;
> -   get_caps_buf[VTEST_CMD_ID] = VCMD_GET_CAPS;
> +   get_caps_buf[VTEST_CMD_ID] = VCMD_GET_CAPS2;
> +   get_caps_buf[VTEST_CMD_LEN + 2] = 0;
> +   get_caps_buf[VTEST_CMD_ID + 2] = VCMD_GET_CAPS;
>   
>      virgl_block_write(vws->sock_fd, &get_caps_buf, sizeof(get_caps_buf));
>   
> @@ -142,7 +144,27 @@ int virgl_vtest_send_get_caps(struct virgl_vtest_winsys *vws,
>      if (ret <= 0)
>         return 0;
>   
> -   ret = virgl_block_read(vws->sock_fd, &caps->caps, sizeof(struct virgl_caps_v1));
> +   if (resp_buf[1] == 2) {
> +       struct virgl_caps_v1 dummy;
> +       uint32_t resp_size = resp_buf[0] - 1;
> +       uint32_t dummy_size = 0;
> +       if (resp_size > caps_size) {
> +	   dummy_size = resp_size - caps_size;
> +	   resp_size = caps_size;
> +       }
> +
> +       ret = virgl_block_read(vws->sock_fd, &caps->caps, resp_size);
> +
> +       if (dummy_size)
> +	   ret = virgl_block_read(vws->sock_fd, &dummy, dummy_size);

Isn't there a risk that the "dummy_size" is larger then the struct 
virgl_caps_v1 dummy and cause it to write over the stack? (I am assuming 
you are using the dummy here as a place to put the extra caps the host 
is exposing but the driver isn't supporting).

Wouldn't it be better if we had a virgl_block_skip function?

Cheers, Jakob.

> +
> +       /* now read back the pointless caps v1 we requested */
> +       ret = virgl_block_read(vws->sock_fd, resp_buf, sizeof(resp_buf));
> +       if (ret <= 0)
> +	   return 0;
> +       ret = virgl_block_read(vws->sock_fd, &dummy, sizeof(struct virgl_caps_v1));
> +   } else
> +       ret = virgl_block_read(vws->sock_fd, &caps->caps, sizeof(struct virgl_caps_v1));
>   
>      return 0;
>   }
> diff --git a/src/gallium/winsys/virgl/vtest/vtest_protocol.h b/src/gallium/winsys/virgl/vtest/vtest_protocol.h
> index 86d197f006c..95bd8c1d0bd 100644
> --- a/src/gallium/winsys/virgl/vtest/vtest_protocol.h
> +++ b/src/gallium/winsys/virgl/vtest/vtest_protocol.h
> @@ -47,6 +47,8 @@
>   
>   /* pass the process cmd line for debugging */
>   #define VCMD_CREATE_RENDERER 8
> +
> +#define VCMD_GET_CAPS2 9
>   /* get caps */
>   /* 0 length cmd */
>   /* resp VCMD_GET_CAPS + caps */
>