Virtualization - Sep 2023 - [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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230919/8d3072ea/attachment-0001.sig>

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