virtio-sound linux driver conformance to spec

virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

Hello,

This email is to report a behavior of the Linux virtio-sound driver that
looks like it is not conforming to the VirtIO specification. The kernel
driver is moving buffers from the used ring to the available ring
without knowing if the content has been updated from the user. If the
device picks up buffers from the available ring just after it is
notified, it happens that the content is old. This problem can be fixed
by waiting a period of time after the device dequeues a buffer from the
available ring. The driver should not be allowed to change the content
of a buffer in the available ring. When buffers are enqueued in the
available ring, the device can consume them immediately.

1. Is the actual driver implementation following the spec?  
2. Shall the spec be extended to support such a use-case?

Thanks, Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] virtio-sound linux driver conformance to spec

[Paolo Bonzini]

On Wed, Sep 13, 2023 at 5:05 PM Matias Ezequiel Vara Larsen
<mvaralar@redhat.com> wrote:
>
> Hello,
>
> This email is to report a behavior of the Linux virtio-sound driver that
> looks like it is not conforming to the VirtIO specification. The kernel
> driver is moving buffers from the used ring to the available ring
> without knowing if the content has been updated from the user. If the
> device picks up buffers from the available ring just after it is
> notified, it happens that the content is old. This problem can be fixed
> by waiting a period of time after the device dequeues a buffer from the
> available ring. The driver should not be allowed to change the content
> of a buffer in the available ring. When buffers are enqueued in the
> available ring, the device can consume them immediately.
>
> 1. Is the actual driver implementation following the spec?

If I understand the issue correctly, it's not. As you say, absent any
clarification a buffer that has been placed in the available ring
should be unmodifiable; the driver should operate as if the data in
the available ring is copied to an internal buffer instantly when the
buffer id is added to the ring.

I am assuming this is a playback buffer. To clarify, does the driver
expect buffers to be read only as needed, which is a fraction of a
second before the data is played back?

> 2. Shall the spec be extended to support such a use-case?

If it places N buffers, I think it's a reasonable expectation (for the
sound device only!) that the Nth isn't read until the (N-1)th has
started playing. With two buffers only, the behavior you specify would
not be permissible (because as soon as buffer 1 starts playing, the
device can read buffer 2; there is never an idle buffer). With three
buffers, you could write buffer 3 while buffer 1 plays; write buffer 1
while buffer 2 plays; and write buffer 2 while buffer 3 plays. Is this
enough?

That said, being reasonable isn't enough for virtio-sound to do it and
deviate from other virtio devices. Why does the Linux driver behave
like this? Is it somehow constrained by the kernel->userspace APIs?

Paolo

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Wed, Sep 13, 2023 at 05:50:48PM +0200, Paolo Bonzini wrote:
> On Wed, Sep 13, 2023 at 5:05 PM Matias Ezequiel Vara Larsen
> <mvaralar@redhat.com> wrote:
> >
> > Hello,
> >
> > This email is to report a behavior of the Linux virtio-sound driver that
> > looks like it is not conforming to the VirtIO specification. The kernel
> > driver is moving buffers from the used ring to the available ring
> > without knowing if the content has been updated from the user. If the
> > device picks up buffers from the available ring just after it is
> > notified, it happens that the content is old. This problem can be fixed
> > by waiting a period of time after the device dequeues a buffer from the
> > available ring. The driver should not be allowed to change the content
> > of a buffer in the available ring. When buffers are enqueued in the
> > available ring, the device can consume them immediately.
> >
> > 1. Is the actual driver implementation following the spec?
> 
> If I understand the issue correctly, it's not. As you say, absent any
> clarification a buffer that has been placed in the available ring
> should be unmodifiable; the driver should operate as if the data in
> the available ring is copied to an internal buffer instantly when the
> buffer id is added to the ring.
> 
> I am assuming this is a playback buffer. To clarify, does the driver
> expect buffers to be read only as needed, which is a fraction of a
> second before the data is played back?
> 
The driver expects that device to read buffers a fraction of a second
before playing them back. If the device reads it just when they are
exposed in the available ring, the content is old. The device has to
read it just when the audio engine is going to consume it.

