Re: [Qemu-devel] [PATCH 4/4] s390x/css: fix incorrect length indication

[Cornelia Huck]

On Tue, 12 Sep 2017 19:19:56 +0200
Halil Pasic <address@hidden> wrote:

> On 09/12/2017 05:59 PM, Cornelia Huck wrote:
> > On Tue, 12 Sep 2017 17:43:03 +0200
> > Halil Pasic <address@hidden> wrote:
> >   
> >> On 09/12/2017 04:37 PM, Cornelia Huck wrote:  
> >>> On Mon, 11 Sep 2017 13:36:29 +0200
> >>> Halil Pasic <address@hidden> wrote:
> >>>     
> >>>> On 09/11/2017 12:07 PM, Cornelia Huck wrote:    
> >>>>> On Fri,  8 Sep 2017 17:24:46 +0200
> >>>>> Halil Pasic <address@hidden> wrote:
> >>>>>       
> >>>>>> We report incorrect length via SCSW program check instead of incorrect
> >>>>>> length check (SCWS word 2 bit 10 instead of bit 9). Since we have there
> >>>>>> is no fitting errno for incorrect length, and since I don't like what 
> >>>>>> we
> >>>>>> do with the errno's, as part of the fix, errnos used for control flow 
> >>>>>> in
> >>>>>> ccw interpretation are replaced with an enum using more speaking 
> >>>>>> names.      
> >>>>>
> >>>>> I'm not sure whether this is the way to go. I mainly dislike the size
> >>>>> of the patch (and the fact that it mixes a fix and a change of function
> >>>>> signature).      
> >>>>
> >>>> Do you agree that we should move away from POSIX errno codes? I think
> >>>> if we do, this cant' get much smaller.    
> >>>
> >>> I'm not really a fan of defining our own return values, tbh.
> >>>     
> >>
> >> I've suspected. But your statement, although being useful, does
> >> not answer my question. I think we need to agree on this question
> >> before proceeding.
> >>
> >> In my opinion both the EIO bug and this bug are great examples
> >> why the POSIX errno codes are sub-optimal and misleading, but
> >> that's my opinion.  
> > 
> > It depends. I prefer them over home-grown ones.
> > 
> > (And I tend to dislike absolute statements.)
> >   
> 
> Ah, we may have a misunderstanding here. POSIX errno codes are great
> if they are used for what they are supposed to. The context was meant
> to be implicitly included in the statement limiting it's scope.
> 
> Other than spotting a possible misunderstanding (I hope I did
> not misunderstand what do you mean by absolute statements myself) this
> did not bring me any further.

As said, I'd probably prefer the alternative approach, as I'm not
really a fan of defining a set of return codes.


> >>>>>> For virtio, if incorrect length checking is suppressed we keep the
> >>>>>> current behavior (channel-program check).      
> >>>>>
> >>>>> Confused. If it is suppressed, there should not be an error, no?      
> >>>>
> >>>> No.
> >>>>
> >>>> From VIRTIO 1.0 4.3.1.2  Device Requirements: Basic Concepts
> >>>>
> >>>> "If a driver did suppress length checks for a channel command, the device
> >>>> MUST present a check condition if the transmitted data does not contain
> >>>> enough data to process the command."
> >>>> (http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-1230001)
> >>>>
> >>>> So for virtio we have to present a check condition. Architecturally it
> >>>> might look better if the one refusing is the device and not the CSS, but
> >>>> for that we would have to change the VIRTIO spec. With the given
> >>>> constraints a program check is IMHO the best fit.    
> >>>
> >>> Ah, but that's not general length checking for virtio-ccw :)    
> >>
> >> What is general length checking for virtio-ccw? Did I say it
> >> was general length checking for virtio-ccw?  
> > 
> > Hm? Generally, suppressing is supposed to allow incorrect length
> > specifications. For virtio-ccw, that only applies to 'too much' and not
> > 'not enough'.
> > 
> > Also, reading the statement in the spec: It only talks about a 'check
> > condition', not _which_ one - so there's no requirement to keep a
> > channel-program check (other than possibly confusing guests)?
> >   
> .
> You are right, and I was wrong. We could also present an unit-check
> (that's also check  -- and is the only one in device status. The spec
> even says the 'device must present', although I device is in virtio sense
> and not in PoP sense here, and does not use 'present subchannel status'
> as in the previous sentence.  For a unit check I would have expected the
> some sense bit specified to though (like it's specified that under
> certain conditions we have to do an unit check with a command reject
> (that is sense bit 0). What shall we do in your opinion?

Whatever matches both the architecture and the qemu code flow best ;)

If we can present an incorrect-length check on short data even with
suppression set, that seems like the most consistent one.

Else I would keep the current behaviour. Defining a sense bit (we'd
need to spec that) seems like overkill to me.