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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Aug 16, 2022 at 11:48:51AM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> > On Fri, 12 Aug 2022 13:19:20 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  content.tex | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 76b5a28..53be680 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > >  combination.
> > >  
> > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DeviceFeatures.
> > > +
> > > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DriverFeatures even if offered by the device.
> > > +
> > 
> > I'm not sure I understand the intention here. I believe what we try to
> > accomplish here is the following. The Channel I/O transport *currently*
> > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > the Channel I/O transport. Or am I wrong?
> >
> > If my assumptions are right, then the old incarnation of the spec could
> > contradict the new incarnation of the spec. Thus I would prefer something
> > like.
> 
> Relaxing requirenents is always okay.
> 
> > 
> > """
> > Currently the following features are not supported by the Channel I/O
> > transport:
> > * VIRTIO_F_ADMIN_VQ
> > """
> 
> what I want to prevent is driver saying "oh device will not set ADMIN_VQ
> so it's ok to acknowledge it if offered, it is never offered even
> though it does not suport it".
> because then it becomes impossible to know when actually a new driver
> appears with actual 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.
> 
> 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.
> 
> > 
> > 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.
> 

And also, I don't *want* to make it easy to add features only to some
transports. Possible, ok, but not easy.

> > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > >  
> > >  The device's configuration space is located in host memory.