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
Paolo Bonzini
2023-Sep-19 06:58 UTC
[virtio-comment] Re: virtio-sound linux driver conformance to spec
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