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

[Halil Pasic]

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.

>>
>>>
>>> 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.
> 

That is a can do. I dislike refactoring known bugs, because fixing
bugs is usually higher priority than making the code nicer, or even
marginally faster. (Btw I found these while trying to refactor.)
This however is a weak principle of mine and can be easily overpowered
by a maintainer request for example.

>>
>>> (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 ;)
> 

OK, I will look into this, and probably send an RFC these days.

>>>> 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?

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

Could you provide a whole sentence? I think my original sentence is OK
(purpose: indicate that virtio is special, and that we have to bend the
architecture a bit), but I agree, being a little more verbose may be a
good idea. I just can't come up with a nice sentence.

Halil