Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Cornelia Huck <cohuck@redhat.com>]

[finally got around to looking at this thread]

On Tue, Aug 30 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 29 Aug 2022 14:28:05 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Aug 18, 2022 at 03:39:58PM +0200, Halil Pasic wrote:
> [..]
>> > Fair point!
>> > 
>> > I would prefer a driver normative which goes like this:
>> > 
>> > """
>> > A driver SHOULD NOT accept features (i.e. have code that would do so if
>> > the feature is offered) if the feature is not supported by the driver
>> > (e.g. because unsupported by the transport), even if the specification
>> > implies that the device can not offer these features in the first place
>> > (e.g. because the feature is not yet supported by the transport.
>> > """  
>> 
>> ok. why not MUST NOT?
>
> I'm fine with MUST NOT. Since this is a general statement (i.e. not
> scoped to ADMIN_VQ) I felt like SHOULD NOT is a bit safer because
> provided somebody is doing this wrong for some feature already, it
> wouldn't render that implementation outright non-compliant. But I
> believe this is just a theoretical possibility. I'm fine with MUST NOT.

I'd assume that any driver that does this (accept a feature even if not
supported by the transport) today is already broken, so yes, MUST NOT is
the way to go here IMHO.

>
>> 
>> > And a similar device normative as well, which just that it may not offer
>> > such features.
>> > 
>> > """
>> > Note: The rationale behind the [reference to the normative] is that
>> > while some features can not be implemented within the boundaries of the
>> > current virtio specification, future incarnations of the specificaton may
>> > make such implementations possible.  A most prominent example is optional
>> > features dependent on optional virtio facilities whose transport specific
>> > implementation is not yet specified for some transports. Should one end
>> > gain the ability to support these features, the old implementation which
>> > made the assumption that the other end will make sure these features are
>> > not negotiated would end up negotiating something it can't actually
>> > support.
>> > """
>> > 
>> > 
>> >   
>> > > 
>> > > So, Maybe just add text
>> > > 
>> > > Note: future versions of this specification will allow setting ADMIN_VQ
>> > > for driver and device. Device MUST NOT assume driver does not
>> > > acknowledge ADMIN_VQ if offered.  
>> > 
>> > I would not lean out of the window and promise something with regards to
>> > future versions of this spec.  
>> 
>> s/will/might/
>
> With this change it works like a charm!

Works for me as well. (Although I'd use definite articles with device
and driver.)

>
>> 
>> > > 
>> > > And similarly for drivers:
>> > > 
>> > > Note: future versions of this specification will allow setting ADMIN_VQ
>> > > for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.

Same here.

>> > >   
>> > 
>> > I think we can then make a note which references the generic normative
>> > for each feature affected where it suits us.
>> >   
>> > > > 
>> > > > If we want, we can also state what needs to be done in general when
>> > > > features are unsupported by the transport. And yes, that normative
>> > > > material in my opinion.
>> > > > 
>> > > > Regards,
>> > > > Halil    
>> > > 
>> > > 
>> > > Are there other examples? I want to call out the list explicitly because
>> > > it is so easy to enable an extra feature by mistake.
>> > >   
>> > 
>> > I don't think CCW supports the shared memory yet... But I may be wrong.

You are right about this one. I think we never figured out an
architecture that would work with a mix of virtio-ccw and virtio-pci
devices...

I think ring reset and notification data should also be on that list of
non-supported features. Things like SR_IOV obviously don't make sense
for ccw, so they will never be implemented.