> > 2. Shall the spec be extended to support such a use-case?
> 
> If it places N buffers, I think it's a reasonable expectation (for the
> sound device only!) that the Nth isn't read until the (N-1)th has
> started playing. With two buffers only, the behavior you specify would
> not be permissible (because as soon as buffer 1 starts playing, the
> device can read buffer 2; there is never an idle buffer). With three
> buffers, you could write buffer 3 while buffer 1 plays; write buffer 1
> while buffer 2 plays; and write buffer 2 while buffer 3 plays. Is this
> enough?
> 
> That said, being reasonable isn't enough for virtio-sound to do it and
> deviate from other virtio devices. Why does the Linux driver behave
> like this? Is it somehow constrained by the kernel->userspace APIs?
> 
AFAIU, the driver sends four requests before starting playing, e.g.,
aplay 'FrontLeft.wav', each with PERIOD_SIZE bytes. PERIOD_SIZE is
negotiated between the device and the driver before playback begins.
The requests are split into multiple buffers.  After a PERIOD_SIZE is
played, the device notifies the guest.  These buffers are part of a ring
buffer shared with the user application. The device just picks the last
used set of buffers and enqueues again in the available ring. For
example, in the case of 4 requests of PERIOD_SIZE bytes each, the
mechanism is roughly as follows. The driver pre-buffers 4 requests. When
it starts to play, the device starts with [0]. After [0] is played, it
adds this request to the used ring and it picks up [1] for playing. The
driver gets the notification that [0] has been consumed, and moves the
request to the available ring thus notifying the device. At this point,
the device should not get the content of [0] since it is still old. The
content shall be read only BEFORE [0] is consumed again, e.g., after 4
periods.

Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Mon, Sep 18, 2023 at 1:13 PM Matias Ezequiel Vara Larsen
<mvaralar@redhat.com> wrote:
>
> On Wed, Sep 13, 2023 at 05:50:48PM +0200, Paolo Bonzini wrote:
> > On Wed, Sep 13, 2023 at 5:05 PM Matias Ezequiel Vara Larsen
> > <mvaralar@redhat.com> wrote:
> > >
> > > Hello,
> > >
> > > This email is to report a behavior of the Linux virtio-sound driver that
> > > looks like it is not conforming to the VirtIO specification. The kernel
> > > driver is moving buffers from the used ring to the available ring
> > > without knowing if the content has been updated from the user. If the
> > > device picks up buffers from the available ring just after it is
> > > notified, it happens that the content is old. This problem can be fixed
> > > by waiting a period of time after the device dequeues a buffer from the
> > > available ring. The driver should not be allowed to change the content
> > > of a buffer in the available ring. When buffers are enqueued in the
> > > available ring, the device can consume them immediately.
> > >
> > > 1. Is the actual driver implementation following the spec?
> >
> > If I understand the issue correctly, it's not. As you say, absent any
> > clarification a buffer that has been placed in the available ring
> > should be unmodifiable; the driver should operate as if the data in
> > the available ring is copied to an internal buffer instantly when the
> > buffer id is added to the ring.
> >
> > I am assuming this is a playback buffer. To clarify, does the driver
> > expect buffers to be read only as needed, which is a fraction of a
> > second before the data is played back?
> >
> The driver expects that device to read buffers a fraction of a second
> before playing them back. If the device reads it just when they are
> exposed in the available ring, the content is old. The device has to
> read it just when the audio engine is going to consume it.
>
> > > 2. Shall the spec be extended to support such a use-case?
> >
> > If it places N buffers, I think it's a reasonable expectation (for the
> > sound device only!) that the Nth isn't read until the (N-1)th has
> > started playing. With two buffers only, the behavior you specify would
> > not be permissible (because as soon as buffer 1 starts playing, the
> > device can read buffer 2; there is never an idle buffer). With three
> > buffers, you could write buffer 3 while buffer 1 plays; write buffer 1
> > while buffer 2 plays; and write buffer 2 while buffer 3 plays. Is this
> > enough?
> >
> > That said, being reasonable isn't enough for virtio-sound to do it and
> > deviate from other virtio devices. Why does the Linux driver behave
> > like this? Is it somehow constrained by the kernel->userspace APIs?
> >
> AFAIU, the driver sends four requests before starting playing, e.g.,
> aplay 'FrontLeft.wav', each with PERIOD_SIZE bytes. PERIOD_SIZE is
> negotiated between the device and the driver before playback begins.
> The requests are split into multiple buffers.  After a PERIOD_SIZE is
> played, the device notifies the guest.  These buffers are part of a ring
> buffer shared with the user application.

I mean the user application in the guest.

> The device just picks the last
> used set of buffers and enqueues again in the available ring. For

In this sentence, I mean the driver, not the device.

Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Wed, Sep 13, 2023 at 06:58:30PM +0300, Manos Pitsidianakis wrote:
> Hello Matias,
> 
> Please show and refer to code snippets from the kernel tree that you
> think are related to your question. It'd help us make sure we all talk
> about the same thing.
> 

