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

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

On Wed, Aug 31, 2022 at 04:33:49PM +0200, Halil Pasic wrote:
> On Sun, 28 Aug 2022 05:35:53 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > Like here:
> > > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > 	 +DriverFeatures even if offered by the device.
> > > > 
> > > > This makes sure that drivers do not make an assumption that
> > > > devices do not set the bit. But yes, maybe spell it out:
> > > > 
> > > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > 	 +DriverFeatures even if offered by the device.
> > > > 	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
> > > > 	 +In particular driver MUST NOT fail feature negotiation if
> > > > 	 +device offers VIRTIO_F_ADMIN_VQ.
> > > > 
> > > > ok now?  
> > > 
> > > Sorry, it still does not work for me. But I may be wrong. My problem
> > > is that what we mean is the following:
> > > 
> > > If the driver (where driver includes both the transport part and the
> > > transport agnostic part) does not support VIRTIO_F_ADMIN_VQ then it must
> > > not set VIRTIO_F_ADMIN_VQ. And any reasoning along the lines "hey the
> > > device was not supposed to offer that bit in the first place" is
> > > misguided.  
> > 
> > Yes, this is exactly what I'm trying to prevent here.
> > 
> > > The crucial part here is that the MUST NOT accept VIRTIO_F_ADMIN_VQ
> > > partee is only applicable if the driver does not support
> > > VIRTIO_F_ADMIN_VQ. That is, if we happen to extend the Channel I/O transport, and we
> > > decide to implement VIRTIO_F_ADMIN_VQ for the over Channel I/O devices,
> > > that MUST NOT accept does not get in the way.  
> > 
> > Then we'll describe how it works in the spec and then drop this.
> > 
> > > My problem with your proposal is, that the MUST NOT is not guarded by a
> > > proper precondition (it is a prohibition that does not allow for any
> > > exceptions).
> > > 
> > > I would very much like Conny to chime in on this.
> > > 
> > > Regards,
> > > Halil  
> > 
> > But we do this all the time. We disallow some behaviour then
> > following spec versions start allowing it.
> > 
> > Basically removing a requirement is ok as long as the other side
> > does not rely on it.
> > 
> 
> Sorry I don't want to delay this any further. It is unlikely to
> do any harm so I'm fine with moving forward with this.
> 
> TLDR: I agree: it would be safe to remove both of the statements
> you introduce with this patch once the transport gains support
> for the feature.
> 
> But I would still prefer being generic about the problem and maintaining
> a list of not (yet) supported features rather than adding and removing
> normative statement pairs. Just like we do for features, and also in your
> cfg_type example.
> 
> The rest are my thoughts on the implications. 
> 
> You seem to say that all the normative statements may be classified
> into two categories:
> C1) the other side does not rely on this requirement
> C2) the other side does rely on this requirement
> 
> The requirements form C1 can be removed form future versions of the
> spec, and devices may start not honoring these without breaking
> compatibility because the other side did not rely on these being held
> up in the first place.
> 
> The requirements from C2 can not be removed from future versions of the
> spec, because a device compliant to the previous version of the spec
> that relies on this requirement being fulfilled would break. And
> interchangeability is about compatibility and interoperability -- so
> that is no good.
> 
> I understand that reasoning. But what I don't understand is the
> following: how is the reader of the specification supposed to decide
> if a certain requirement is a C1 requirement or a C2 requirement.
> Misclassifying a requirement could obviously pose a problem.

The reader is supposed to satisfy all requirements.
Whether it is ok to rely on requirements is a problem that
unfortunately we don't always clarify.


> Some examples where it may not be entirely obvious if it is C1 or C2:
> 
> 2.1.2:
> The device MUST NOT consume buffers or send any used buffer
> notifications to the driver before DRIVER_OK. 
> 
> 2.2.1
> The driver MUST NOT accept a feature which the device did not offer, and
> MUST NOT accept a feature which requires another feature which was not
> accepted.
> 
> 2.4.1
> Drivers MUST NOT limit structure size and device configuration space
> size.
> 
> After some more thinking... Regarding whether the other side (B) is
> allowed to rely on a certain normative requirement or not really boils
> down to the question is there a normative statement that explicitly prohibits
> the other side (B) to rely on the property of the other side (A) that is
> implied by the first normative statement. I think feature bits are a
> good example.
> 
> I think, it would conceptually simplify things a lot if the avoided
> normative statements that we hope to drop in the future. 
> 
> This is a rather academic discussion and probably not a worthwhile one.

I absolutely understand where you are coming from... I'm fine just using
SHOULD NOT here for now.
Do you feel that's the way to go or have you changed your mind?
Couldn't figure it out.



But generally, I feel C1 is the default unless there is text that
explicitly says C2.


> 
> 
> > For example, we had this for a while:
> > 
> > 
> > 	The driver MUST ignore any vendor-specific capability structure which has
> > 	a reserved \field{cfg_type} value.
> > 
> > 
> > but the meaning of a "reserved cfg_type" changed over time, allowing
> > driver to access new cfg_type values.
> 
> Right! I completely agree, our goal is that version N and version N+M
> should be interoperable.  And that does not mean that every
> device/driver that is compliant to version N+M must also be compliant
> to the version N of the spec. I agree that having when adding new
> stuff the entity implementing the new stuff must be non-compliant
> to the statements of the previous specification that say the unknown
> stuff must be fenced.
> 
> Yet I prefer phrasings like this one where the phrasing of statement is
> still valid for all future versions of the spec (just the parameters
> change: in this case what is reserved and what is not).
> 
> Regards,
> Halil


I guess it's a matter of taste.
What worries me is that it's so easy to miss these implications.


-- 
MST