Re: [virtio-comment] Re: [PATCH v3 4/4] Add CCW configuration field "indirect_num"

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 22 Mar 2022 10:57:00 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, Mar 22 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 21 Mar 2022 17:36:26 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Sat, Mar 19 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> >>   
> >> > On Freitag, 18. März 2022 17:06:25 CET Halil Pasic wrote:    
> >>   
> >> >> I agree that the "including" is important, but I'm not sure about the
> >> >> "its contents are undefined". I don't really understand why should we use
> >> >> a plural here. What speaks against specifying that in SHOULD be stored
> >> >> as 0 by the device, and MUST be ignored by the driver?    
> >> >
> >> > Both solutions would be viable. Personally I would just use something like 
> >> > "Should be zero" if there is a value in recommending that, but I don't see a 
> >> > value in recommending to set something to zero and at the same time requiring 
> >> > to not access it in the first place.
> >> >    
> >> >> Currently we say that \field{max_indirect_num} exists like a be32 field
> >> >> even if VIRTIO_RING_F_INDIRECT_SIZE is not negotiated. Which kind of
> >> >> implies that at least type invariants should hold. Of course, there is
> >> >> none here (i.e. every bits value is also a be32 value), but for something
> >> >> like an enum interesting corner cases can pop up.    
> >> >
> >> > I can't follow you on that one. What has that do with enums in this case?
> >> >
> >> > Anyway, I won't persist on my suggestion to use the (IMO more compact form) 
> >> > "undefined". If you guys prefer the more specific solution "SHOULD be 0 and 
> >> > MUST not be accessed" then I will go that way.    
> >> 
> >> I'm not sure what mandating 0 and non-access would buy us here... the
> >> driver can of course read the field (e.g. when copying the structure
> >> wholesale); it just can't make use of the contents when it did not
> >> negotiate the feature (but why would it do so in that case anyway?)  
> >
> > My train of thought was that making the device give us a well defined
> > 0 could benefit robustness. The idea was, that even if the driver was
> > buggy, and used the value we would still end up with some sane behavior.  
> 
> I'm not sure a 0 would lead to sane behaviour in an already buggy
> driver... operating with a limit of 0 would imply that the driver cannot
> really do anything, and I'm not sure a driver buggy enough to access the
> field would heed that. There's nothing wrong with a device using 0 if
> the feature had not been negotiated, but I don't think it will help much
> with already buggy drivers.
> 

I don't consider this awfully important. While I do see some value in
devices presenting some saneish value in this situation over presenting
junk, I am fine with junk as well. Actually implementations can still do
whatever they want.

> >  
> >> 
> >> Also, I think junk remains junk, whether it is a be32 field or
> >> interpreted as an enum. It is simply not valid, even if it might by
> >> accident end up to be a defined enum value.  
> >
> > What I had in mind is the difference between "trap representation" and
> > "unspecified value" in terms of the C standard. Using a "trap
> > representation" is undefined behavior, while using an "unspecified value"
> > is far less serious. As far as I remember, there are no trap
> > representations for enumerated types in C, so the example ain't perfect.
> > But if some code was to assume that all it can see it the values defined
> > in the enum, strange stuff may happen.  
> 
> While the struct definitions look suspiciously like C, they are not in
> fact C :) 

I'm aware. I actually merely used the C standard lingo, because most of
us are familiar with C, and it is easy to read up on the precise meaning.

I pointed out the difference between using an unspecified value and
using a trap representation to showcase, that the difference between the
two might matter.

>I don't think the spec defines anything of the above, and I
> don't think it should.

Never stated the spec defines anything of the above.

> 
> >
> >  
> >> 
> >> So I think "undefined" should be fine.
> >>   
> >
> > BTW the C standard uses the term "indeterminate value" in this situation.  
> 
> "Indeterminate value" is a bit of a mouthful, though; "undefined" or
> "unpredictable" from the driver's point of view should already capture
> it, as the driver is not supposed to do anything with the value anyway.
> 

Yes the reader is more than likely to figure out what "undefined value"
or "unpredictable value" is supposed to mean in this context from the
context.  I should really stop splitting hairs. Nevertheless given
that revision >= 3 and was VIRTIO_RING_F_INDIRECT_SIZE negotiated, is
the value of indirect_max_num predictable by the driver? In my opinion
it is not. And neither is the value defined by this spec. The semantic
of the value is defined, but the value itself isn't really any more
defined than when VIRTIO_RING_F_INDIRECT_SIZE is not negotiated. 

We could say that when VIRTIO_RING_F_INDIRECT_SIZE is not negotiated
Queue Indirect Size is undefined. But I should really stop splitting
hairs. Sorry.

Regards,
Halil