Re: [PATCH] virtio-transport: Clarify requirements

[Alex BennÃe <alex.bennee@linaro.org>]

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?

>
>>>> +
>>>> +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)?

> (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

> 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 :-)

-- 
Alex BennÃe
Virtualisation Tech Lead @ Linaro