In this discussion, I am referring to the way in which the virtio-sound
driver is manipulating buffers that have been consumed by the device,
e.g., used-ring in the tx queue. My understanding is the driver builds a
ring-buffer that is shared with the user application in the guest. As
soon as the device returns a buffer to the used ring, the driver puts
the request in the available ring again. This is my understanding from
sound/virtio/virtio_pcm_msg.c#L324. The user application updates the
content of the buffer at sound/virtio/virtio_pcm_msg.c#L322, but this
task is deferred by using schedule_work(). The update of the buffer may
happen once the buffers are already in the available ring.

Thanks, Matias.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] virtio-sound linux driver conformance to spec

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1572 bytes --]

On Mon, Sep 18, 2023 at 02:50:09PM +0200, Matias Ezequiel Vara Larsen wrote:
> On Wed, Sep 13, 2023 at 06:58:30PM +0300, Manos Pitsidianakis wrote:
> > Hello Matias,
> > 
> > Please show and refer to code snippets from the kernel tree that you
> > think are related to your question. It'd help us make sure we all talk
> > about the same thing.
> > 
> 
> In this discussion, I am referring to the way in which the virtio-sound
> driver is manipulating buffers that have been consumed by the device,
> e.g., used-ring in the tx queue. My understanding is the driver builds a
> ring-buffer that is shared with the user application in the guest. As
> soon as the device returns a buffer to the used ring, the driver puts
> the request in the available ring again. This is my understanding from
> sound/virtio/virtio_pcm_msg.c#L324. The user application updates the
> content of the buffer at sound/virtio/virtio_pcm_msg.c#L322, but this
> task is deferred by using schedule_work(). The update of the buffer may
> happen once the buffers are already in the available ring.

The driver cannot rely on the device accessing the buffer via shared
memory at a specific time. The device may process the buffer as soon as
the driver marks the buffer available and/or the buffer may not be in
shared memory (there is a discussion about virtio over TCP).

I haven't looked at the code myself, but based on your interpretation it
seems the driver is buggy. Buffers should only be submitted when the
buffer contents are no longer subject to change.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev via Virtualization]

Hello Matias,

On 14.09.2023 00:04, Matias Ezequiel Vara Larsen wrote:
> Hello,
> 
> This email is to report a behavior of the Linux virtio-sound driver that
> looks like it is not conforming to the VirtIO specification. The kernel
> driver is moving buffers from the used ring to the available ring
> without knowing if the content has been updated from the user. If the
> device picks up buffers from the available ring just after it is
> notified, it happens that the content is old. This problem can be fixed
> by waiting a period of time after the device dequeues a buffer from the
> available ring. The driver should not be allowed to change the content
> of a buffer in the available ring. When buffers are enqueued in the
> available ring, the device can consume them immediately.
> 
> 1. Is the actual driver implementation following the spec?

If the Linux virtio sound driver violates a specification, then there must be
a conformance statement that the driver does not follow. As far as I know,
there is no such thing at the moment.

During the design discussion, it was decided that the driver should implement
typical cyclic DMA logic. And the main idea behind was that, unlike many other
virtio devices, the sound device is isochronous. It consumes or supplies data
at a fixed rate. Which means that the device (in an idealized case) should
access buffers in virtqueues not as soon as they are available, but when
required. Whether this is a good idea or not is a debatable question.


> 2. Shall the spec be extended to support such a use-case?

There are some things you can try without having to change the spec.

The driver now handles RW and MMAP substream access modes in the same way.
Someone could try to change the handling of RW mode exactly as you describe,
because the UAPI allows it. But this logic cannot be reliably applied to MMAP
mode.


Best regards,
-- 
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec

[Paolo Bonzini]

On 9/19/23 02:35, Anton Yakovlev wrote:
> 
> If the Linux virtio sound driver violates a specification, then there 
> must be
> a conformance statement that the driver does not follow. As far as I know,
> there is no such thing at the moment.

There is one in 2.7.13.3: "The device MAY access the descriptor chains 
the driver created and the memory they refer to immediately"

And likewise for packed virtqueues in 2.8.21.1: "The device MAY access 
the descriptor and any following descriptors the driver created and the 
memory they refer to immediately"

I think it's a mistake to use MAY here, as opposed to "may".  This is 
not an optional feature, it's a MUST NOT requirement on the driver's 
part that should be in 2.7.13.3.1 and 2.8.21.1.1.

