Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH

[Halil Pasic]

On 09/06/2017 06:31 AM, Dong Jia Shi wrote:
> * Halil Pasic <address@hidden> [2017-09-06 00:30:58 +0200]:
> 
> [...]
> 
>>>
>>>> But I guess, I was afraid of somebody blaming me for this
>>>> comment (such TODOs in production code are a bit strange -- what
>>>> does it mean: we did not bother to figure it out?).
>>>
>>> For one, the TODO is preexisting... and we really should remember to
>>> figure out if there's something better rather than just drop the
>>> comment.
>>>
>>> (And I sure hope nobody is using vfio-ccw in production yet...)
>>>
>> Since blame says the TODO has been around since 2017-05-17
>> let me have a critical look at it.
>>
>> At a first glance I would say addressing exception for SSCH
>> is not what we want: the only possibility I see for address
>> exception for SSCH is due to the ORB address. But if that's
>> the case we will never reach the code in question.
> Agree.
> 
>> So I suppose we can do better.
> As the comment said, I'm (still) in the state of 'wondering'.
> 
>>
>> Adding Ren. @Ren: Do you agree with my analysis. If you do,
>> I could come up with a proposal what to do -- after some reading.
> If you have a better idea, and time, why not? ;)
> 

We have basically two possibilities/options which ask for different
handling:
1) EFAULT is due to a bug in the vfio-ccw implementation
(can be QEMU or kernel).
2) EFAULT is due to buggy channel program.

Option 2) is basically to be handled with a channel-program check and
setting primary secondary and alert status. For reference see PoP page
15-59 ("Designation of Storage Area").  An exception may be an invalid
channel program address in the ORB. There the channel-program check ain't
explicitly stated (although) I would expect one. It may be implied by the
things on page 15-59 though.

Option 1) is however very similar to other we have figured out that the
implementation is broken situations and should be handled consequently.
The current state of the discussion is with a unit exception.

Does that make sense?

Now, Dong Jia, I need your help to figure out do we have option 1 or
option 2 here? After quick look at the kernel code, it appears to me that
I've seen both option 1 and option 2 (I'm afraid) -- but my assessment
was really very superficial.

I would expect option 2 to be handled differently (kernel provides the
SCSW) though.

Regards,
Halil