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

[Cornelia Huck]

On Wed, 18 Oct 2017 11:52:03 +0200
Thomas Huth <address@hidden> wrote:

> On 18.10.2017 11:30, Thomas Huth wrote:
> > On 17.10.2017 16:04, Halil Pasic 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 and absence of required ORB flags.
> >>
> >> Signed-off-by: Halil Pasic <address@hidden>  
> [...]
> >> @@ -71,10 +71,24 @@ again:
> >>              goto again;
> >>          }
> >>          error_report("vfio-ccw: wirte I/O region failed with errno=%d", 
> >> errno);
> >> -        return -errno;
> >> +        ret = -errno;
> >> +    } else {
> >> +        ret = region->ret_code;
> >> +    }
> >> +    switch (-ret) {
> >> +    case 0:
> >> +        return IOINST_CC_EXPECTED;
> >> +    case EBUSY:
> >> +        return IOINST_CC_BUSY;
> >> +    case ENODEV:
> >> +    case EACCES:
> >> +        return IOINST_CC_NOT_OPERATIONAL;
> >> +    case EFAULT:
> >> +    default:
> >> +        sch_gen_unit_exception(sch);
> >> +        css_inject_io_interrupt(sch);
> >> +        return IOINST_CC_EXPECTED;  
> > 
> > Do we feel really confident that it is OK to do the setcc() in case of
> > an exception here later? ... otherwise it might be necessery to
> > introduce something like IOINST_EXCEPTION to the enum to signal the
> > ioinst_handle_xxx() callers that they should not do the setcc() anymore...  
> 
> ... or maybe rather at least return IOINST_CC_STATUS_PRESENT instead?
> IOINST_CC_EXPECTED sounds somewhat wrong to me here.

But the ssch did conclude as expected :)

Keep in mind that QEMU performs the start function synchronously (i.e.,
before the condition code is set). On real hardware, you get a cc 0 for
the ssch if the subchannel is basically in a status that's ok for
triggering the start function. A unit exception is a possible result of
the start function (and therefore generating an I/O interrupt, which
you only get if ssch set cc 0.)