virtio-comment

 View Only
Expand all | Collapse all

virtio-sound linux driver conformance to spec

  • 1.  virtio-sound linux driver conformance to spec

    Posted 09-13-2023 15:04
    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




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

    Posted 09-13-2023 15:51
    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




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

    Posted 09-13-2023 15:59
    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.

    On Wed, 13 Sept 2023 at 18:51, Paolo Bonzini <pbonzini@redhat.com> 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?
    >
    > > 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
    >
    >
    > This publicly archived list offers a means to provide input to the
    > OASIS Virtual I/O Device (VIRTIO) TC.
    >
    > In order to verify user consent to the Feedback License terms and
    > to minimize spam in the list archive, subscription is required
    > before posting.
    >
    > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
    > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
    > List help: virtio-comment-help@lists.oasis-open.org
    > List archive: https://lists.oasis-open.org/archives/virtio-comment/
    > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
    > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
    > Committee: https://www.oasis-open.org/committees/virtio/
    > Join OASIS: https://www.oasis-open.org/join/
    >



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

    Posted 09-18-2023 12:50
    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.




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

    Posted 09-18-2023 18:20
    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



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

    Posted 09-18-2023 11:13
    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




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

    Posted 09-18-2023 11:26
    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




  • 8.  Re: virtio-sound linux driver conformance to spec

    Posted 09-19-2023 00:36
    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



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

    Posted 09-19-2023 06:59
    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




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

    Posted 09-19-2023 15:11
    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



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

    Posted 09-25-2023 00:37
    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



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

    Posted 09-25-2023 00:25
    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



  • 13.  Re: virtio-sound linux driver conformance to spec

    Posted 09-19-2023 09:44
    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




  • 14.  Re: virtio-sound linux driver conformance to spec

    Posted 09-19-2023 14:19
    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




  • 15.  Re: virtio-sound linux driver conformance to spec

    Posted 09-19-2023 15:52
    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




  • 16.  Re: virtio-sound linux driver conformance to spec

    Posted 09-20-2023 13:19
    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




  • 17.  Re: virtio-sound linux driver conformance to spec

    Posted 09-25-2023 01:05
    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



  • 18.  Re: virtio-sound linux driver conformance to spec

    Posted 09-25-2023 14:34
    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.




  • 19.  Re: virtio-sound linux driver conformance to spec

    Posted 09-25-2023 00:56
    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