Re: [PATCH V7] virtio-transport: Add a new section to clarify transport requirements

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Jul 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 11, 2024 at 04:44:55PM +0200, Cornelia Huck wrote:
>> On Thu, Jul 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Thu, Jul 11, 2024 at 01:59:34PM +0200, Cornelia Huck wrote:
>> >> On Thu, Jul 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> 
>> >> > On Thu, Jul 11, 2024 at 01:46:19PM +0200, Cornelia Huck wrote:
>> >> >> On Thu, Jul 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> >> 
>> >> >> > On Thu, Jul 11, 2024 at 01:18:18PM +0530, Viresh Kumar wrote:
>> >> >> >> The virtio documentation currently doesn't define any generic
>> >> >> >> requirements that are applicable to all transports. They can be useful
>> >> >> >> while adding support for a new transport.
>> >> >> >> 
>> >> >> >> This commit tries to define the same under a new Appendix section.
>> >> >> >> 
>> >> >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> >> >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >> >> >> ---
>> >> >> >
>> >> >> > I am not sure what is this supposed doing - just listing basic things
>> >> >> > transport does or all things it can do?
>> >> >> >
>> >> >> > If the later, this ignores several things that transports can do,
>> >> >> > such as shared memory, data in notification, etc.
>> >> >> 
>> >> >> Maybe split this up?
>> >> >> 
>> >> >> A transport needs to implement the following:
>> >> >> <list of mandatory things>
>> >> >> 
>> >> >> A transport is encouraged to implement the following:
>> >> >> <list of non-mandatory, but strongly suggested things>
>> >> >
>> >> > Some things just depend on feature bits, and a transport
>> >> > can make a feature bit mandatory if it wants to.
>> >> 
>> >> I wasn't thinking about mandatory for a device/driver, but mandatory for
>> >> a transport (e.g. stuff like per-queue reset which is not mandatory for
>> >> a transport to implement.)
>> >
>> > Same thing really.
>> > For example, queue reset is controlled by VIRTIO_F_RING_RESET
>> 
>> Well, it is -- but a transport can choose not to implement the feature
>> bit. I guess what I'd like to do is to set expectations: every transport
>> needs e.g. some way to do notifications, but it can skip some other
>> things like "device discovery", for example. I think the appendix is
>> intended as a reference for someone wanting to design a transport, and
>> some kind of laundry list would help there.
>
> Yes. I am however a little worried that if we include optional features
> we have just increased the cost of adding any new transport feature -
> it now has to be added in this appendix, too.
>
> So maybe stick to just the basics? WDYT?

I'd be happy with the basics only -- the features are documented anyway,
and the important part is being able to figure out what is not optional.