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

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 11 Sep 2017 13:36:29 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/11/2017 12:07 PM, Cornelia Huck wrote:
> > On Fri,  8 Sep 2017 17:24:46 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> 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.

> 
> > 
> > Can we instead choose a mapping for incorrect length, and defer a
> > possible rework?
> >   
> 
> In the commit message, I say that I don't have a fitting errno.
> If you tell me which one to use, I would be glad to split this up.
> I don't like mixing re-factoring and changing behavior myself.
> 
> Can I have your position on the re-factoring (that is let us
> imagine I did not change handling for incorrect length)?

If there is no return code that can be made to fit, we probably won't
be able to get around some kind of refactoring... but then I'd prefer
to do the refactoring first and the fix second.

> 
> > (Another idea would be to have the callback prepare the scsw via helper
> > functions. We'd just keep -EAGAIN to keep processing a chain and 0 to
> > stop.)
> >   
> 
> That was my first idea how to improve on this. I should still have the
> code (patches), but I'm not sure whether it's clean or lumped together
> with other experiments.
> 
> After pushing the handling down the call chain (caller would use
> inline functions to manipulate SCSW), I've realized that it does
> not buy us much/anything expect the better names, while we get
> the machine code manipulating the SCSW generated in multiple
> instead of in one place. I also showed the results to Dong Jia and
> he was ambivalent too: said something like it does look better,
> but it ain't better enough to make it worthwhile.
> 
> This is why I've decided to go with a less intrusive approach:
> just change the names so that it's obvious what's happening.

Something like return channel_program_check(...); or so would be quite
obvious, I think.

But yes, it will be easier to evaluate this for an actual patch ;)

> >> 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 :)

Reword the sentence to use 'short data with incorrect length checking
suppressed' or so?