Re: [virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck <cohuck@redhat.com>]

On Mon, Dec 18 2023, Alex BennÃe <alex.bennee@linaro.org> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
>
>> 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.
>>>>
> <snip>
>>>>> +
>>>>> +\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?
>
> I'm not sure if we have example of the host donating memory apart from
> the sort of static partitioning we see with guests on start-up where a
> region is defined as shared. The Xen grant model leaves the guest to
> grant access to its own pages to the backend.
>
> I guess for firmware mediated sharing this would still be driven by the
> guest rather than the host?

Yes, I think the guest telling the firmware which parts of its memory
the host may access is the usual pattern, or at least what I've seen so
far.

>>
>>>>> +
>>>>> +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,
>
> What examples of host initiated actions are there (aside from an IPI
> indicating a receive VirtQueue has buffers waiting)?

Also notifications for configuration changes; but I think we can put all
of this under the device->driver notification header. (Hmm, we probably
should change all those host/guest references for ccw some day...)

>
>> (b) probably not
>> all of them shold be mandatory (and some of them are more of an artifact
>> of how channel I/O works),
>
> These ones?
>
>   #define CCW_CMD_SET_IND 0x43
>   #define CCW_CMD_SET_CONF_IND 0x53
>   #define CCW_CMD_SET_IND_ADAPTER 0x73

Yes, and also CCW_CMD_SET_VIRTIO_REV (I think there used to be some
interest to implement something like that for mmio, but I don't think
anything ever ended up being specified?)

>
>> 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.
>
> Got to start somewhere :-)

Indeed :)