virgl/vtest: add support to vtest for new cap getting.

Submitted by Dave Airlie on June 8, 2018, 6:22 a.m.

Details

Message ID 20180608062221.17608-1-airlied@gmail.com
State New
Series "virgl/vtest: add support to vtest for new cap getting."
Headers show

Commit Message

Dave Airlie June 8, 2018, 6:22 a.m.
From: Dave Airlie <airlied@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(-)

Patch hide | download patch | download mbox

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);
+
+       /* 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 */

Comments

Jakob Bornecrantz June 27, 2018, 5:25 p.m.
On 2018-06-08 07:22, Dave Airlie wrote:
> From: Dave Airlie <airlied@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 */
>
Dave Airlie June 27, 2018, 10:26 p.m.
On 28 June 2018 at 03:25, Jakob Bornecrantz <jakob@collabora.com> wrote:
> On 2018-06-08 07:22, Dave Airlie wrote:
>>
>> From: Dave Airlie <airlied@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).

We've pretty much fixed caps_v1 size for ever, the protocol won't return
anything other than the 308 byte struct we have now.

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

We don't really know what a block is, it's just a unix socket, if we find
ourselves doing this more then maybe a dummy refactor might be neeeded
but hopefully this is a once off bad protocol design fix. We may want a new
protocol here anyways that is more robust anyways.

Dave.
Jakob Bornecrantz June 28, 2018, 11:43 a.m.
On 2018-06-27 23:26, Dave Airlie wrote:
> On 28 June 2018 at 03:25, Jakob Bornecrantz <jakob@collabora.com> wrote:
>> On 2018-06-08 07:22, Dave Airlie wrote:
>>>
>>> From: Dave Airlie <airlied@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).
> 
> We've pretty much fixed caps_v1 size for ever, the protocol won't return
> anything other than the 308 byte struct we have now.

I may be wrong here, but isn't the "if (dummy_size)" read dealing with 
the case when the "struct virgl_caps_v2" has grown on the host but not 
the guest size? And has nothing to do with caps_v1 other then that is 
what dummy happens to be.

So in the case the host has extended the v2 caps struct with more then 
308 bytes and the driver hasn't been updated. Wont that cause the 
dummy_size to be larger then sizeof(struct virgl_caps_v1) and cause it 
to smash the stack? I mean it is doubtful to ever happen but it seems a 
bit of a repeat of what happened with the v1 to v2 switch.

> 
>>
>> Wouldn't it be better if we had a virgl_block_skip function?
> 
> We don't really know what a block is, it's just a unix socket, if we find
> ourselves doing this more then maybe a dummy refactor might be neeeded
> but hopefully this is a once off bad protocol design fix. We may want a new
> protocol here anyways that is more robust anyways.

Okay sounds good.

Cheers, Jakob.