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

[Halil Pasic]

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

Regards,
Halil