This does not prevent the virtio-snd spec from overriding this.  If an 
override is desirable (for example because other hardware behaves like 
this), there should be a provision in 2.7.13.3.1 and 2.8.21.1.1.  For 
example:

2.7.13.3.1 Unless the device specification specifies otherwise, the 
driver MUST NOT write to the descriptor chains and the memory they refer 
to, between the /idx/ update and the time the device places the driver 
on the used ring.

2.8.21.1.1 "Unless the device specification specifies otherwise, the 
driver MUST NOT write to the descriptor, to any following descriptors 
the driver created, nor to the memory the refer to, between the /flags/ 
update and the time the device places the driver on the used ring.


In the virtio-snd there would be a normative statement like

5.14.6.8.1.1  The device MUST NOT read from available device-readable 
buffers beyond the first buffer_bytes / period_bytes periods.

5.14.6.8.1.2  The driver MAY write to device-readable buffers beyond the 
first buffer_bytes / period_bytes periods, even after offering them to 
the device.



As an aside, here are two other statements that have a similar issue:

- 2.6.1.1.2 "the driver MAY release any resource associated with that
virtqueue" (instead 2.6.1.1.1 should have something like "After a queue 
has been reset by the driver, the device MUST NOT access any resource 
associated with a virtqueue").

- 2.7.5.1 "[the device] MAY do so for debugging or diagnostic purposes" 
(this is not normative and can be just "may")


Thanks,

Paolo

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Michael S. Tsirkin]

On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
> Hello,
> 
> This email is to report a behavior of the Linux virtio-sound driver that
> looks like it is not conforming to the VirtIO specification. The kernel
> driver is moving buffers from the used ring to the available ring
> without knowing if the content has been updated from the user. If the
> device picks up buffers from the available ring just after it is
> notified, it happens that the content is old.

Then, what happens, exactly? Do things still work?

> This problem can be fixed
> by waiting a period of time after the device dequeues a buffer from the
> available ring. The driver should not be allowed to change the content
> of a buffer in the available ring. When buffers are enqueued in the
> available ring, the device can consume them immediately.
> 
> 1. Is the actual driver implementation following the spec?  
> 2. Shall the spec be extended to support such a use-case?
> 
> Thanks, Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
> On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
> > Hello,
> > 
> > This email is to report a behavior of the Linux virtio-sound driver that
> > looks like it is not conforming to the VirtIO specification. The kernel
> > driver is moving buffers from the used ring to the available ring
> > without knowing if the content has been updated from the user. If the
> > device picks up buffers from the available ring just after it is
> > notified, it happens that the content is old.
> 
> Then, what happens, exactly? Do things still work?

We are currently developing a vhost-user backend for virtio-sound and
what happens is that if the backend implementation decides to copy the
content of a buffer from a request that just arrived to the available
ring, it gets the old content thus reproducing some sections two times.
For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
`Front, front left...`. To fix this issue, our current implementation
delays reading from guest memory just until the audio engine requires.
However, the first implementation shall also work since it is conforming
to the specification.

Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 3044 bytes --]

On Tue, Sep 19, 2023 at 08:58:32AM +0200, Paolo Bonzini wrote:
> On 9/19/23 02:35, Anton Yakovlev wrote:
> > 
> > If the Linux virtio sound driver violates a specification, then there
> > must be
> > a conformance statement that the driver does not follow. As far as I know,
> > there is no such thing at the moment.
> 
> There is one in 2.7.13.3: "The device MAY access the descriptor chains the
> driver created and the memory they refer to immediately"
> 
> And likewise for packed virtqueues in 2.8.21.1: "The device MAY access the
> descriptor and any following descriptors the driver created and the memory
> they refer to immediately"
> 
> I think it's a mistake to use MAY here, as opposed to "may".  This is not an
> optional feature, it's a MUST NOT requirement on the driver's part that
> should be in 2.7.13.3.1 and 2.8.21.1.1.
> 
> This does not prevent the virtio-snd spec from overriding this.  If an
> override is desirable (for example because other hardware behaves like
> this), there should be a provision in 2.7.13.3.1 and 2.8.21.1.1.  For
> example:
> 
> 2.7.13.3.1 Unless the device specification specifies otherwise, the driver
> MUST NOT write to the descriptor chains and the memory they refer to,
> between the /idx/ update and the time the device places the driver on the
> used ring.
> 
> 2.8.21.1.1 "Unless the device specification specifies otherwise, the driver
> MUST NOT write to the descriptor, to any following descriptors the driver
> created, nor to the memory the refer to, between the /flags/ update and the
> time the device places the driver on the used ring.
> 
> 
> In the virtio-snd there would be a normative statement like
> 
> 5.14.6.8.1.1  The device MUST NOT read from available device-readable
> buffers beyond the first buffer_bytes / period_bytes periods.
> 
> 5.14.6.8.1.2  The driver MAY write to device-readable buffers beyond the
> first buffer_bytes / period_bytes periods, even after offering them to the
> device.
> 
> 
> 
> As an aside, here are two other statements that have a similar issue:
> 
> - 2.6.1.1.2 "the driver MAY release any resource associated with that
> virtqueue" (instead 2.6.1.1.1 should have something like "After a queue has
> been reset by the driver, the device MUST NOT access any resource associated
> with a virtqueue").
> 
> - 2.7.5.1 "[the device] MAY do so for debugging or diagnostic purposes"
> (this is not normative and can be just "may")

