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

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

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