1. Patchew
  2. QEMU
  3. [PATCH 00/10] virtio-sound migration part 1
  4. View patch

[PATCH 05/10] hw/audio/virtio-sound: return correct command response size

Volker Rümelin posted 10 patches 10 months, 3 weeks ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Manos Pitsidianakis <manos.pitsidianakis@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
[PATCH 05/10] hw/audio/virtio-sound: return correct command response size
Posted by Volker Rümelin 10 months, 3 weeks ago 
The payload size returned by command VIRTIO_SND_R_PCM_INFO is
wrong. The code in process_cmd() assumes that all commands
return only a virtio_snd_hdr payload, but some commands like
VIRTIO_SND_R_PCM_INFO may return an additional payload.

Add a zero initialized payload_size variable to struct
virtio_snd_ctrl_command to allow for additional payloads.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/audio/virtio-snd.c         | 7 +++++--
 include/hw/audio/virtio-snd.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a2817a64b5..9f3269d72b 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -262,12 +262,13 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
         memset(&pcm_info[i].padding, 0, 5);
     }
 
+    cmd->payload_size = sizeof(virtio_snd_pcm_info) * count;
     cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
     iov_from_buf(cmd->elem->in_sg,
                  cmd->elem->in_num,
                  sizeof(virtio_snd_hdr),
                  pcm_info,
-                 sizeof(virtio_snd_pcm_info) * count);
+                 cmd->payload_size);
 }
 
 /*
@@ -805,7 +806,8 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
                  0,
                  &cmd->resp,
                  sizeof(virtio_snd_hdr));
-    virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
+    virtqueue_push(cmd->vq, cmd->elem,
+                   sizeof(virtio_snd_hdr) + cmd->payload_size);
     virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
 }
 
@@ -856,6 +858,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         cmd->elem = elem;
         cmd->vq = vq;
         cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
+        /* implicit cmd->payload_size = 0; */
         QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next);
         elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
     }
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index d1a68444d0..d6e08bd30d 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -210,6 +210,7 @@ struct virtio_snd_ctrl_command {
     VirtQueue *vq;
     virtio_snd_hdr ctrl;
     virtio_snd_hdr resp;
+    size_t payload_size;
     QTAILQ_ENTRY(virtio_snd_ctrl_command) next;
 };
 #endif
-- 
2.35.3
Re: [PATCH 05/10] hw/audio/virtio-sound: return correct command response size
Posted by Marc-André Lureau 10 months, 3 weeks ago 
Hi

On Fri, Jan 5, 2024 at 12:34 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>
> The payload size returned by command VIRTIO_SND_R_PCM_INFO is
> wrong. The code in process_cmd() assumes that all commands
> return only a virtio_snd_hdr payload, but some commands like
> VIRTIO_SND_R_PCM_INFO may return an additional payload.
>
> Add a zero initialized payload_size variable to struct
> virtio_snd_ctrl_command to allow for additional payloads.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  hw/audio/virtio-snd.c         | 7 +++++--
>  include/hw/audio/virtio-snd.h | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index a2817a64b5..9f3269d72b 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -262,12 +262,13 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
>          memset(&pcm_info[i].padding, 0, 5);
>      }
>
> +    cmd->payload_size = sizeof(virtio_snd_pcm_info) * count;
>      cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
>      iov_from_buf(cmd->elem->in_sg,
>                   cmd->elem->in_num,
>                   sizeof(virtio_snd_hdr),
>                   pcm_info,
> -                 sizeof(virtio_snd_pcm_info) * count);
> +                 cmd->payload_size);

iov_from_buf() return value should probably be checked to match the
expected written size..

The earlier check for iov_size() is then probably redundant. Hmm, it
checks for req.size, which should probably match
sizeof(virtio_snd_pcm_info) too.