The spec should not make an exception for virtio-sound because the
virtqueue model was not intended as a shared memory mechanism. Allowing
it would prevent message-passing implementations of virtqueues.

Instead the device should use Shared Memory Regions:
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10200010

BTW, the virtio-sound spec already has VIRTIO_SND_PCM_F_SHMEM_HOST and
VIRTIO_SND_PCM_F_SHMEM_GUEST bits reserved but they currently have no
meaning. I wonder what that was intended for?

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Michael S. Tsirkin]

On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
> On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > Hello,
> > > 
> > > This email is to report a behavior of the Linux virtio-sound driver that
> > > looks like it is not conforming to the VirtIO specification. The kernel
> > > driver is moving buffers from the used ring to the available ring
> > > without knowing if the content has been updated from the user. If the
> > > device picks up buffers from the available ring just after it is
> > > notified, it happens that the content is old.
> > 
> > Then, what happens, exactly? Do things still work?
> 
> We are currently developing a vhost-user backend for virtio-sound and
> what happens is that if the backend implementation decides to copy the
> content of a buffer from a request that just arrived to the available
> ring, it gets the old content thus reproducing some sections two times.
> For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
> `Front, front left...`. To fix this issue, our current implementation
> delays reading from guest memory just until the audio engine requires.
> However, the first implementation shall also work since it is conforming
> to the specification.
> 
> Matias

Sounds like it. How hard is it to change the behaviour though?
Does it involve changing userspace?
Maybe we need to fix the spec after all...

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Tue, Sep 19, 2023 at 11:52:27AM -0400, Michael S. Tsirkin wrote:
> On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
> > On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > > Hello,
> > > > 
> > > > This email is to report a behavior of the Linux virtio-sound driver that
> > > > looks like it is not conforming to the VirtIO specification. The kernel
> > > > driver is moving buffers from the used ring to the available ring
> > > > without knowing if the content has been updated from the user. If the
> > > > device picks up buffers from the available ring just after it is
> > > > notified, it happens that the content is old.
> > > 
> > > Then, what happens, exactly? Do things still work?
> > 
> > We are currently developing a vhost-user backend for virtio-sound and
> > what happens is that if the backend implementation decides to copy the
> > content of a buffer from a request that just arrived to the available
> > ring, it gets the old content thus reproducing some sections two times.
> > For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
> > `Front, front left...`. To fix this issue, our current implementation
> > delays reading from guest memory just until the audio engine requires.
> > However, the first implementation shall also work since it is conforming
> > to the specification.
> > 
> > Matias
> 
> Sounds like it. How hard is it to change the behaviour though?
> Does it involve changing userspace?

AFAIU, a fix for the driver may be to somehow wait until userspace
updates the buffer before add it in the available ring.  
So far, when the device notifies the driver that a new buffer is in the
used ring, the driver calls the virtsnd_pcm_msg_complete() function to
do:
``` 
schedule_work(&vss->elapsed_period);

virtsnd_pcm_msg_send(vss); 
``` 
It first defers the notification to userspace regarding an elapse period
and then it enqueues the request again in the available
ring.`schedule_work()` defers the calling to the
`virtsnd_pcm_period_elapsed()` function that issues
`snd_pcm_period_elapsed(vss->substream);` to notify userspace.  
My proposal would be that the driver could also defer
`virtsnd_pcm_msg_send(vss)` to execute just after
`snd_pcm_period_elapsed(vss->substream)`. Something like this:

diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
index c10d91fff2fb..41f1e74c8478 100644
--- a/sound/virtio/virtio_pcm.c
+++ b/sound/virtio/virtio_pcm.c
@@ -309,6 +309,7 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
                container_of(work, struct virtio_pcm_substream, elapsed_period);
 
        snd_pcm_period_elapsed(vss->substream);
+       virtsnd_pcm_msg_send(vss);
 }
 
 /**
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index aca2dc1989ba..43f0078b1152 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -321,7 +321,6 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
 
                schedule_work(&vss->elapsed_period);
 
-               virtsnd_pcm_msg_send(vss);
        } else if (!vss->msg_count) {
                wake_up_all(&vss->msg_empty);
        }


I tested it and it looks it fixes the issue. However, I am not sure if
this is enough since I do not know if when `snd_pcm_period_elapsed()`
returns, the buffers have been already updated.

Matias

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev via Virtualization]

Hello Paolo,

On 19.09.2023 15:58, Paolo Bonzini wrote:
> On 9/19/23 02:35, Anton Yakovlev wrote:
>>
>> If the Linux virtio sound driver violates a specification, then there must be
>> a conformance statement that the driver does not follow. As far as I know,
>> there is no such thing at the moment.
> 
> There is one in 2.7.13.3: "The device MAY access the descriptor chains the 
> driver created and the memory they refer to immediately"
> 
> And likewise for packed virtqueues in 2.8.21.1: "The device MAY access the 
> descriptor and any following descriptors the driver created and the memory 
> they refer to immediately"
> 
> I think it's a mistake to use MAY here, as opposed to "may".  This is not an 
> optional feature, it's a MUST NOT requirement on the driver's part that should 
> be in 2.7.13.3.1 and 2.8.21.1.1.
> 
> This does not prevent the virtio-snd spec from overriding this.  If an 
> override is desirable (for example because other hardware behaves like this), 
> there should be a provision in 2.7.13.3.1 and 2.8.21.1.1.  For example:
> 
> 2.7.13.3.1 Unless the device specification specifies otherwise, the driver 
> MUST NOT write to the descriptor chains and the memory they refer to, between 
> the /idx/ update and the time the device places the driver on the used ring.
> 
> 2.8.21.1.1 "Unless the device specification specifies otherwise, the driver 
> MUST NOT write to the descriptor, to any following descriptors the driver 
> created, nor to the memory the refer to, between the /flags/ update and the 
> time the device places the driver on the used ring.
> 
> 
> In the virtio-snd there would be a normative statement like
> 
> 5.14.6.8.1.1  The device MUST NOT read from available device-readable buffers 
> beyond the first buffer_bytes / period_bytes periods.
> 
> 5.14.6.8.1.2  The driver MAY write to device-readable buffers beyond the first 
> buffer_bytes / period_bytes periods, even after offering them to the device.

Thanks for pointing this out.

Yes, it looks like the driver does not strictly follow the specification. But
in this case it's a matter of driver implementation in the Linux kernel, so I
don’t think there is a need to change the spec.


-- 
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev via Virtualization]

Hello Stefan,

On 20.09.2023 00:10, Stefan Hajnoczi wrote:

>> As an aside, here are two other statements that have a similar issue:
>>
>> - 2.6.1.1.2 "the driver MAY release any resource associated with that
>> virtqueue" (instead 2.6.1.1.1 should have something like "After a queue has
>> been reset by the driver, the device MUST NOT access any resource associated
>> with a virtqueue").
>>
>> - 2.7.5.1 "[the device] MAY do so for debugging or diagnostic purposes"
>> (this is not normative and can be just "may")
> 
> The spec should not make an exception for virtio-sound because the
> virtqueue model was not intended as a shared memory mechanism. Allowing
> it would prevent message-passing implementations of virtqueues.
> 
> Instead the device should use Shared Memory Regions:
> https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10200010
> 
> BTW, the virtio-sound spec already has VIRTIO_SND_PCM_F_SHMEM_HOST and
> VIRTIO_SND_PCM_F_SHMEM_GUEST bits reserved but they currently have no
> meaning. I wonder what that was intended for?

In the original version of the design it was proposed to use shared memory for
the buffer. But since not all architectures allow the use of shared memory, it
was decided to make message-passing the basis. For shared memory, stream
features were reserved for further work on the spec.


-- 
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev via Virtualization]

Hello Michael,

On 20.09.2023 00:52, Michael S. Tsirkin wrote:
> On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
>> On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
>>> On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
>>>> Hello,
>>>>
>>>> This email is to report a behavior of the Linux virtio-sound driver that
>>>> looks like it is not conforming to the VirtIO specification. The kernel
>>>> driver is moving buffers from the used ring to the available ring
>>>> without knowing if the content has been updated from the user. If the
>>>> device picks up buffers from the available ring just after it is
>>>> notified, it happens that the content is old.
>>>
>>> Then, what happens, exactly? Do things still work?
>>
>> We are currently developing a vhost-user backend for virtio-sound and
>> what happens is that if the backend implementation decides to copy the
>> content of a buffer from a request that just arrived to the available
>> ring, it gets the old content thus reproducing some sections two times.
>> For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
>> `Front, front left...`. To fix this issue, our current implementation
>> delays reading from guest memory just until the audio engine requires.
>> However, the first implementation shall also work since it is conforming
>> to the specification.
>>
>> Matias
> 
> Sounds like it. How hard is it to change the behaviour though?
> Does it involve changing userspace?
> Maybe we need to fix the spec after all...

Fixing the problem Matias described only requires changes to the driver. But
we will need to restrict applications from mmap()'ing the buffer. Applications
will be able to read/write frames only through ioctl() requests.

I think we could expand the sound specification to add support for shared
memory. Then it should be possible to implement mmap() support on top of it.


-- 
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Anton Yakovlev via Virtualization]

Hello Matias,

On 20.09.2023 22:18, Matias Ezequiel Vara Larsen wrote:
> On Tue, Sep 19, 2023 at 11:52:27AM -0400, Michael S. Tsirkin wrote:
>> On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
>>> On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
>>>> On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
>>>>> Hello,
>>>>>
>>>>> This email is to report a behavior of the Linux virtio-sound driver that
>>>>> looks like it is not conforming to the VirtIO specification. The kernel
>>>>> driver is moving buffers from the used ring to the available ring
>>>>> without knowing if the content has been updated from the user. If the
>>>>> device picks up buffers from the available ring just after it is
>>>>> notified, it happens that the content is old.
>>>>
>>>> Then, what happens, exactly? Do things still work?
>>>
>>> We are currently developing a vhost-user backend for virtio-sound and
>>> what happens is that if the backend implementation decides to copy the
>>> content of a buffer from a request that just arrived to the available
>>> ring, it gets the old content thus reproducing some sections two times.
>>> For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
>>> `Front, front left...`. To fix this issue, our current implementation
>>> delays reading from guest memory just until the audio engine requires.
>>> However, the first implementation shall also work since it is conforming
>>> to the specification.
>>>
>>> Matias
>>
>> Sounds like it. How hard is it to change the behaviour though?
>> Does it involve changing userspace?
> 
> AFAIU, a fix for the driver may be to somehow wait until userspace
> updates the buffer before add it in the available ring.
> So far, when the device notifies the driver that a new buffer is in the
> used ring, the driver calls the virtsnd_pcm_msg_complete() function to
> do:
> ```
> schedule_work(&vss->elapsed_period);
> 
> virtsnd_pcm_msg_send(vss);
> ```
> It first defers the notification to userspace regarding an elapse period
> and then it enqueues the request again in the available
> ring.`schedule_work()` defers the calling to the
> `virtsnd_pcm_period_elapsed()` function that issues
> `snd_pcm_period_elapsed(vss->substream);` to notify userspace.
> My proposal would be that the driver could also defer
> `virtsnd_pcm_msg_send(vss)` to execute just after
> `snd_pcm_period_elapsed(vss->substream)`. Something like this:
> 
> diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> index c10d91fff2fb..41f1e74c8478 100644
> --- a/sound/virtio/virtio_pcm.c
> +++ b/sound/virtio/virtio_pcm.c
> @@ -309,6 +309,7 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
>                  container_of(work, struct virtio_pcm_substream, elapsed_period);
>   
>          snd_pcm_period_elapsed(vss->substream);
> +       virtsnd_pcm_msg_send(vss);
>   }
>   
>   /**
> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index aca2dc1989ba..43f0078b1152 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -321,7 +321,6 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
>   
>                  schedule_work(&vss->elapsed_period);
>   
> -               virtsnd_pcm_msg_send(vss);
>          } else if (!vss->msg_count) {
>                  wake_up_all(&vss->msg_empty);
>          }
> 
> 
> I tested it and it looks it fixes the issue. However, I am not sure if
> this is enough since I do not know if when `snd_pcm_period_elapsed()`
> returns, the buffers have been already updated.

It's just a lucky coincidence that this change helped in your case.

Instead, to solve the problem, it's necessary to implement read/write()
operations for the substream, and disable MMAP access mode.

I'll try, but I'm not sure I'll have enough time for this in the near future.


-- 
Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio-sound linux driver conformance to spec

[Matias Ezequiel Vara Larsen]

On Mon, Sep 25, 2023 at 10:04:33AM +0900, Anton Yakovlev wrote:
> Hello Matias,
> 
> On 20.09.2023 22:18, Matias Ezequiel Vara Larsen wrote:
> > On Tue, Sep 19, 2023 at 11:52:27AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Sep 19, 2023 at 04:18:57PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > > On Tue, Sep 19, 2023 at 05:43:56AM -0400, Michael S. Tsirkin wrote:
> > > > > On Wed, Sep 13, 2023 at 05:04:24PM +0200, Matias Ezequiel Vara Larsen wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > This email is to report a behavior of the Linux virtio-sound driver that
> > > > > > looks like it is not conforming to the VirtIO specification. The kernel
> > > > > > driver is moving buffers from the used ring to the available ring
> > > > > > without knowing if the content has been updated from the user. If the
> > > > > > device picks up buffers from the available ring just after it is
> > > > > > notified, it happens that the content is old.
> > > > > 
> > > > > Then, what happens, exactly? Do things still work?
> > > > 
> > > > We are currently developing a vhost-user backend for virtio-sound and
> > > > what happens is that if the backend implementation decides to copy the
> > > > content of a buffer from a request that just arrived to the available
> > > > ring, it gets the old content thus reproducing some sections two times.
> > > > For example, we observe that when issuing `aplay FrontLeft.wav`, we hear
> > > > `Front, front left...`. To fix this issue, our current implementation
> > > > delays reading from guest memory just until the audio engine requires.
> > > > However, the first implementation shall also work since it is conforming
> > > > to the specification.
> > > > 
> > > > Matias
> > > 
> > > Sounds like it. How hard is it to change the behaviour though?
> > > Does it involve changing userspace?
> > 
> > AFAIU, a fix for the driver may be to somehow wait until userspace
> > updates the buffer before add it in the available ring.
> > So far, when the device notifies the driver that a new buffer is in the
> > used ring, the driver calls the virtsnd_pcm_msg_complete() function to
> > do:
> > ```
> > schedule_work(&vss->elapsed_period);
> > 
> > virtsnd_pcm_msg_send(vss);
> > ```
> > It first defers the notification to userspace regarding an elapse period
> > and then it enqueues the request again in the available
> > ring.`schedule_work()` defers the calling to the
> > `virtsnd_pcm_period_elapsed()` function that issues
> > `snd_pcm_period_elapsed(vss->substream);` to notify userspace.
> > My proposal would be that the driver could also defer
> > `virtsnd_pcm_msg_send(vss)` to execute just after
> > `snd_pcm_period_elapsed(vss->substream)`. Something like this:
> > 
> > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c
> > index c10d91fff2fb..41f1e74c8478 100644
> > --- a/sound/virtio/virtio_pcm.c
> > +++ b/sound/virtio/virtio_pcm.c
> > @@ -309,6 +309,7 @@ static void virtsnd_pcm_period_elapsed(struct work_struct *work)
> >                  container_of(work, struct virtio_pcm_substream, elapsed_period);
> >          snd_pcm_period_elapsed(vss->substream);
> > +       virtsnd_pcm_msg_send(vss);
> >   }
> >   /**
> > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> > index aca2dc1989ba..43f0078b1152 100644
> > --- a/sound/virtio/virtio_pcm_msg.c
> > +++ b/sound/virtio/virtio_pcm_msg.c
> > @@ -321,7 +321,6 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
> >                  schedule_work(&vss->elapsed_period);
> > -               virtsnd_pcm_msg_send(vss);
> >          } else if (!vss->msg_count) {
> >                  wake_up_all(&vss->msg_empty);
> >          }
> > 
> > 
> > I tested it and it looks it fixes the issue. However, I am not sure if
> > this is enough since I do not know if when `snd_pcm_period_elapsed()`
> > returns, the buffers have been already updated.
> 
> It's just a lucky coincidence that this change helped in your case.
> 
> Instead, to solve the problem, it's necessary to implement read/write()
> operations for the substream, and disable MMAP access mode.
> 
> I'll try, but I'm not sure I'll have enough time for this in the near future.
> 
> 
Hello Anton,

Thanks, I will try to propose a better fix in the next few weeks. I
am not familiar with sound drivers so I may require some help. I'll let
you know if I have any question.

Thanks, Matias.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization