On 09/11/2017 12:07 PM, Cornelia Huck wrote: > On Fri, 8 Sep 2017 17:24:46 +0200 > Halil Pasic <pa...@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. > > 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)? > (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. If you like, I look for those patches and if it ain't too much work post them as an RFC -- so we have both options in front of our eyes. >> >> 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. > >> >> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> >> --- >> hw/s390x/3270-ccw.c | 24 +++++----- >> hw/s390x/css.c | 67 +++++++++++++++----------- >> hw/s390x/virtio-ccw.c | 128 >> ++++++++++++++++++++++++------------------------- >> include/hw/s390x/css.h | 13 ++++- >> 4 files changed, 127 insertions(+), 105 deletions(-) >