>  }
>
>  /*
> @@ -805,7 +806,8 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
>                   0,
>                   &cmd->resp,
>                   sizeof(virtio_snd_hdr));
> -    virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
> +    virtqueue_push(cmd->vq, cmd->elem,
> +                   sizeof(virtio_snd_hdr) + cmd->payload_size);
>      virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
>  }
>
> @@ -856,6 +858,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>          cmd->elem = elem;
>          cmd->vq = vq;
>          cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
> +        /* implicit cmd->payload_size = 0; */
>          QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next);
>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>      }
> diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
> index d1a68444d0..d6e08bd30d 100644
> --- a/include/hw/audio/virtio-snd.h
> +++ b/include/hw/audio/virtio-snd.h
> @@ -210,6 +210,7 @@ struct virtio_snd_ctrl_command {
>      VirtQueue *vq;
>      virtio_snd_hdr ctrl;
>      virtio_snd_hdr resp;
> +    size_t payload_size;
>      QTAILQ_ENTRY(virtio_snd_ctrl_command) next;
>  };
>  #endif
> --
> 2.35.3
>

otherwise, lgtm. Should it be backported to -stable ?
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Re: [PATCH 05/10] hw/audio/virtio-sound: return correct command response size
Posted by Volker Rümelin 9 months, 1 week ago 
Am 05.01.24 um 12:36 schrieb Marc-André Lureau:
> Hi
>
> On Fri, Jan 5, 2024 at 12:34 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>> The payload size returned by command VIRTIO_SND_R_PCM_INFO is
>> wrong. The code in process_cmd() assumes that all commands
>> return only a virtio_snd_hdr payload, but some commands like
>> VIRTIO_SND_R_PCM_INFO may return an additional payload.
>>
>> Add a zero initialized payload_size variable to struct
>> virtio_snd_ctrl_command to allow for additional payloads.
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>  hw/audio/virtio-snd.c         | 7 +++++--
>>  include/hw/audio/virtio-snd.h | 1 +
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
>> index a2817a64b5..9f3269d72b 100644
>> --- a/hw/audio/virtio-snd.c
>> +++ b/hw/audio/virtio-snd.c
>> @@ -262,12 +262,13 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
>>          memset(&pcm_info[i].padding, 0, 5);
>>      }
>>
>> +    cmd->payload_size = sizeof(virtio_snd_pcm_info) * count;
>>      cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
>>      iov_from_buf(cmd->elem->in_sg,
>>                   cmd->elem->in_num,
>>                   sizeof(virtio_snd_hdr),
>>                   pcm_info,
>> -                 sizeof(virtio_snd_pcm_info) * count);
>> +                 cmd->payload_size);
> iov_from_buf() return value should probably be checked to match the
> expected written size..
>
> The earlier check for iov_size() is then probably redundant. Hmm, it
> checks for req.size, which should probably match
> sizeof(virtio_snd_pcm_info) too.

I wouldn't like to change that in this patch. There are more places in
this device and also in other virtio devices where this is done in
exactly the same way.

>
>>  }
>>
>>  /*
>> @@ -805,7 +806,8 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
>>                   0,
>>                   &cmd->resp,
>>                   sizeof(virtio_snd_hdr));
>> -    virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
>> +    virtqueue_push(cmd->vq, cmd->elem,
>> +                   sizeof(virtio_snd_hdr) + cmd->payload_size);
>>      virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
>>  }
>>
>> @@ -856,6 +858,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>>          cmd->elem = elem;
>>          cmd->vq = vq;
>>          cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
>> +        /* implicit cmd->payload_size = 0; */
>>          QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next);
>>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>>      }
>> diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
>> index d1a68444d0..d6e08bd30d 100644
>> --- a/include/hw/audio/virtio-snd.h
>> +++ b/include/hw/audio/virtio-snd.h
>> @@ -210,6 +210,7 @@ struct virtio_snd_ctrl_command {
>>      VirtQueue *vq;
>>      virtio_snd_hdr ctrl;
>>      virtio_snd_hdr resp;
>> +    size_t payload_size;
>>      QTAILQ_ENTRY(virtio_snd_ctrl_command) next;
>>  };
>>  #endif
>> --
>> 2.35.3
>>
> otherwise, lgtm. Should it be backported to -stable ?

Yes, I will cc qemu-stable in my v2 series. While the Linux virtio sound
driver ignores the returned "used length", this is required by the
virtio specification.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.