On Tue, Dec 05 2023, Alex Bennée alex.bennee@linaro.org wrote:
Cornelia Huck cohuck@redhat.com writes:
On Tue, Dec 05 2023, Viresh Kumar viresh.kumar@linaro.org wrote:
+The device MUST present each event, in a transport defined way, from the +moment it takes place until the driver acknowledges the event.
I don't believe "event" is well-defined here.
Maybe:
"A device initiated transaction can isn't considered complete until acknowledged by the driver. As such data MUST remain visible to the driver until the transaction is complete"?
Transaction is good, what about:
"Any data associated with a device-initiated transaction MUST remain accessible to the driver until the driver acknowledges the transaction to be complete."
+The device MUST NOT access virtqueue's contents before the driver +notifies that the queue is ready for access, in a transport defined way.
+The device MUST NOT access buffers on the virtqueue, after it has +modified them and notified the driver about their availability.
+The device MUST reset the virtqueues if requested by the driver, in a +transport defined way.
Isn't all of this already defined in one place of the spec or another?
I think the recent example is the virtio-sound driver continuing to feed data into buffers after those buffers where submitted into the virtqueue. We should be explicit that the only time both sides of a VirtIO implementation can access things at the same time is with explicitly shared memory (and you need some sort of mechanism to mediate that to avoid chaos).
Fair enough, let's make it explicit if people already stumbled here. Some rewording suggestions:
"The device MUST NOT access the contents of a virtqueue before the driver notifies, in a transport defined way, the device that the virtqueue is ready to be accessed.
The device MUST NOT access or modify buffers on a virtqueue after it has notified the driver about their availability.
The device MUST reset the virtqueues if requested, in a transport defined way, by the driver."
+\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
+The driver MUST NOT access guest memory locations outside what's made +available by the device to the driver.
I don't think that makes sense -- I'd assume most guest memory locations do not have anything to do with virtio, and we should try to avoid host/guest terminology.
I agree guest memory isn't the right terminology here. However there are discussions about how to implement secure buffers for VirtIO - so for example a buffer mediated by some sort of secure layer. In those cases the driver may not have access to it outside of the transactions.
Yes, I think we need to limit the scope of "guest memory" here. I think we are basically wanting to deal with any memory used by virtio (device type including memory access controlled by it, transport, and the protocol itself). We would be talking about memory made available to the device by the driver for explicit usage to implement the virtio spec. I think this would cover mediation by a secure layer as well (with the driver calling into that secure layer?) Or does the (host) device end up donating memory to the (guest) driver, and we need to make sure it doesn't scribble over it?
+The driver MUST NOT access virtqueue contents before the device notifies +about the readiness of the same.
+The driver MUST NOT access buffers, after it has added them to the +virtqueue and notified the device about their availability. The driver +MAY access them after the device has processed them and notified the +driver of their availability, in a transport defined way.
+The driver MAY ask the device to reset the virtqueues if, for example, +the driver times out waiting for a notification from the device for a +previously queued request.
Again, I believe this has already been covered in the generic sections -- do we instead need to specify that a transport MUST provide a method to do xy? (or SHOULD, MAY, as applicable -- it would be good to list explicitly what is mandatory for a transport to implement, and what is optional.)
Yes I think so. The s390x channel transport gets referenced because it has a nice enumerated list of operations. It would be good to codify which operations are mandatory for all transports and which are optional.
The problem with the ccw transport is that while it has a nice list of operations, (a) it only covers guest-initiated actions, (b) probably not all of them shold be mandatory (and some of them are more of an artifact of how channel I/O works), and (c) it only implements a subset of the defined operations (which makes the not-implemented ones de facto optional, of course :) But yes, we could use it as a starting point.