On Tue, Dec 05 2023, Alex Bennée <alex.ben...@linaro.org> wrote:
> Cornelia Huck <coh...@redhat.com> writes:
>
>> On Tue, Dec 05 2023, Viresh Kumar <viresh.ku...@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.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
