Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck <cohuck@redhat.com>]

On Mon, Dec 18 2023, Bill Mills <bill.mills@linaro.org> wrote:

> On 12/18/23 9:19 AM, Alex BennÃe wrote:
>> Cornelia Huck <cohuck@redhat.com> writes:
>> 
>>> On Wed, Dec 06 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>
>>>> On 05-12-23, 14:18, Cornelia Huck wrote:
>>>>> On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> <snip>
>>>>
>>>> - The transport MUST provide a mechanism for the device and the driver
>>>>    to notify each other, for example about availability of a buffer on
>>>>    the virtqueue.
>>>
>>> Probably a mechanism for the device to notify the driver, and a
>>> mechanism for the driver to notify the device? They can be different, as
>>> long both of them are present.
>>>
>>>>
>>>> - The transport SHOULD provide a mechanism for the driver to initiate
>>>>    a reset of the virtqueues and device.
>>>
>>> Can we mandate a mechanism for a device reset? Reset of virtqueues needs
>>> to be optional, I'm not sure whether a SHOULD would be appropriate for
>>> that (or maybe we should finally come up with something for ccw ;)
>>>
>>> Other things that probably should be mandatory:
>>>
>>> - A way for the driver to change the device status. Reading it would
>>>    probably be a strong SHOULD.
>>> - A way to implement config space. (For example, channel devices don't
>>>    have a config space or anything similar enough to repurpose, so I had
>>>    to invent a mechanism for ccw... maybe future transports will be in
>>>    the same boat.)
>> 
>> I think Bill has run into problems with config space on OpenAMP setups
>> where there can be no specific trapping between domains making it hard
>> to manage a config space where both sides might want to make updates. I
>> guess the original MMIO transport gets away with it because config space
>> is always in the trap-and-emulate address space in a normal VM/emulation
>> situation.
>> 
>> Bill,
>> 
>> Is the plan to introduce a new transport that manages config in a
>> different way?
>> 
>
> I/We had a couple of ideas.  This is a thorny issue if you want 
> something that 100% matches the implications of the virtio spec.  If you 
> want something that works with existing virtio protocols it is not as 
> bad but still tricky.
>
> If you read the virtio spec it implies you could have a device with zero 
> virtqueues and just does things with the config space.  To me this 
> should be a SHOULD NOT.  Something like "The config space SHOULD NOT be 
> used as a mechanism for frequent changes."
>
> To me the config space is best used as a device to driver info space 
> with infrequent updates from both sides.  I don't know how that should 
> be expressed in the spec.

We do have some text suggesting that this should work like that, i.e. in
"Config Space" we say "Device configuration space is generally used for
rarely-changing or initialization-time parameters." and in Appendix B we
say "Device configuration space should only be used for
initialization-time parameters. It is a limited resource with no
synchronization between field written by the driver, so for most uses it
is better to use a virtqueue to update configuration information". I'm
not sure where to add a normative statement, given that this is
something that device types need to take care of.

>
> It is true that virtio-pci and virtio-mmio can signal changes in config 
> space from the driver to the device.  However the device driver can read 
> the config space asynchronously whenever it wants.  The generation 
> counter was added to fix non-atomic updates from the device.
>
> The generation counter in virtio-mmio and virtio-pci transport config 
> spaces only work because the hypervisor can see each read attempt to the 
> config space.  The virtio-pci suggests not incrementing the generation 
> counter unless the guest is doing a read.  (Because the generation 
> counter on virtio-pci is small.)
>
> In addition, config updates from the driver at not formally signaled and 
> it again relies on the hypervisor intersecting writes to the config space.

The configuration space for ccw is really different conceptually: both
reading from and writing to it are initiated by the driver sending
commands, and the architecture makes sure that reads and writes are
processed by the device strictly in the order that the driver sent
them. IOW, unless the device messes up, the driver will get consistent
information.

>
> Anyway I have thought of two solutions for the config space that do not 
> rely on hypervisor magic.
>
> 1) Config generation counter odd == update in progress
>
> In OpenAMP we are working on a transport that looks like virtio-mmio but 
> has modifications to allow it to be used w/o hypervisor magic.  A dumb 
> shared memory and bi-directional notifications should be enough.
>
> In this model we are adding multiple events in each direction.  One 
> event in each direction is a config space update.  This helps solve the 
> driver -> device config space change notification.  Assuming the driver 
> does not send back to back overlapping changes this should be enough 
> (but is not perfect.)
>
> To allow asynchronous config space reads by the driver, we could define 
> a 32 bit generation counter where odd numbers meant update in progress. 
> The config space read operation would look similar to the current model 
> => if the two values don't match >>> or are odd <<<, then read again.
>
> Not great but may be workable.
>
> 2) Message based config space
>
> The other transport option I am considering is something that is still 
> vring/virtqueue based but replaces the transport level structure with a 
> simple message passing facility.
>
> This would apply to all transport level ops (virtqueue discover and 
> config, feature negotiation, etc) but the config space would work like 
> below.
>
> In this model there would be no shared config space.  Each side would 
> have its own copy of the config space and would send messages to make 
> updates.  The driver side would start empty and would send a message to 
> discover the size of the config space and read its initial value. 
> Multiple reads may be needed to read the whole initial value if the 
> config space was large (>128 bytes?).  After the initial sync either 
> side can make updates and sends the offset, size and value of a region 
> that contains the change.

If all messages were sent by the driver, this would be very similar to
the ccw implementation. You would still have the device view of the
config space, and the driver could either have its own copy and update
if needed, or just obtain/change information when it actually needs it.
Plus a notification mechanism for device->driver "something changed, you
might want to re-read" (instead of sending a message with the change).

>
> Sorry for the info dump.  I was working on a doc that described the 
> problems and the AMP virtio-mmio option but it got stalled.  I will try 
> to get it going again.

This is actually very interesting :)