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

[Dave Airlie]

On 28 June 2018 at 03:25, Jakob Bornecrantz <jakob at collabora.com> wrote:
> 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).

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.