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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]