Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH

[Halil Pasic]

On 10/12/2017 02:11 PM, Cornelia Huck wrote:
> On Thu, 12 Oct 2017 14:06:42 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> On 10/10/2017 04:36 PM, Halil Pasic wrote:
>>>
>>>
>>> On 10/10/2017 03:07 PM, Cornelia Huck wrote:  
>>>> On Wed,  4 Oct 2017 17:41:39 +0200
>>>> Halil Pasic <address@hidden> wrote:
>>>>  
>>>>> Simplify the error handling of the SSCH and RSCH handler avoiding
>>>>> arbitrary and cryptic error codes being used to tell how the instruction
>>>>> is supposed to end.  Let the code detecting the condition tell how it's
>>>>> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
>>>>> in one go as the emulation of the two shares a lot of code.
>>>>>
>>>>> For passthrough this change isn't pure refactoring, but changes the
>>>>> way kernel reported EFAULT is handled. After clarifying the kernel
>>>>> interface we decided that EFAULT shall be mapped to unit exception.
>>>>> Same goes for unexpected error codes.
>>>>>
>>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>>> ---
>>>>>
>>>>> AFAIR we decided in the previous round to rather do transformation
>>>>> and fixing in one patch than touch stuff twice. Hence this patch
>>>>> ain't pure transformation any more.
>>>>> ---
>>>>>  hw/s390x/css.c              | 83 
>>>>> +++++++++++++--------------------------------
>>>>>  hw/s390x/s390-ccw.c         | 11 +++---
>>>>>  hw/vfio/ccw.c               | 30 ++++++++++++----
>>>>>  include/hw/s390x/css.h      | 24 +++++++++----
>>>>>  include/hw/s390x/s390-ccw.h |  2 +-
>>>>>  target/s390x/ioinst.c       | 53 ++++-------------------------
>>>>>  6 files changed, 77 insertions(+), 126 deletions(-)  
>>>>
>>>> After browsing through this patch, I think the change will work just as
>>>> well if you use e.g. #defines instead of the structure, won't it?
>>>>  
>>>
>>> Sure. We just loose type safety. For example if someone ever should try to
>>> propagate a normal errno via return do_something() the compiler won't
>>> catch it we effectively use int for both.
>>>
>>>  
>>
>> @Connie: Discussion seems to have died down quite a bit, and besides
>> of the struct vs enum vs int and the point of Dong Jia I've seen no
>> complaints.
>>
>> I would like to do a re-spin to accommodate the complaints, but before
>> I abandon the IOInstEnding struct I would like to confirm, that this
>> patch, and it's behavioral changes are OK. I would hate to do the dance
>> backwards, because in the next iteration it turns out that we want to
>> keep the "program_interrupt(env, PGM_ADDRESSING, 4);". Could you please
>> pass your judgment on this patch (assuming I fix the issues found by
>> Dong Jia)?
> 
> I don't think I really have complaints. Currently quite busy with other
> things, though.
> 

Thanks a lot! Unless told otherwise I intend to do a v3 beginning
next week with the things we agreed on: use enum, fix the things
by Dong Jia, and drop the last 'template approach' patch from the
series. If somebody finds something else I'm gonna fix that too
of course.

Regards,
Halil