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

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 12 Oct 2017 14:06:42 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com>
> >>> ---
> >>>
> >>> 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.