Re: [Qemu-devel] [PATCH 2/9] s390x: fix invalid use of cc 1 for SSCH

[Halil Pasic]

On 09/05/2017 05:46 PM, Cornelia Huck wrote:
> On Tue, 5 Sep 2017 17:24:19 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> My problem with a program check (indicated by SCSW word 2 bit 10) is
>> that, in my reading of the architecture, the semantic behind it is: The
>> channel subsystem (not the cu or device) has detected, that the 
>> the channel program (previously submitted as an ORB) is erroneous. Which
>> programs are erroneous is specified by the architecture. What we have
>> here does not qualify.
>>
>> My idea was to rather blame the virtual hardware (device) and put no blame
>> on the program nor he channel subsystem. This could be done using device
>> status (unit check with command reject, maybe unit exception) or interface
>> check. My train of thought was, the problem is not consistent across a
>> device type, so it has to be device specific.
> 
> Unit exception might be a better way to express what is happening here.
> At least, it moves us away from cc 1 and not towards cc 3 :)
> 

I will do a follow up patch pursuing device exception.

>>
>> Of course blaming the device could mislead the person encountering the
>> problem, and make him believe it's an non-virtual hardware problem.
>>
>> About the misleading, I think the best we can do is log out a message
>> indicating what really happened.
> 
> Just document it in the code? If it doesn't happen with Linux as a
> guest, it is highly unlikely to be seen in the wild.
> 


Well we have two problems here:
1) Unit exception can be already defined by the device type for the
command (reference: 
http://publibfp.dhe.ibm.com/cgi-bin/bookmgr/BOOKS/dz9ar110/2.6.10?DT=19920904110920).
I think this one is what you mean. And I agree that's best handled
with comment in code.
2) The poor user/programmer is trying to figure out why things
don't work (why are we getting the unit exception)? I think that's
best remedied with producing something for the log (maybe a warning
with warn_report which states that the implementation vfio-ccw requires
the given flags).

[..] 
>>>>>> @@ -1115,7 +1112,7 @@ static int do_subchannel_work(SubchDev *sch)
>>>>>>      if (sch->do_subchannel_work) {
>>>>>>          return sch->do_subchannel_work(sch);
>>>>>>      } else {
>>>>>> -        return -EINVAL;
>>>>>> +        return -ENODEV;    
>>>>>
>>>>> This rather seems like a job for an assert? If we don't have a function
>>>>> for the 'asynchronous' handling of the various functions assigned for a
>>>>> subchannel, that looks like an internal error.
>>>>>     
>>>>
>>>> IMHO it depends. Aborting qemu is heavy handed, and as an user I would not
>>>> be happy about it. But certainly it is an assert situation.  We can look 
>>>> for
>>>> an even better solution, but I think this is an improvement. The logic 
>>>> behind
>>>> is that the device is broken and can't be talked to properly.  
>>>
>>> We currently don't have a vast array of subchannel types (and are
>>> unlikely to get more types that need a different handler function). We
>>> know the current ones are fine, and an assert would catch programming
>>> errors early.
>>>   
>>
>> Despite of that we already had a problem of this type: see 1728cff2ab
>> ("s390x/3270: fix instruction interception handler", 2017-06-09) by 
>> Dong Jia. If we had some automated testing covering all the asserts
>> I would not think twice about using an assert here. But I don't think
>> we do and I'm reluctant (not positive that assert is superior to what
>> we have now). Maybe we could agree on reported by again.
> 
> Yes, we (as in generally 'we') are really lacking automated testing...
> (it is somewhere on my todo list).
> 
> Either leave it as-is, or do an assert. -ENODEV just feels wrong.
> 

I think I will leave this one as is and maybe try to discuss with
the folks here about reliable test coverage. Just spoke with Marc H.,
and according to that we have a long way to go.