[Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Halil Pasic]

Abstract
=======

The basic idea is: tell how to handle an unusual condition where it's
identified, instead of mapping it to an errno (more or less arbitrarily),
then possibly mapping these errnos around, to finally (mentally) map the
errno back to the condition and take appropriate action.

According to Dong Jia the patch-set also has a functional value: for ccw
pass-through, he is planing to pass-through the instruction completion
information (cc or interruption condition) from the kernel, and this
patch set can pretty much be seen as a preparation for that.

Changelog
=========

Patch 1 should be already applied to Conny's tree. I've included it
nevertheless so guys working on top of current master have everything in
place.

v2 -> v3:
* somewhat uwillingly traded the type safe struct to a somewhat
  type safe enum (because considered ugly) (Thomas, Conny)
* dropped 'template approach' patch which intended to further
  consolidate IO inst. handlers having lot's of logic and code in
  common (Conny)
* added warning if vfio-ccw ORB does not have the flags required
  by the vfio-ccw implementation as suggested (Dong Jia)
* got rid of some unintentional changes (Dong Jia)
* reworded some stuff (comments, commit messages) (Dong Jia)

v1 -> v2:
* use assert if do_subchannel_work without any functions being
  accepted 
* generate unit-exception if ccw-vfio can't handle an otherwise
  good channel program (due to extra limitations) 
* keep using return values opposed to recording into SubchDev
* split out 'add infrastructure' from 'refactor first handler'
* reworded some commit messanges and comments
* rebased on top of current master
* dropped r-b's and acks because of the magnitude of the
  changes

Testing
=======

Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
testing, especially regarding the changes in vfio-ccw (patch #3).

Halil Pasic (7):
  s390x/css: be more consistent if broken beyond repair
  s390x/css: IO instr handler ending control
  s390x: improve error handling for SSCH and RSCH
  s390x: refactor error handling for XSCH handler
  s390x: refactor error handling for CSCH handler
  s390x: refactor error handling for HSCH handler
  s390x: refactor error handling for MSCH handler

 hw/s390x/css.c              | 163 ++++++++++++--------------------------------
 hw/s390x/s390-ccw.c         |  11 ++-
 hw/vfio/ccw.c               |  28 ++++++--
 include/hw/s390x/css.h      |  47 ++++++++++---
 include/hw/s390x/s390-ccw.h |   2 +-
 target/s390x/ioinst.c       | 136 +++++++-----------------------------
 6 files changed, 132 insertions(+), 255 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v3 1/7] s390x/css: be more consistent if broken beyond repair

[Halil Pasic]

Calling do_subchannel_work with no function control flags set in SCSW is
a programming error. Currently the handle this differently in
do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be
consistent and guard with a common assert against this programming error.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
Already applied to Connies s390-next. Included for the sake of completenes
(with) the old typo in the commit message.
---
 hw/s390x/css.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 35683d7954..aa233d5f8a 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1245,9 +1245,6 @@ int do_subchannel_work_virtual(SubchDev *sch)
     } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
         /* Triggered by both ssch and rsch. */
         sch_handle_start_func_virtual(sch);
-    } else {
-        /* Cannot happen. */
-        return 0;
     }
     css_inject_io_interrupt(sch);
     return 0;
@@ -1255,22 +1252,17 @@ int do_subchannel_work_virtual(SubchDev *sch)
 
 int do_subchannel_work_passthrough(SubchDev *sch)
 {
-    int ret;
+    int ret = 0;
     SCSW *s = &sch->curr_status.scsw;
 
     if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
         /* TODO: Clear handling */
         sch_handle_clear_func(sch);
-        ret = 0;
     } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
         /* TODO: Halt handling */
         sch_handle_halt_func(sch);
-        ret = 0;
     } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
         ret = sch_handle_start_func_passthrough(sch);
-    } else {
-        /* Cannot happen. */
-        return -ENODEV;
     }
 
     return ret;
@@ -1278,11 +1270,11 @@ int do_subchannel_work_passthrough(SubchDev *sch)
 
 static int do_subchannel_work(SubchDev *sch)
 {
-    if (sch->do_subchannel_work) {
-        return sch->do_subchannel_work(sch);
-    } else {
+    if (!sch->do_subchannel_work) {
         return -EINVAL;
     }
+    g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
+    return sch->do_subchannel_work(sch);
 }
 
 static void copy_pmcw_to_guest(PMCW *dest, const PMCW *src)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 1/7] s390x/css: be more consistent if broken beyond repair

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> Calling do_subchannel_work with no function control flags set in SCSW is
> a programming error. Currently the handle this differently in
> do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be
> consistent and guard with a common assert against this programming error.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> Already applied to Connies s390-next. Included for the sake of completenes
> (with) the old typo in the commit message.

Looks like Cornelia fixed it up in s390-next :-)

> ---
>  hw/s390x/css.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)

FWIW:

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Halil Pasic]

CSS code needs to tell the IO instruction handlers located in how should
the emulated instruction be ended. Currently this is done by returning
generic (POSIX) error codes, and mapping them to outcomes like condition
codes. This makes bugs easy to create and hard to recognise.

As a preparation for moving a way form (mis)using generic error codes for
flow control let us introduce a type which tells the instruction
handler function how to end the instruction, in a more straight-forward
and less ambiguous way.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 include/hw/s390x/css.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 69b374730e..7e0dbd162f 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -99,6 +99,22 @@ typedef struct CcwDataStream {
     hwaddr cda;
 } CcwDataStream;
 
+/*
+ * IO instructions conclude according this. Currently we have only
+ * cc codes. Valid values are 0,1,2,3 and the generic semantic for
+ * IO instructions is described briefly. For more details consult the PoP.
+ */
+typedef enum IOInstEnding {
+    /* produced expected result */
+    IOINST_CC_EXPECTED = 0,
+    /* status conditions were present or produced alternate result */
+    IOINST_CC_STATUS_PRESENT = 1,
+    /* inst. ineffective because busy with previously initiated function */
+    IOINST_CC_BUSY = 2,
+    /* inst. ineffective because not operational */
+    IOINST_CC_NOT_OPERATIONAL = 3
+} IOInstEnding;
+
 typedef struct SubchDev SubchDev;
 struct SubchDev {
     /* channel-subsystem related things: */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:48 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> CSS code needs to tell the IO instruction handlers located in how should
> the emulated instruction be ended. Currently this is done by returning
> generic (POSIX) error codes, and mapping them to outcomes like condition
> codes. This makes bugs easy to create and hard to recognise.
> 
> As a preparation for moving a way form (mis)using generic error codes for

s/form/from/

> flow control let us introduce a type which tells the instruction
> handler function how to end the instruction, in a more straight-forward
> and less ambiguous way.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  include/hw/s390x/css.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 69b374730e..7e0dbd162f 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
>      hwaddr cda;
>  } CcwDataStream;
>  
> +/*
> + * IO instructions conclude according this. Currently we have only

s/this/to this/

> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for

blanks between numbers?

> + * IO instructions is described briefly. For more details consult the PoP.
> + */
> +typedef enum IOInstEnding {
> +    /* produced expected result */
> +    IOINST_CC_EXPECTED = 0,
> +    /* status conditions were present or produced alternate result */
> +    IOINST_CC_STATUS_PRESENT = 1,
> +    /* inst. ineffective because busy with previously initiated function */
> +    IOINST_CC_BUSY = 2,
> +    /* inst. ineffective because not operational */
> +    IOINST_CC_NOT_OPERATIONAL = 3
> +} IOInstEnding;

This looks a bit odd for some I/O instructions (STCRW, TPI, TSCH), but
is fine for the others. But as the PoP also defines the meanings as
above, it should be fine (and not confusing).

> +
>  typedef struct SubchDev SubchDev;
>  struct SubchDev {
>      /* channel-subsystem related things: */

Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Halil Pasic]

On 10/17/2017 04:58 PM, Cornelia Huck wrote:
> On Tue, 17 Oct 2017 16:04:48 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> CSS code needs to tell the IO instruction handlers located in how should
>> the emulated instruction be ended. Currently this is done by returning
>> generic (POSIX) error codes, and mapping them to outcomes like condition
>> codes. This makes bugs easy to create and hard to recognise.

also
s/recognise/recognize/

To on mix American and British.

>>
>> As a preparation for moving a way form (mis)using generic error codes for
> 
> s/form/from/
> 

Sorry this seems to be a recurring typo of me (not detected by spell check
because both valid).

>> flow control let us introduce a type which tells the instruction
>> handler function how to end the instruction, in a more straight-forward
>> and less ambiguous way.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  include/hw/s390x/css.h | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index 69b374730e..7e0dbd162f 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
>> @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
>>      hwaddr cda;
>>  } CcwDataStream;
>>  
>> +/*
>> + * IO instructions conclude according this. Currently we have only
> 
> s/this/to this/
> 
>> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for
> 
> blanks between numbers?

Overrated. Just joking. Definitely blanks between numbers.

> 
>> + * IO instructions is described briefly. For more details consult the PoP.
>> + */
>> +typedef enum IOInstEnding {
>> +    /* produced expected result */
>> +    IOINST_CC_EXPECTED = 0,
>> +    /* status conditions were present or produced alternate result */
>> +    IOINST_CC_STATUS_PRESENT = 1,
>> +    /* inst. ineffective because busy with previously initiated function */
>> +    IOINST_CC_BUSY = 2,
>> +    /* inst. ineffective because not operational */
>> +    IOINST_CC_NOT_OPERATIONAL = 3
>> +} IOInstEnding;
> 
> This looks a bit odd for some I/O instructions (STCRW, TPI, TSCH), but
> is fine for the others. But as the PoP also defines the meanings as
> above, it should be fine (and not confusing).

Yeah. The original idea was to keep stuff abstract, but I decided
to go with the names carrying meaning because Thomas seems to
be more reliable than me when it comes to matters of taste, and also
for consistency with SIGP_CC_*. I think, in the end it does not matter
that much.

> 
>> +
>>  typedef struct SubchDev SubchDev;
>>  struct SubchDev {
>>      /* channel-subsystem related things: */
> 
> 

Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> CSS code needs to tell the IO instruction handlers located in how should

locate in? ... sorry, I've got trouble to parse this sentence ...

> the emulated instruction be ended. Currently this is done by returning
> generic (POSIX) error codes, and mapping them to outcomes like condition
> codes. This makes bugs easy to create and hard to recognise.
> 
> As a preparation for moving a way form (mis)using generic error codes for

s/a way/away/ ?

> flow control let us introduce a type which tells the instruction
> handler function how to end the instruction, in a more straight-forward
> and less ambiguous way.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  include/hw/s390x/css.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 69b374730e..7e0dbd162f 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
>      hwaddr cda;
>  } CcwDataStream;
>  
> +/*
> + * IO instructions conclude according this. Currently we have only

Maybe rather "terminate like this" or "finish like this"? I'm not a
native speaker, but "conclude" sounds a little bit strange to me here.

> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for
> + * IO instructions is described briefly. For more details consult the PoP.
> + */
> +typedef enum IOInstEnding {
> +    /* produced expected result */
> +    IOINST_CC_EXPECTED = 0,
> +    /* status conditions were present or produced alternate result */
> +    IOINST_CC_STATUS_PRESENT = 1,
> +    /* inst. ineffective because busy with previously initiated function */
> +    IOINST_CC_BUSY = 2,
> +    /* inst. ineffective because not operational */
> +    IOINST_CC_NOT_OPERATIONAL = 3
> +} IOInstEnding;
> +
>  typedef struct SubchDev SubchDev;
>  struct SubchDev {
>      /* channel-subsystem related things: */
> 

With the patch description fixed:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Cornelia Huck]

On Wed, 18 Oct 2017 10:45:28 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17.10.2017 16:04, Halil Pasic wrote:
> > CSS code needs to tell the IO instruction handlers located in how should  
> 
> locate in? ... sorry, I've got trouble to parse this sentence ...

"located in ioinst.c", I guess.

And maybe also move "should" a bit later in the sentence.

> 
> > the emulated instruction be ended. Currently this is done by returning
> > generic (POSIX) error codes, and mapping them to outcomes like condition
> > codes. This makes bugs easy to create and hard to recognise.
> > 
> > As a preparation for moving a way form (mis)using generic error codes for  
> 
> s/a way/away/ ?

I can fix that up as well.

> 
> > flow control let us introduce a type which tells the instruction
> > handler function how to end the instruction, in a more straight-forward
> > and less ambiguous way.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >  include/hw/s390x/css.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 69b374730e..7e0dbd162f 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
> >      hwaddr cda;
> >  } CcwDataStream;
> >  
> > +/*
> > + * IO instructions conclude according this. Currently we have only  
> 
> Maybe rather "terminate like this" or "finish like this"? I'm not a
> native speaker, but "conclude" sounds a little bit strange to me here.

I do remember reading "conclude" in various places in s390
documentation, so I think I'll keep this.

> 
> > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for
> > + * IO instructions is described briefly. For more details consult the PoP.
> > + */
> > +typedef enum IOInstEnding {
> > +    /* produced expected result */
> > +    IOINST_CC_EXPECTED = 0,
> > +    /* status conditions were present or produced alternate result */
> > +    IOINST_CC_STATUS_PRESENT = 1,
> > +    /* inst. ineffective because busy with previously initiated function */
> > +    IOINST_CC_BUSY = 2,
> > +    /* inst. ineffective because not operational */
> > +    IOINST_CC_NOT_OPERATIONAL = 3
> > +} IOInstEnding;
> > +
> >  typedef struct SubchDev SubchDev;
> >  struct SubchDev {
> >      /* channel-subsystem related things: */
> >   
> 
> With the patch description fixed:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:48 +0200]:

[...]

> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 69b374730e..7e0dbd162f 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
>      hwaddr cda;
>  } CcwDataStream;
> 
> +/*
> + * IO instructions conclude according this. Currently we have only
> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for
> + * IO instructions is described briefly. For more details consult the PoP.
> + */
> +typedef enum IOInstEnding {
> +    /* produced expected result */
> +    IOINST_CC_EXPECTED = 0,
> +    /* status conditions were present or produced alternate result */
> +    IOINST_CC_STATUS_PRESENT = 1,
> +    /* inst. ineffective because busy with previously initiated function */
> +    IOINST_CC_BUSY = 2,
> +    /* inst. ineffective because not operational */
> +    IOINST_CC_NOT_OPERATIONAL = 3
> +} IOInstEnding;
> +
>  typedef struct SubchDev SubchDev;
>  struct SubchDev {
>      /* channel-subsystem related things: */
> -- 
> 2.13.5
> 

With what has been pointed out by Conny and Thomas addressed:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

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

[Halil Pasic]

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 <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c              | 84 +++++++++++++--------------------------------
 hw/s390x/s390-ccw.c         | 11 +++---
 hw/vfio/ccw.c               | 28 +++++++++++----
 include/hw/s390x/css.h      | 23 +++++++++----
 include/hw/s390x/s390-ccw.h |  2 +-
 target/s390x/ioinst.c       | 53 ++++------------------------
 6 files changed, 75 insertions(+), 126 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index aa233d5f8a..ff5a05c34b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
 
 }
 
-static int sch_handle_start_func_passthrough(SubchDev *sch)
+static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
 {
 
     PMCW *p = &sch->curr_status.pmcw;
     SCSW *s = &sch->curr_status.scsw;
-    int ret;
 
     ORB *orb = &sch->orb;
     if (!(s->ctrl & SCSW_ACTL_SUSP)) {
@@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
      */
     if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
         !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
-        return -EINVAL;
+        warn_report("vfio-ccw requires PFCH and C64 flags set...");
+        sch_gen_unit_exception(sch);
+        css_inject_io_interrupt(sch);
+        return IOINST_CC_EXPECTED;
     }
-
-    ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
-    switch (ret) {
-    /* Currently we don't update control block and just return the cc code. */
-    case 0:
-        break;
-    case -EBUSY:
-        break;
-    case -ENODEV:
-        break;
-    case -EACCES:
-        /* Let's reflect an inaccessible host device by cc 3. */
-        ret = -ENODEV;
-        break;
-    default:
-       /*
-        * All other return codes will trigger a program check,
-        * or set cc to 1.
-        */
-       break;
-    };
-
-    return ret;
+    return s390_ccw_cmd_request(sch);
 }
 
 /*
@@ -1233,7 +1213,7 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
  * read/writes) asynchronous later on if we start supporting more than
  * our current very simple devices.
  */
-int do_subchannel_work_virtual(SubchDev *sch)
+IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
 {
 
     SCSW *s = &sch->curr_status.scsw;
@@ -1247,12 +1227,12 @@ int do_subchannel_work_virtual(SubchDev *sch)
         sch_handle_start_func_virtual(sch);
     }
     css_inject_io_interrupt(sch);
-    return 0;
+    /* inst must succeed if this func is called */
+    return IOINST_CC_EXPECTED;
 }
 
-int do_subchannel_work_passthrough(SubchDev *sch)
+IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
 {
-    int ret = 0;
     SCSW *s = &sch->curr_status.scsw;
 
     if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
@@ -1262,16 +1242,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
         /* TODO: Halt handling */
         sch_handle_halt_func(sch);
     } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
-        ret = sch_handle_start_func_passthrough(sch);
+        return sch_handle_start_func_passthrough(sch);
     }
-
-    return ret;
+    return IOINST_CC_EXPECTED;
 }
 
-static int do_subchannel_work(SubchDev *sch)
+static IOInstEnding do_subchannel_work(SubchDev *sch)
 {
     if (!sch->do_subchannel_work) {
-        return -EINVAL;
+        return IOINST_CC_STATUS_PRESENT;
     }
     g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
     return sch->do_subchannel_work(sch);
@@ -1561,27 +1540,23 @@ static void css_update_chnmon(SubchDev *sch)
     }
 }
 
-int css_do_ssch(SubchDev *sch, ORB *orb)
+IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
-    int ret;
 
     if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
-        ret = -ENODEV;
-        goto out;
+        return IOINST_CC_NOT_OPERATIONAL;
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     if (s->ctrl & (SCSW_FCTL_START_FUNC |
                    SCSW_FCTL_HALT_FUNC |
                    SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return IOINST_CC_BUSY;
     }
 
     /* If monitoring is active, update counter. */
@@ -1594,10 +1569,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb)
     s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND);
     s->flags &= ~SCSW_FLAGS_MASK_PNO;
 
-    ret = do_subchannel_work(sch);
-
-out:
-    return ret;
+    return do_subchannel_work(sch);
 }
 
 static void copy_irb_to_guest(IRB *dest, const IRB *src, PMCW *pmcw,
@@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
     }
 }
 
-int css_do_rsch(SubchDev *sch)
+IOInstEnding css_do_rsch(SubchDev *sch)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
-    int ret;
 
     if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
-        ret = -ENODEV;
-        goto out;
+        return IOINST_CC_NOT_OPERATIONAL;
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
         (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
         (!(s->ctrl & SCSW_ACTL_SUSP))) {
-        ret = -EINVAL;
-        goto out;
+        return IOINST_CC_BUSY;
     }
 
     /* If monitoring is active, update counter. */
@@ -1873,11 +1841,7 @@ int css_do_rsch(SubchDev *sch)
     }
 
     s->ctrl |= SCSW_ACTL_RESUME_PEND;
-    do_subchannel_work(sch);
-    ret = 0;
-
-out:
-    return ret;
+    return do_subchannel_work(sch);
 }
 
 int css_do_rchp(uint8_t cssid, uint8_t chpid)
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 8614dda6f8..0ef232ec27 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -18,15 +18,14 @@
 #include "hw/s390x/css-bridge.h"
 #include "hw/s390x/s390-ccw.h"
 
-int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
+IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
 {
-    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
+    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
 
-    if (cdc->handle_request) {
-        return cdc->handle_request(orb, scsw, data);
-    } else {
-        return -ENOSYS;
+    if (!cdc->handle_request) {
+        return IOINST_CC_STATUS_PRESENT;
     }
+    return cdc->handle_request(sch);
 }
 
 static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 76323c6bde..1cc2e5d873 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -47,9 +47,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
     .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
 };
 
-static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
+static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
 {
-    S390CCWDevice *cdev = data;
+    S390CCWDevice *cdev = sch->driver_data;
     VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
     struct ccw_io_region *region = vcdev->io_region;
     int ret;
@@ -60,8 +60,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
 
     memset(region, 0, sizeof(*region));
 
-    memcpy(region->orb_area, orb, sizeof(ORB));
-    memcpy(region->scsw_area, scsw, sizeof(SCSW));
+    memcpy(region->orb_area, &sch->orb, sizeof(ORB));
+    memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
 
 again:
     ret = pwrite(vcdev->vdev.fd, region,
@@ -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;
     }
-
-    return region->ret_code;
 }
 
 static void vfio_ccw_reset(DeviceState *dev)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 7e0dbd162f..10bde18452 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -136,11 +136,22 @@ struct SubchDev {
     /* transport-provided data: */
     int (*ccw_cb) (SubchDev *, CCW1);
     void (*disable_cb)(SubchDev *);
-    int (*do_subchannel_work) (SubchDev *);
+    IOInstEnding (*do_subchannel_work) (SubchDev *);
     SenseId id;
     void *driver_data;
 };
 
+static inline void sch_gen_unit_exception(SubchDev *sch)
+{
+    sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+    sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY |
+                                  SCSW_STCTL_SECONDARY |
+                                  SCSW_STCTL_ALERT |
+                                  SCSW_STCTL_STATUS_PEND;
+    sch->curr_status.scsw.cpa = sch->channel_prog + 8;
+    sch->curr_status.scsw.dstat =  SCSW_DSTAT_UNIT_EXCEP;
+}
+
 extern const VMStateDescription vmstate_subch_dev;
 
 /*
@@ -199,9 +210,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
 void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
 void css_generate_css_crws(uint8_t cssid);
 void css_clear_sei_pending(void);
-int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data);
-int do_subchannel_work_virtual(SubchDev *sub);
-int do_subchannel_work_passthrough(SubchDev *sub);
+IOInstEnding s390_ccw_cmd_request(SubchDev *sch);
+IOInstEnding do_subchannel_work_virtual(SubchDev *sub);
+IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
 
 typedef enum {
     CSS_IO_ADAPTER_VIRTIO = 0,
@@ -232,7 +243,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *schib);
 int css_do_xsch(SubchDev *sch);
 int css_do_csch(SubchDev *sch);
 int css_do_hsch(SubchDev *sch);
-int css_do_ssch(SubchDev *sch, ORB *orb);
+IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
 int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
 void css_do_tsch_update_subch(SubchDev *sch);
 int css_do_stcrw(CRW *crw);
@@ -243,7 +254,7 @@ int css_collect_chp_desc(int m, uint8_t cssid, uint8_t f_chpid, uint8_t l_chpid,
 void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo);
 int css_enable_mcsse(void);
 int css_enable_mss(void);
-int css_do_rsch(SubchDev *sch);
+IOInstEnding css_do_rsch(SubchDev *sch);
 int css_do_rchp(uint8_t cssid, uint8_t chpid);
 bool css_present(uint8_t cssid);
 #endif
diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
index 9f45cf1347..7d15a1a5d4 100644
--- a/include/hw/s390x/s390-ccw.h
+++ b/include/hw/s390x/s390-ccw.h
@@ -33,7 +33,7 @@ typedef struct S390CCWDeviceClass {
     CCWDeviceClass parent_class;
     void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
     void (*unrealize)(S390CCWDevice *dev, Error **errp);
-    int (*handle_request) (ORB *, SCSW *, void *);
+    IOInstEnding (*handle_request) (SubchDev *sch);
 } S390CCWDeviceClass;
 
 #endif
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 47490f838a..16b5cf2fed 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -218,8 +218,6 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
     SubchDev *sch;
     ORB orig_orb, orb;
     uint64_t addr;
-    int ret = -ENODEV;
-    int cc;
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
@@ -239,33 +237,11 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
     }
     trace_ioinst_sch_id("ssch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_ssch(sch, &orb);
-    }
-    switch (ret) {
-    case -ENODEV:
-        cc = 3;
-        break;
-    case -EBUSY:
-        cc = 2;
-        break;
-    case -EFAULT:
-        /*
-         * TODO:
-         * I'm wondering whether there is something better
-         * to do for us here (like setting some device or
-         * subchannel status).
-         */
-        program_interrupt(env, PGM_ADDRESSING, 4);
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
         return;
-    case 0:
-        cc = 0;
-        break;
-    default:
-        cc = 1;
-        break;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_ssch(sch, &orb));
 }
 
 void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb)
@@ -784,8 +760,6 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
-    int ret = -ENODEV;
-    int cc;
 
     if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
@@ -793,24 +767,11 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
     }
     trace_ioinst_sch_id("rsch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_rsch(sch);
-    }
-    switch (ret) {
-    case -ENODEV:
-        cc = 3;
-        break;
-    case -EINVAL:
-        cc = 2;
-        break;
-    case 0:
-        cc = 0;
-        break;
-    default:
-        cc = 1;
-        break;
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
+        return;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_rsch(sch));
 }
 
 #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)
-- 
2.13.5

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

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:49 +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 and absence of required ORB flags.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 28 +++++++++++----
>  include/hw/s390x/css.h      | 23 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++------------------------
>  6 files changed, 75 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index aa233d5f8a..ff5a05c34b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>  
>  }
>  
> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>  {
>  
>      PMCW *p = &sch->curr_status.pmcw;
>      SCSW *s = &sch->curr_status.scsw;
> -    int ret;
>  
>      ORB *orb = &sch->orb;
>      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> @@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>       */
>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -        return -EINVAL;
> +        warn_report("vfio-ccw requires PFCH and C64 flags set...");

Drop the trailing dots?

> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return IOINST_CC_EXPECTED;
>      }
> -
> -    ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
> -    switch (ret) {
> -    /* Currently we don't update control block and just return the cc code. */
> -    case 0:
> -        break;
> -    case -EBUSY:
> -        break;
> -    case -ENODEV:
> -        break;
> -    case -EACCES:
> -        /* Let's reflect an inaccessible host device by cc 3. */
> -        ret = -ENODEV;
> -        break;
> -    default:
> -       /*
> -        * All other return codes will trigger a program check,
> -        * or set cc to 1.
> -        */
> -       break;
> -    };
> -
> -    return ret;
> +    return s390_ccw_cmd_request(sch);
>  }
>  
>  /*
> @@ -1233,7 +1213,7 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>   * read/writes) asynchronous later on if we start supporting more than
>   * our current very simple devices.
>   */
> -int do_subchannel_work_virtual(SubchDev *sch)
> +IOInstEnding do_subchannel_work_virtual(SubchDev *sch)
>  {
>  
>      SCSW *s = &sch->curr_status.scsw;
> @@ -1247,12 +1227,12 @@ int do_subchannel_work_virtual(SubchDev *sch)
>          sch_handle_start_func_virtual(sch);
>      }
>      css_inject_io_interrupt(sch);
> -    return 0;
> +    /* inst must succeed if this func is called */
> +    return IOINST_CC_EXPECTED;
>  }
>  
> -int do_subchannel_work_passthrough(SubchDev *sch)
> +IOInstEnding do_subchannel_work_passthrough(SubchDev *sch)
>  {
> -    int ret = 0;
>      SCSW *s = &sch->curr_status.scsw;
>  
>      if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
> @@ -1262,16 +1242,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
>          /* TODO: Halt handling */
>          sch_handle_halt_func(sch);
>      } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> -        ret = sch_handle_start_func_passthrough(sch);
> +        return sch_handle_start_func_passthrough(sch);
>      }
> -
> -    return ret;
> +    return IOINST_CC_EXPECTED;
>  }
>  
> -static int do_subchannel_work(SubchDev *sch)
> +static IOInstEnding do_subchannel_work(SubchDev *sch)
>  {
>      if (!sch->do_subchannel_work) {
> -        return -EINVAL;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>      g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
>      return sch->do_subchannel_work(sch);
> @@ -1561,27 +1540,23 @@ static void css_update_chnmon(SubchDev *sch)
>      }
>  }
>  
> -int css_do_ssch(SubchDev *sch, ORB *orb)
> +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (s->ctrl & (SCSW_FCTL_START_FUNC |
>                     SCSW_FCTL_HALT_FUNC |
>                     SCSW_FCTL_CLEAR_FUNC)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_BUSY;
>      }
>  
>      /* If monitoring is active, update counter. */
> @@ -1594,10 +1569,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb)
>      s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND);
>      s->flags &= ~SCSW_FLAGS_MASK_PNO;
>  
> -    ret = do_subchannel_work(sch);
> -
> -out:
> -    return ret;
> +    return do_subchannel_work(sch);
>  }
>  
>  static void copy_irb_to_guest(IRB *dest, const IRB *src, PMCW *pmcw,
> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>      }
>  }
>  
> -int css_do_rsch(SubchDev *sch)
> +IOInstEnding css_do_rsch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
> -        ret = -EINVAL;
> -        goto out;
> +        return IOINST_CC_BUSY;
>      }
>  
>      /* If monitoring is active, update counter. */
> @@ -1873,11 +1841,7 @@ int css_do_rsch(SubchDev *sch)
>      }
>  
>      s->ctrl |= SCSW_ACTL_RESUME_PEND;
> -    do_subchannel_work(sch);
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return do_subchannel_work(sch);
>  }
>  
>  int css_do_rchp(uint8_t cssid, uint8_t chpid)
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 8614dda6f8..0ef232ec27 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -18,15 +18,14 @@
>  #include "hw/s390x/css-bridge.h"
>  #include "hw/s390x/s390-ccw.h"
>  
> -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
>  {
> -    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
> +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>  
> -    if (cdc->handle_request) {
> -        return cdc->handle_request(orb, scsw, data);
> -    } else {
> -        return -ENOSYS;
> +    if (!cdc->handle_request) {
> +        return IOINST_CC_STATUS_PRESENT;
>      }
> +    return cdc->handle_request(sch);
>  }
>  
>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 76323c6bde..1cc2e5d873 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -47,9 +47,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
>      .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
>  };
>  
> -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>  {
> -    S390CCWDevice *cdev = data;
> +    S390CCWDevice *cdev = sch->driver_data;
>      VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>      struct ccw_io_region *region = vcdev->io_region;
>      int ret;
> @@ -60,8 +60,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
>  
>      memset(region, 0, sizeof(*region));
>  
> -    memcpy(region->orb_area, orb, sizeof(ORB));
> -    memcpy(region->scsw_area, scsw, sizeof(SCSW));
> +    memcpy(region->orb_area, &sch->orb, sizeof(ORB));
> +    memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
>  
>  again:
>      ret = pwrite(vcdev->vdev.fd, region,
> @@ -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;
>      }

Checking ret for the negative error values is more idiomatic.

> -
> -    return region->ret_code;
>  }
>  
>  static void vfio_ccw_reset(DeviceState *dev)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 7e0dbd162f..10bde18452 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -136,11 +136,22 @@ struct SubchDev {
>      /* transport-provided data: */
>      int (*ccw_cb) (SubchDev *, CCW1);
>      void (*disable_cb)(SubchDev *);
> -    int (*do_subchannel_work) (SubchDev *);
> +    IOInstEnding (*do_subchannel_work) (SubchDev *);
>      SenseId id;
>      void *driver_data;
>  };
>  
> +static inline void sch_gen_unit_exception(SubchDev *sch)
> +{
> +    sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND;
> +    sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY |
> +                                  SCSW_STCTL_SECONDARY |
> +                                  SCSW_STCTL_ALERT |
> +                                  SCSW_STCTL_STATUS_PEND;
> +    sch->curr_status.scsw.cpa = sch->channel_prog + 8;
> +    sch->curr_status.scsw.dstat =  SCSW_DSTAT_UNIT_EXCEP;
> +}
> +
>  extern const VMStateDescription vmstate_subch_dev;
>  
>  /*
> @@ -199,9 +210,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
>  void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
>  void css_generate_css_crws(uint8_t cssid);
>  void css_clear_sei_pending(void);
> -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data);
> -int do_subchannel_work_virtual(SubchDev *sub);
> -int do_subchannel_work_passthrough(SubchDev *sub);
> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch);
> +IOInstEnding do_subchannel_work_virtual(SubchDev *sub);
> +IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
>  
>  typedef enum {
>      CSS_IO_ADAPTER_VIRTIO = 0,
> @@ -232,7 +243,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *schib);
>  int css_do_xsch(SubchDev *sch);
>  int css_do_csch(SubchDev *sch);
>  int css_do_hsch(SubchDev *sch);
> -int css_do_ssch(SubchDev *sch, ORB *orb);
> +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
>  int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
>  void css_do_tsch_update_subch(SubchDev *sch);
>  int css_do_stcrw(CRW *crw);
> @@ -243,7 +254,7 @@ int css_collect_chp_desc(int m, uint8_t cssid, uint8_t f_chpid, uint8_t l_chpid,
>  void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo);
>  int css_enable_mcsse(void);
>  int css_enable_mss(void);
> -int css_do_rsch(SubchDev *sch);
> +IOInstEnding css_do_rsch(SubchDev *sch);
>  int css_do_rchp(uint8_t cssid, uint8_t chpid);
>  bool css_present(uint8_t cssid);
>  #endif
> diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
> index 9f45cf1347..7d15a1a5d4 100644
> --- a/include/hw/s390x/s390-ccw.h
> +++ b/include/hw/s390x/s390-ccw.h
> @@ -33,7 +33,7 @@ typedef struct S390CCWDeviceClass {
>      CCWDeviceClass parent_class;
>      void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp);
>      void (*unrealize)(S390CCWDevice *dev, Error **errp);
> -    int (*handle_request) (ORB *, SCSW *, void *);
> +    IOInstEnding (*handle_request) (SubchDev *sch);
>  } S390CCWDeviceClass;
>  
>  #endif
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 47490f838a..16b5cf2fed 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -218,8 +218,6 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
>      SubchDev *sch;
>      ORB orig_orb, orb;
>      uint64_t addr;
> -    int ret = -ENODEV;
> -    int cc;
>      CPUS390XState *env = &cpu->env;
>      uint8_t ar;
>  
> @@ -239,33 +237,11 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
>      }
>      trace_ioinst_sch_id("ssch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_ssch(sch, &orb);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case -EFAULT:
> -        /*
> -         * TODO:
> -         * I'm wondering whether there is something better
> -         * to do for us here (like setting some device or
> -         * subchannel status).
> -         */
> -        program_interrupt(env, PGM_ADDRESSING, 4);
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
>          return;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_ssch(sch, &orb));
>  }
>  
>  void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb)
> @@ -784,8 +760,6 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
>  
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -793,24 +767,11 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("rsch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_rsch(sch);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EINVAL:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_rsch(sch));
>  }
>  
>  #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)

Looks sane.

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

[Thomas Huth]

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 <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 28 +++++++++++----
>  include/hw/s390x/css.h      | 23 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++------------------------
>  6 files changed, 75 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index aa233d5f8a..ff5a05c34b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>  
>  }
>  
> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>  {
>  
>      PMCW *p = &sch->curr_status.pmcw;
>      SCSW *s = &sch->curr_status.scsw;
> -    int ret;
>  
>      ORB *orb = &sch->orb;
>      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> @@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>       */
>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -        return -EINVAL;
> +        warn_report("vfio-ccw requires PFCH and C64 flags set...");

Not sure, but should this maybe rather be a
"qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?
Anyway, as Cornelia already mentioned it: Please drop the trailing dots.

> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return IOINST_CC_EXPECTED;
>      }
[...]
> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>      }
>  }
>  
> -int css_do_rsch(SubchDev *sch)
> +IOInstEnding css_do_rsch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
> -        ret = -EINVAL;
> -        goto out;
> +        return IOINST_CC_BUSY;

Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
IOINST_CC_STATUS_PRESENT instead?

>      }
[...]
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 76323c6bde..1cc2e5d873 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -47,9 +47,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
>      .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
>  };
>  
> -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>  {
> -    S390CCWDevice *cdev = data;
> +    S390CCWDevice *cdev = sch->driver_data;
>      VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>      struct ccw_io_region *region = vcdev->io_region;
>      int ret;
> @@ -60,8 +60,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
>  
>      memset(region, 0, sizeof(*region));
>  
> -    memcpy(region->orb_area, orb, sizeof(ORB));
> -    memcpy(region->scsw_area, scsw, sizeof(SCSW));
> +    memcpy(region->orb_area, &sch->orb, sizeof(ORB));
> +    memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
>  
>  again:
>      ret = pwrite(vcdev->vdev.fd, region,
> @@ -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...

 Thomas

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

[Thomas Huth]

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 <pasic@linux.vnet.ibm.com>
[...]
>> @@ -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.

 Thomas

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 <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>  
> [...]
> >> @@ -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.)

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

[Thomas Huth]

On 18.10.2017 11:58, Cornelia Huck wrote:
> On Wed, 18 Oct 2017 11:52:03 +0200
> Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>  
>> [...]
>>>> @@ -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.)

OK, I'm not that familiar with the I/O sub-system. If you say that it is
ok, then simply ignore my comment, please :-)

 Thomas

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

[Cornelia Huck]

On Wed, 18 Oct 2017 11:30:47 +0200
Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
> >  hw/s390x/s390-ccw.c         | 11 +++---
> >  hw/vfio/ccw.c               | 28 +++++++++++----
> >  include/hw/s390x/css.h      | 23 +++++++++----
> >  include/hw/s390x/s390-ccw.h |  2 +-
> >  target/s390x/ioinst.c       | 53 ++++------------------------
> >  6 files changed, 75 insertions(+), 126 deletions(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index aa233d5f8a..ff5a05c34b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
> >  
> >  }
> >  
> > -static int sch_handle_start_func_passthrough(SubchDev *sch)
> > +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> >  {
> >  
> >      PMCW *p = &sch->curr_status.pmcw;
> >      SCSW *s = &sch->curr_status.scsw;
> > -    int ret;
> >  
> >      ORB *orb = &sch->orb;
> >      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> > @@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
> >       */
> >      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> > -        return -EINVAL;
> > +        warn_report("vfio-ccw requires PFCH and C64 flags set...");  
> 
> Not sure, but should this maybe rather be a
> "qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?

Is that visible by default, though? I'd rather want the admin to be
able to find a hint in a log somewhere why the guest I/O is rejected.

> Anyway, as Cornelia already mentioned it: Please drop the trailing dots.
> 
> > +        sch_gen_unit_exception(sch);
> > +        css_inject_io_interrupt(sch);
> > +        return IOINST_CC_EXPECTED;
> >      }  
> [...]
> > @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
> >      }
> >  }
> >  
> > -int css_do_rsch(SubchDev *sch)
> > +IOInstEnding css_do_rsch(SubchDev *sch)
> >  {
> >      SCSW *s = &sch->curr_status.scsw;
> >      PMCW *p = &sch->curr_status.pmcw;
> > -    int ret;
> >  
> >      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> > -        ret = -ENODEV;
> > -        goto out;
> > +        return IOINST_CC_NOT_OPERATIONAL;
> >      }
> >  
> >      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> > -        ret = -EINPROGRESS;
> > -        goto out;
> > +        return IOINST_CC_STATUS_PRESENT;
> >      }
> >  
> >      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
> >          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
> >          (!(s->ctrl & SCSW_ACTL_SUSP))) {
> > -        ret = -EINVAL;
> > -        goto out;
> > +        return IOINST_CC_BUSY;  
> 
> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
> IOINST_CC_STATUS_PRESENT instead?

No, that is correct (see the PoP for when cc 2 is supposed to be set by
rsch).

> 
> >      }  
> [...]
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index 76323c6bde..1cc2e5d873 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -47,9 +47,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
> >      .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
> >  };
> >  
> > -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> > +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
> >  {
> > -    S390CCWDevice *cdev = data;
> > +    S390CCWDevice *cdev = sch->driver_data;
> >      VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> >      struct ccw_io_region *region = vcdev->io_region;
> >      int ret;
> > @@ -60,8 +60,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> >  
> >      memset(region, 0, sizeof(*region));
> >  
> > -    memcpy(region->orb_area, orb, sizeof(ORB));
> > -    memcpy(region->scsw_area, scsw, sizeof(SCSW));
> > +    memcpy(region->orb_area, &sch->orb, sizeof(ORB));
> > +    memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
> >  
> >  again:
> >      ret = pwrite(vcdev->vdev.fd, region,
> > @@ -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...

I think Halil's comments in patch 2 already hint at possibly needing to
add IOINST_EXCEPTION later.

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

[Thomas Huth]

On 18.10.2017 11:52, Cornelia Huck wrote:
> On Wed, 18 Oct 2017 11:30:47 +0200
> Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>
>>> ---
>>>  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
>>>  hw/s390x/s390-ccw.c         | 11 +++---
>>>  hw/vfio/ccw.c               | 28 +++++++++++----
>>>  include/hw/s390x/css.h      | 23 +++++++++----
>>>  include/hw/s390x/s390-ccw.h |  2 +-
>>>  target/s390x/ioinst.c       | 53 ++++------------------------
>>>  6 files changed, 75 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index aa233d5f8a..ff5a05c34b 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>>  
>>>  }
>>>  
>>> -static int sch_handle_start_func_passthrough(SubchDev *sch)
>>> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>>  {
>>>  
>>>      PMCW *p = &sch->curr_status.pmcw;
>>>      SCSW *s = &sch->curr_status.scsw;
>>> -    int ret;
>>>  
>>>      ORB *orb = &sch->orb;
>>>      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
>>> @@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>>>       */
>>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>>> -        return -EINVAL;
>>> +        warn_report("vfio-ccw requires PFCH and C64 flags set...");  
>>
>> Not sure, but should this maybe rather be a
>> "qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?
> 
> Is that visible by default, though? I'd rather want the admin to be
> able to find a hint in a log somewhere why the guest I/O is rejected.

Well, the guest could also trigger this condition on purpose (e.g.
kvm-unit-tests), so I wonder whether we want to see the warning in that
case, too...
IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been
implemented for: Log errors from the guest in case you suspect that the
guest is doing something wrong. But that's just my 0.02 €, feel free to
ignore me.

>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>>>      }
>>>  }
>>>  
>>> -int css_do_rsch(SubchDev *sch)
>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>  {
>>>      SCSW *s = &sch->curr_status.scsw;
>>>      PMCW *p = &sch->curr_status.pmcw;
>>> -    int ret;
>>>  
>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>> -        ret = -ENODEV;
>>> -        goto out;
>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>      }
>>>  
>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>> -        ret = -EINPROGRESS;
>>> -        goto out;
>>> +        return IOINST_CC_STATUS_PRESENT;
>>>      }
>>>  
>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>> -        ret = -EINVAL;
>>> -        goto out;
>>> +        return IOINST_CC_BUSY;  
>>
>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>> IOINST_CC_STATUS_PRESENT instead?
> 
> No, that is correct (see the PoP for when cc 2 is supposed to be set by
> rsch).

So if this is on purpose, this change in behavior should also be
mentioned in the patch description, I think.

 Thomas

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

[Halil Pasic]

On 10/18/2017 12:07 PM, Thomas Huth wrote:
> On 18.10.2017 11:52, Cornelia Huck wrote:
>> On Wed, 18 Oct 2017 11:30:47 +0200
>> Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
>>>>  hw/s390x/s390-ccw.c         | 11 +++---
>>>>  hw/vfio/ccw.c               | 28 +++++++++++----
>>>>  include/hw/s390x/css.h      | 23 +++++++++----
>>>>  include/hw/s390x/s390-ccw.h |  2 +-
>>>>  target/s390x/ioinst.c       | 53 ++++------------------------
>>>>  6 files changed, 75 insertions(+), 126 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index aa233d5f8a..ff5a05c34b 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>>>  
>>>>  }
>>>>  
>>>> -static int sch_handle_start_func_passthrough(SubchDev *sch)
>>>> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>>>  {
>>>>  
>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>>      SCSW *s = &sch->curr_status.scsw;
>>>> -    int ret;
>>>>  
>>>>      ORB *orb = &sch->orb;
>>>>      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
>>>> @@ -1200,31 +1199,12 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>>>>       */
>>>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>>>> -        return -EINVAL;
>>>> +        warn_report("vfio-ccw requires PFCH and C64 flags set...");  
>>>
>>> Not sure, but should this maybe rather be a
>>> "qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?
>>
>> Is that visible by default, though? I'd rather want the admin to be
>> able to find a hint in a log somewhere why the guest I/O is rejected.
> 
> Well, the guest could also trigger this condition on purpose (e.g.
> kvm-unit-tests), so I wonder whether we want to see the warning in that
> case, too...
> IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been
> implemented for: Log errors from the guest in case you suspect that the
> guest is doing something wrong. But that's just my 0.02 €, feel free to
> ignore me.
> 
>>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>>>>      }
>>>>  }
>>>>  
>>>> -int css_do_rsch(SubchDev *sch)
>>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>>  {
>>>>      SCSW *s = &sch->curr_status.scsw;
>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>> -    int ret;
>>>>  
>>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>>> -        ret = -ENODEV;
>>>> -        goto out;
>>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>>      }
>>>>  
>>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>>> -        ret = -EINPROGRESS;
>>>> -        goto out;
>>>> +        return IOINST_CC_STATUS_PRESENT;
>>>>      }
>>>>  
>>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>>> -        ret = -EINVAL;
>>>> -        goto out;
>>>> +        return IOINST_CC_BUSY;  
>>>
>>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>>> IOINST_CC_STATUS_PRESENT instead?
>>
>> No, that is correct (see the PoP for when cc 2 is supposed to be set by
>> rsch).
> 
> So if this is on purpose, this change in behavior should also be
> mentioned in the patch description, I think.
> 

No. have a look at the function ioinst_handle_rsch. It used
to map -EINVAL to cc 2 (and was an oddball in this respect) so we
keep the old behavior, it's just more obvious whats happening.

Regards,
Halil

>  Thomas
> 

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

[Thomas Huth]

On 18.10.2017 13:07, Halil Pasic wrote:
> 
> 
> On 10/18/2017 12:07 PM, Thomas Huth wrote:
>> On 18.10.2017 11:52, Cornelia Huck wrote:
>>> On Wed, 18 Oct 2017 11:30:47 +0200
>>> Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>
[...]
>>>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> -int css_do_rsch(SubchDev *sch)
>>>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>>>  {
>>>>>      SCSW *s = &sch->curr_status.scsw;
>>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>>> -    int ret;
>>>>>  
>>>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>>>> -        ret = -ENODEV;
>>>>> -        goto out;
>>>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>>>      }
>>>>>  
>>>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>>>> -        ret = -EINPROGRESS;
>>>>> -        goto out;
>>>>> +        return IOINST_CC_STATUS_PRESENT;
>>>>>      }
>>>>>  
>>>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>>>> -        ret = -EINVAL;
>>>>> -        goto out;
>>>>> +        return IOINST_CC_BUSY;  
>>>>
>>>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>>>> IOINST_CC_STATUS_PRESENT instead?
>>>
>>> No, that is correct (see the PoP for when cc 2 is supposed to be set by
>>> rsch).
>>
>> So if this is on purpose, this change in behavior should also be
>> mentioned in the patch description, I think.
> 
> No. have a look at the function ioinst_handle_rsch. It used
> to map -EINVAL to cc 2 (and was an oddball in this respect) so we
> keep the old behavior, it's just more obvious whats happening.

Oh, you're right, I missed that different mapping of the error codes!
... so I see, the previous behavior was really confusing and error
prone, it's really good that you're cleaning this up now!

 Thomas

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

[Halil Pasic]

On 10/18/2017 01:12 PM, Thomas Huth wrote:
> On 18.10.2017 13:07, Halil Pasic wrote:
>>
>>
>> On 10/18/2017 12:07 PM, Thomas Huth wrote:
>>> On 18.10.2017 11:52, Cornelia Huck wrote:
>>>> On Wed, 18 Oct 2017 11:30:47 +0200
>>>> Thomas Huth <thuth@redhat.com> 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 <pasic@linux.vnet.ibm.com>
> [...]
>>>>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo)
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> -int css_do_rsch(SubchDev *sch)
>>>>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>>>>  {
>>>>>>      SCSW *s = &sch->curr_status.scsw;
>>>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>>>> -    int ret;
>>>>>>  
>>>>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>>>>> -        ret = -ENODEV;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>>>>      }
>>>>>>  
>>>>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>>>>> -        ret = -EINPROGRESS;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_STATUS_PRESENT;
>>>>>>      }
>>>>>>  
>>>>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>>>>> -        ret = -EINVAL;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_BUSY;  
>>>>>
>>>>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>>>>> IOINST_CC_STATUS_PRESENT instead?
>>>>
>>>> No, that is correct (see the PoP for when cc 2 is supposed to be set by
>>>> rsch).
>>>
>>> So if this is on purpose, this change in behavior should also be
>>> mentioned in the patch description, I think.
>>
>> No. have a look at the function ioinst_handle_rsch. It used
>> to map -EINVAL to cc 2 (and was an oddball in this respect) so we
>> keep the old behavior, it's just more obvious whats happening.
> 
> Oh, you're right, I missed that different mapping of the error codes!
> ... so I see, the previous behavior was really confusing and error
> prone, it's really good that you're cleaning this up now!
> 
>  Thomas
> 

Many thanks for the watchful eyes! In the end I did manage to sneak
in a bug (for MSCH). I prefer a lots of false positives that than a
couple of false negatives (for review).

Regards,
Halil

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

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:49 +0200]:

> 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 <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c              | 84 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 28 +++++++++++----
>  include/hw/s390x/css.h      | 23 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++------------------------
>  6 files changed, 75 insertions(+), 126 deletions(-)
> 

Agree for what already planned to fix when applying.

LGTM:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

[Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler

[Halil Pasic]

Simplify the error handling of the XSCH.  Let the code detecting the
condition tell (in a less ambiguous way) how it's to be handled. No
changes in behavior.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 17 +++++------------
 include/hw/s390x/css.h |  2 +-
 target/s390x/ioinst.c  | 23 ++++-------------------
 3 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index ff5a05c34b..1b3be7729d 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1402,20 +1402,17 @@ out:
     return ret;
 }
 
-int css_do_xsch(SubchDev *sch)
+IOInstEnding css_do_xsch(SubchDev *sch)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
-    int ret;
 
     if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
-        ret = -ENODEV;
-        goto out;
+        return IOINST_CC_NOT_OPERATIONAL;
     }
 
     if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
-        ret = -EINPROGRESS;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     if (!(s->ctrl & SCSW_CTRL_MASK_FCTL) ||
@@ -1423,8 +1420,7 @@ int css_do_xsch(SubchDev *sch)
         (!(s->ctrl &
            (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) ||
         (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) {
-        ret = -EBUSY;
-        goto out;
+        return IOINST_CC_BUSY;
     }
 
     /* Cancel the current operation. */
@@ -1436,10 +1432,7 @@ int css_do_xsch(SubchDev *sch)
     sch->last_cmd_valid = false;
     s->dstat = 0;
     s->cstat = 0;
-    ret = 0;
-
-out:
-    return ret;
+    return IOINST_CC_EXPECTED;
 }
 
 int css_do_csch(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 10bde18452..3c319c9096 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -240,7 +240,7 @@ void css_conditional_io_interrupt(SubchDev *sch);
 int css_do_stsch(SubchDev *sch, SCHIB *schib);
 bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
 int css_do_msch(SubchDev *sch, const SCHIB *schib);
-int css_do_xsch(SubchDev *sch);
+IOInstEnding css_do_xsch(SubchDev *sch);
 int css_do_csch(SubchDev *sch);
 int css_do_hsch(SubchDev *sch);
 IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 16b5cf2fed..4ad07e9181 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -42,8 +42,6 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
-    int ret = -ENODEV;
-    int cc;
 
     if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
@@ -51,24 +49,11 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
     }
     trace_ioinst_sch_id("xsch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_xsch(sch);
-    }
-    switch (ret) {
-    case -ENODEV:
-        cc = 3;
-        break;
-    case -EBUSY:
-        cc = 2;
-        break;
-    case 0:
-        cc = 0;
-        break;
-    default:
-        cc = 1;
-        break;
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
+        return;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_xsch(sch));
 }
 
 void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:50 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Simplify the error handling of the XSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 17 +++++------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 32 deletions(-)

Looks sane (quoting the whole mail for the benefit of the qemu-s390x list).

> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index ff5a05c34b..1b3be7729d 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1402,20 +1402,17 @@ out:
>      return ret;
>  }
>  
> -int css_do_xsch(SubchDev *sch)
> +IOInstEnding css_do_xsch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (!(s->ctrl & SCSW_CTRL_MASK_FCTL) ||
> @@ -1423,8 +1420,7 @@ int css_do_xsch(SubchDev *sch)
>          (!(s->ctrl &
>             (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) ||
>          (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_BUSY;
>      }
>  
>      /* Cancel the current operation. */
> @@ -1436,10 +1432,7 @@ int css_do_xsch(SubchDev *sch)
>      sch->last_cmd_valid = false;
>      s->dstat = 0;
>      s->cstat = 0;
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return IOINST_CC_EXPECTED;
>  }
>  
>  int css_do_csch(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 10bde18452..3c319c9096 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -240,7 +240,7 @@ void css_conditional_io_interrupt(SubchDev *sch);
>  int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>  int css_do_msch(SubchDev *sch, const SCHIB *schib);
> -int css_do_xsch(SubchDev *sch);
> +IOInstEnding css_do_xsch(SubchDev *sch);
>  int css_do_csch(SubchDev *sch);
>  int css_do_hsch(SubchDev *sch);
>  IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 16b5cf2fed..4ad07e9181 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -42,8 +42,6 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
>  
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -51,24 +49,11 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("xsch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_xsch(sch);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_xsch(sch));
>  }
>  
>  void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)

Re: [Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> Simplify the error handling of the XSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 17 +++++------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 32 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:50 +0200]:

> Simplify the error handling of the XSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 17 +++++------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index ff5a05c34b..1b3be7729d 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1402,20 +1402,17 @@ out:
>      return ret;
>  }
> 
> -int css_do_xsch(SubchDev *sch)
> +IOInstEnding css_do_xsch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
> 
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
> 
>      if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
> 
>      if (!(s->ctrl & SCSW_CTRL_MASK_FCTL) ||
> @@ -1423,8 +1420,7 @@ int css_do_xsch(SubchDev *sch)
>          (!(s->ctrl &
>             (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) ||
>          (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_BUSY;
>      }
> 
>      /* Cancel the current operation. */
> @@ -1436,10 +1432,7 @@ int css_do_xsch(SubchDev *sch)
>      sch->last_cmd_valid = false;
>      s->dstat = 0;
>      s->cstat = 0;
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return IOINST_CC_EXPECTED;
>  }
> 
>  int css_do_csch(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 10bde18452..3c319c9096 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -240,7 +240,7 @@ void css_conditional_io_interrupt(SubchDev *sch);
>  int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>  int css_do_msch(SubchDev *sch, const SCHIB *schib);
> -int css_do_xsch(SubchDev *sch);
> +IOInstEnding css_do_xsch(SubchDev *sch);
>  int css_do_csch(SubchDev *sch);
>  int css_do_hsch(SubchDev *sch);
>  IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 16b5cf2fed..4ad07e9181 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -42,8 +42,6 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
> 
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -51,24 +49,11 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("xsch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_xsch(sch);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
?:
s/3/IOINST_CC_NOT_OPERATIONAL/

This also applies to other alike cases.

> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_xsch(sch));
>  }
> 
>  void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> -- 
> 2.13.5
> 

Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler

[Halil Pasic]

On 10/19/2017 08:11 AM, Dong Jia Shi wrote:
>> +    if (!sch || !css_subch_visible(sch)) {
>> +        setcc(cpu, 3);
> ?:
> s/3/IOINST_CC_NOT_OPERATIONAL/
> 
> This also applies to other alike cases.
> 

I do not know. My first reaction was that I'm against because
were IOInstEnding strongly typed, it would not work as set
cc takes an int and works not just for IO instructions.

OTOH the same goes for setcc(cpu, css_do_xsch(sch)) so that
ain't really an argument, and it would improve grepability.

So I think I'm in favor now.

Halil

[Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler

[Halil Pasic]

Simplify the error handling of the cSCH.  Let the code detecting the
condition tell (in a less ambiguous way) how it's to be handled. No
changes in behavior.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 12 +++---------
 include/hw/s390x/css.h |  2 +-
 target/s390x/ioinst.c  | 14 ++++----------
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 1b3be7729d..9935d48b72 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1435,26 +1435,20 @@ IOInstEnding css_do_xsch(SubchDev *sch)
     return IOINST_CC_EXPECTED;
 }
 
-int css_do_csch(SubchDev *sch)
+IOInstEnding css_do_csch(SubchDev *sch)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
-    int ret;
 
     if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
-        ret = -ENODEV;
-        goto out;
+        return IOINST_CC_NOT_OPERATIONAL;
     }
 
     /* Trigger the clear function. */
     s->ctrl &= ~(SCSW_CTRL_MASK_FCTL | SCSW_CTRL_MASK_ACTL);
     s->ctrl |= SCSW_FCTL_CLEAR_FUNC | SCSW_ACTL_CLEAR_PEND;
 
-    do_subchannel_work(sch);
-    ret = 0;
-
-out:
-    return ret;
+    return do_subchannel_work(sch);
 }
 
 int css_do_hsch(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 3c319c9096..f38c59c9b7 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -241,7 +241,7 @@ int css_do_stsch(SubchDev *sch, SCHIB *schib);
 bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
 int css_do_msch(SubchDev *sch, const SCHIB *schib);
 IOInstEnding css_do_xsch(SubchDev *sch);
-int css_do_csch(SubchDev *sch);
+IOInstEnding css_do_csch(SubchDev *sch);
 int css_do_hsch(SubchDev *sch);
 IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
 int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 4ad07e9181..fd659e77a5 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -60,8 +60,6 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
-    int ret = -ENODEV;
-    int cc;
 
     if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
@@ -69,15 +67,11 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
     }
     trace_ioinst_sch_id("csch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_csch(sch);
-    }
-    if (ret == -ENODEV) {
-        cc = 3;
-    } else {
-        cc = 0;
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
+        return;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_csch(sch));
 }
 
 void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Simplify the error handling of the cSCH.  Let the code detecting the

s/cSCH/CSCH/

> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 12 +++---------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 14 ++++----------
>  3 files changed, 8 insertions(+), 20 deletions(-)

Looks sane (quoting the whole mail for the benefit of the qemu-s390x list).

> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 1b3be7729d..9935d48b72 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1435,26 +1435,20 @@ IOInstEnding css_do_xsch(SubchDev *sch)
>      return IOINST_CC_EXPECTED;
>  }
>  
> -int css_do_csch(SubchDev *sch)
> +IOInstEnding css_do_csch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      /* Trigger the clear function. */
>      s->ctrl &= ~(SCSW_CTRL_MASK_FCTL | SCSW_CTRL_MASK_ACTL);
>      s->ctrl |= SCSW_FCTL_CLEAR_FUNC | SCSW_ACTL_CLEAR_PEND;
>  
> -    do_subchannel_work(sch);
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return do_subchannel_work(sch);
>  }
>  
>  int css_do_hsch(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 3c319c9096..f38c59c9b7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -241,7 +241,7 @@ int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>  int css_do_msch(SubchDev *sch, const SCHIB *schib);
>  IOInstEnding css_do_xsch(SubchDev *sch);
> -int css_do_csch(SubchDev *sch);
> +IOInstEnding css_do_csch(SubchDev *sch);
>  int css_do_hsch(SubchDev *sch);
>  IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
>  int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 4ad07e9181..fd659e77a5 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -60,8 +60,6 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
>  
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -69,15 +67,11 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("csch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_csch(sch);
> -    }
> -    if (ret == -ENODEV) {
> -        cc = 3;
> -    } else {
> -        cc = 0;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_csch(sch));
>  }
>  
>  void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)

Re: [Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> Simplify the error handling of the cSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 12 +++---------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 14 ++++----------
>  3 files changed, 8 insertions(+), 20 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:51 +0200]:

> Simplify the error handling of the cSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
[...]

> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 4ad07e9181..fd659e77a5 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -60,8 +60,6 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
> 
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -69,15 +67,11 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("csch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_csch(sch);
> -    }
> -    if (ret == -ENODEV) {
> -        cc = 3;
> -    } else {
> -        cc = 0;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
Same question here.

> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_csch(sch));
>  }
> 
>  void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
> -- 
> 2.13.5
> 

If we agree to replace 3 with IOINST_CC_NOT_OPERATIONAL, maybe Conny
could fix it. Or we can do that in a following cleanup patch?

W/ or w/o the fix, for now:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler

[Cornelia Huck]

On Thu, 19 Oct 2017 14:14:45 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:51 +0200]:
> 
> > Simplify the error handling of the cSCH.  Let the code detecting the
> > condition tell (in a less ambiguous way) how it's to be handled. No
> > changes in behavior.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---  
> [...]
> 
> > diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> > index 4ad07e9181..fd659e77a5 100644
> > --- a/target/s390x/ioinst.c
> > +++ b/target/s390x/ioinst.c
> > @@ -60,8 +60,6 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> >  {
> >      int cssid, ssid, schid, m;
> >      SubchDev *sch;
> > -    int ret = -ENODEV;
> > -    int cc;
> > 
> >      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
> >          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> > @@ -69,15 +67,11 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> >      }
> >      trace_ioinst_sch_id("csch", cssid, ssid, schid);
> >      sch = css_find_subch(m, cssid, ssid, schid);
> > -    if (sch && css_subch_visible(sch)) {
> > -        ret = css_do_csch(sch);
> > -    }
> > -    if (ret == -ENODEV) {
> > -        cc = 3;
> > -    } else {
> > -        cc = 0;
> > +    if (!sch || !css_subch_visible(sch)) {
> > +        setcc(cpu, 3);  
> Same question here.
> 
> > +        return;
> >      }
> > -    setcc(cpu, cc);
> > +    setcc(cpu, css_do_csch(sch));
> >  }
> > 
> >  void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
> > -- 
> > 2.13.5
> >   
> 
> If we agree to replace 3 with IOINST_CC_NOT_OPERATIONAL, maybe Conny
> could fix it. Or we can do that in a following cleanup patch?

Switching it to IOINST_CC_NOT_OPERATIONAL is certainly good, but I
think I've already done enough editing for this series...

A patch on top would be the best way.

> 
> W/ or w/o the fix, for now:
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 

[Qemu-devel] [PATCH v3 6/7] s390x: refactor error handling for HSCH handler

[Halil Pasic]

Simplify the error handling of the HSCH.  Let the code detecting the
condition tell (in a less ambiguous way) how it's to be handled. No
changes in behavior.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 18 +++++-------------
 include/hw/s390x/css.h |  2 +-
 target/s390x/ioinst.c  | 23 ++++-------------------
 3 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 9935d48b72..b9e0329825 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1451,28 +1451,24 @@ IOInstEnding css_do_csch(SubchDev *sch)
     return do_subchannel_work(sch);
 }
 
-int css_do_hsch(SubchDev *sch)
+IOInstEnding css_do_hsch(SubchDev *sch)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
-    int ret;
 
     if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
-        ret = -ENODEV;
-        goto out;
+        return IOINST_CC_NOT_OPERATIONAL;
     }
 
     if (((s->ctrl & SCSW_CTRL_MASK_STCTL) == SCSW_STCTL_STATUS_PEND) ||
         (s->ctrl & (SCSW_STCTL_PRIMARY |
                     SCSW_STCTL_SECONDARY |
                     SCSW_STCTL_ALERT))) {
-        ret = -EINPROGRESS;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     if (s->ctrl & (SCSW_FCTL_HALT_FUNC | SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return IOINST_CC_BUSY;
     }
 
     /* Trigger the halt function. */
@@ -1485,11 +1481,7 @@ int css_do_hsch(SubchDev *sch)
     }
     s->ctrl |= SCSW_ACTL_HALT_PEND;
 
-    do_subchannel_work(sch);
-    ret = 0;
-
-out:
-    return ret;
+    return do_subchannel_work(sch);
 }
 
 static void css_update_chnmon(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index f38c59c9b7..0c1efbae39 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -242,7 +242,7 @@ bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
 int css_do_msch(SubchDev *sch, const SCHIB *schib);
 IOInstEnding css_do_xsch(SubchDev *sch);
 IOInstEnding css_do_csch(SubchDev *sch);
-int css_do_hsch(SubchDev *sch);
+IOInstEnding css_do_hsch(SubchDev *sch);
 IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
 int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
 void css_do_tsch_update_subch(SubchDev *sch);
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index fd659e77a5..8e4dacafd8 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -78,8 +78,6 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
-    int ret = -ENODEV;
-    int cc;
 
     if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
@@ -87,24 +85,11 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
     }
     trace_ioinst_sch_id("hsch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_hsch(sch);
-    }
-    switch (ret) {
-    case -ENODEV:
-        cc = 3;
-        break;
-    case -EBUSY:
-        cc = 2;
-        break;
-    case 0:
-        cc = 0;
-        break;
-    default:
-        cc = 1;
-        break;
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
+        return;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_hsch(sch));
 }
 
 static int ioinst_schib_valid(SCHIB *schib)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 6/7] s390x: refactor error handling for HSCH handler

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:52 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Simplify the error handling of the HSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 18 +++++-------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)

Looks sane (quoting the whole mail for the benefit of the qemu-s390x list).

> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 9935d48b72..b9e0329825 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1451,28 +1451,24 @@ IOInstEnding css_do_csch(SubchDev *sch)
>      return do_subchannel_work(sch);
>  }
>  
> -int css_do_hsch(SubchDev *sch)
> +IOInstEnding css_do_hsch(SubchDev *sch)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
> -    int ret;
>  
>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -        ret = -ENODEV;
> -        goto out;
> +        return IOINST_CC_NOT_OPERATIONAL;
>      }
>  
>      if (((s->ctrl & SCSW_CTRL_MASK_STCTL) == SCSW_STCTL_STATUS_PEND) ||
>          (s->ctrl & (SCSW_STCTL_PRIMARY |
>                      SCSW_STCTL_SECONDARY |
>                      SCSW_STCTL_ALERT))) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (s->ctrl & (SCSW_FCTL_HALT_FUNC | SCSW_FCTL_CLEAR_FUNC)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_BUSY;
>      }
>  
>      /* Trigger the halt function. */
> @@ -1485,11 +1481,7 @@ int css_do_hsch(SubchDev *sch)
>      }
>      s->ctrl |= SCSW_ACTL_HALT_PEND;
>  
> -    do_subchannel_work(sch);
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return do_subchannel_work(sch);
>  }
>  
>  static void css_update_chnmon(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index f38c59c9b7..0c1efbae39 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -242,7 +242,7 @@ bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>  int css_do_msch(SubchDev *sch, const SCHIB *schib);
>  IOInstEnding css_do_xsch(SubchDev *sch);
>  IOInstEnding css_do_csch(SubchDev *sch);
> -int css_do_hsch(SubchDev *sch);
> +IOInstEnding css_do_hsch(SubchDev *sch);
>  IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
>  int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);
>  void css_do_tsch_update_subch(SubchDev *sch);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index fd659e77a5..8e4dacafd8 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -78,8 +78,6 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
>  {
>      int cssid, ssid, schid, m;
>      SubchDev *sch;
> -    int ret = -ENODEV;
> -    int cc;
>  
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
>          program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -87,24 +85,11 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("hsch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_hsch(sch);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_hsch(sch));
>  }
>  
>  static int ioinst_schib_valid(SCHIB *schib)

Re: [Qemu-devel] [PATCH v3 6/7] s390x: refactor error handling for HSCH handler

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> Simplify the error handling of the HSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 18 +++++-------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 6/7] s390x: refactor error handling for HSCH handler

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:52 +0200]:

> Simplify the error handling of the HSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 18 +++++-------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)
> 
[...]

> @@ -87,24 +85,11 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
>      }
>      trace_ioinst_sch_id("hsch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_hsch(sch);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
Ditto.

> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_hsch(sch));
>  }
> 
>  static int ioinst_schib_valid(SCHIB *schib)
> -- 
> 2.13.5
> 

Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

[Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Halil Pasic]

Simplify the error handling of the MSCH.  Let the code detecting the
condition tell (in a less ambiguous way) how it's to be handled. No
changes in behavior.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 18 +++++-------------
 include/hw/s390x/css.h |  2 +-
 target/s390x/ioinst.c  | 23 ++++-------------------
 3 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index b9e0329825..30fc236946 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
     }
 }
 
-int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
+IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
 {
     SCSW *s = &sch->curr_status.scsw;
     PMCW *p = &sch->curr_status.pmcw;
     uint16_t oldflags;
-    int ret;
     SCHIB schib;
 
     if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
-        ret = 0;
-        goto out;
+        return IOINST_CC_EXPECTED;
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     if (s->ctrl &
         (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return IOINST_CC_STATUS_PRESENT;
     }
 
     copy_schib_from_guest(&schib, orig_schib);
@@ -1395,11 +1391,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
         && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) {
         sch->disable_cb(sch);
     }
-
-    ret = 0;
-
-out:
-    return ret;
+    return IOINST_CC_EXPECTED;
 }
 
 IOInstEnding css_do_xsch(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 0c1efbae39..a74f6cc274 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -239,7 +239,7 @@ bool css_subch_visible(SubchDev *sch);
 void css_conditional_io_interrupt(SubchDev *sch);
 int css_do_stsch(SubchDev *sch, SCHIB *schib);
 bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
-int css_do_msch(SubchDev *sch, const SCHIB *schib);
+IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib);
 IOInstEnding css_do_xsch(SubchDev *sch);
 IOInstEnding css_do_csch(SubchDev *sch);
 IOInstEnding css_do_hsch(SubchDev *sch);
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 8e4dacafd8..23962fbebc 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -111,8 +111,6 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
     SubchDev *sch;
     SCHIB schib;
     uint64_t addr;
-    int ret = -ENODEV;
-    int cc;
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
@@ -131,24 +129,11 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
     }
     trace_ioinst_sch_id("msch", cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
-    if (sch && css_subch_visible(sch)) {
-        ret = css_do_msch(sch, &schib);
-    }
-    switch (ret) {
-    case -ENODEV:
-        cc = 3;
-        break;
-    case -EBUSY:
-        cc = 2;
-        break;
-    case 0:
-        cc = 0;
-        break;
-    default:
-        cc = 1;
-        break;
+    if (!sch || !css_subch_visible(sch)) {
+        setcc(cpu, 3);
+        return;
     }
-    setcc(cpu, cc);
+    setcc(cpu, css_do_msch(sch, &schib));
 }
 
 static void copy_orb_from_guest(ORB *dest, const ORB *src)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:53 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Simplify the error handling of the MSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 18 +++++-------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)

Looks sane (quoting the whole mail for the benefit of the qemu-s390x list).

> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index b9e0329825..30fc236946 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
>      }
>  }
>  
> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
>      uint16_t oldflags;
> -    int ret;
>      SCHIB schib;
>  
>      if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
> -        ret = 0;
> -        goto out;
> +        return IOINST_CC_EXPECTED;
>      }
>  
>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (s->ctrl &
>          (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      copy_schib_from_guest(&schib, orig_schib);
> @@ -1395,11 +1391,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
>          && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) {
>          sch->disable_cb(sch);
>      }
> -
> -    ret = 0;
> -
> -out:
> -    return ret;
> +    return IOINST_CC_EXPECTED;
>  }
>  
>  IOInstEnding css_do_xsch(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0c1efbae39..a74f6cc274 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -239,7 +239,7 @@ bool css_subch_visible(SubchDev *sch);
>  void css_conditional_io_interrupt(SubchDev *sch);
>  int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
> -int css_do_msch(SubchDev *sch, const SCHIB *schib);
> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib);
>  IOInstEnding css_do_xsch(SubchDev *sch);
>  IOInstEnding css_do_csch(SubchDev *sch);
>  IOInstEnding css_do_hsch(SubchDev *sch);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 8e4dacafd8..23962fbebc 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -111,8 +111,6 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
>      SubchDev *sch;
>      SCHIB schib;
>      uint64_t addr;
> -    int ret = -ENODEV;
> -    int cc;
>      CPUS390XState *env = &cpu->env;
>      uint8_t ar;
>  
> @@ -131,24 +129,11 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb)
>      }
>      trace_ioinst_sch_id("msch", cssid, ssid, schid);
>      sch = css_find_subch(m, cssid, ssid, schid);
> -    if (sch && css_subch_visible(sch)) {
> -        ret = css_do_msch(sch, &schib);
> -    }
> -    switch (ret) {
> -    case -ENODEV:
> -        cc = 3;
> -        break;
> -    case -EBUSY:
> -        cc = 2;
> -        break;
> -    case 0:
> -        cc = 0;
> -        break;
> -    default:
> -        cc = 1;
> -        break;
> +    if (!sch || !css_subch_visible(sch)) {
> +        setcc(cpu, 3);
> +        return;
>      }
> -    setcc(cpu, cc);
> +    setcc(cpu, css_do_msch(sch, &schib));
>  }
>  
>  static void copy_orb_from_guest(ORB *dest, const ORB *src)

Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Thomas Huth]

On 17.10.2017 16:04, Halil Pasic wrote:
> Simplify the error handling of the MSCH.  Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.

ok, so you claim no changes in behavior ...

> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 18 +++++-------------
>  include/hw/s390x/css.h |  2 +-
>  target/s390x/ioinst.c  | 23 ++++-------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index b9e0329825..30fc236946 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
>      }
>  }
>  
> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
>  {
>      SCSW *s = &sch->curr_status.scsw;
>      PMCW *p = &sch->curr_status.pmcw;
>      uint16_t oldflags;
> -    int ret;
>      SCHIB schib;
>  
>      if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
> -        ret = 0;
> -        goto out;
> +        return IOINST_CC_EXPECTED;
>      }
>  
>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> -        ret = -EINPROGRESS;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }
>  
>      if (s->ctrl &
>          (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
> -        ret = -EBUSY;
> -        goto out;
> +        return IOINST_CC_STATUS_PRESENT;
>      }

... but here you change -EBUSY (which got mapped to CC=2) to
CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
this is either a bug, or you should update the patch description with a
justification for this change in behavior.

 Thomas

Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Cornelia Huck]

On Wed, 18 Oct 2017 12:00:05 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17.10.2017 16:04, Halil Pasic wrote:
> > Simplify the error handling of the MSCH.  Let the code detecting the
> > condition tell (in a less ambiguous way) how it's to be handled. No
> > changes in behavior.  
> 
> ok, so you claim no changes in behavior ...
> 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/css.c         | 18 +++++-------------
> >  include/hw/s390x/css.h |  2 +-
> >  target/s390x/ioinst.c  | 23 ++++-------------------
> >  3 files changed, 10 insertions(+), 33 deletions(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index b9e0329825..30fc236946 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
> >      }
> >  }
> >  
> > -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> > +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> >  {
> >      SCSW *s = &sch->curr_status.scsw;
> >      PMCW *p = &sch->curr_status.pmcw;
> >      uint16_t oldflags;
> > -    int ret;
> >      SCHIB schib;
> >  
> >      if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
> > -        ret = 0;
> > -        goto out;
> > +        return IOINST_CC_EXPECTED;
> >      }
> >  
> >      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> > -        ret = -EINPROGRESS;
> > -        goto out;
> > +        return IOINST_CC_STATUS_PRESENT;
> >      }
> >  
> >      if (s->ctrl &
> >          (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
> > -        ret = -EBUSY;
> > -        goto out;
> > +        return IOINST_CC_STATUS_PRESENT;
> >      }  
> 
> ... but here you change -EBUSY (which got mapped to CC=2) to
> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
> this is either a bug, or you should update the patch description with a
> justification for this change in behavior.

Indeed, that's a bug. We still want cc 2.

Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Halil Pasic]

On 10/18/2017 12:02 PM, Cornelia Huck wrote:
> On Wed, 18 Oct 2017 12:00:05 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17.10.2017 16:04, Halil Pasic wrote:
>>> Simplify the error handling of the MSCH.  Let the code detecting the
>>> condition tell (in a less ambiguous way) how it's to be handled. No
>>> changes in behavior.  
>>
>> ok, so you claim no changes in behavior ...
>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>  hw/s390x/css.c         | 18 +++++-------------
>>>  include/hw/s390x/css.h |  2 +-
>>>  target/s390x/ioinst.c  | 23 ++++-------------------
>>>  3 files changed, 10 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index b9e0329825..30fc236946 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
>>>      }
>>>  }
>>>  
>>> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
>>> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
>>>  {
>>>      SCSW *s = &sch->curr_status.scsw;
>>>      PMCW *p = &sch->curr_status.pmcw;
>>>      uint16_t oldflags;
>>> -    int ret;
>>>      SCHIB schib;
>>>  
>>>      if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
>>> -        ret = 0;
>>> -        goto out;
>>> +        return IOINST_CC_EXPECTED;
>>>      }
>>>  
>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>> -        ret = -EINPROGRESS;
>>> -        goto out;
>>> +        return IOINST_CC_STATUS_PRESENT;
>>>      }
>>>  
>>>      if (s->ctrl &
>>>          (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
>>> -        ret = -EBUSY;
>>> -        goto out;
>>> +        return IOINST_CC_STATUS_PRESENT;
>>>      }  
>>
>> ... but here you change -EBUSY (which got mapped to CC=2) to
>> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
>> this is either a bug, or you should update the patch description with a
>> justification for this change in behavior.
> 
> Indeed, that's a bug. We still want cc 2.
> 

Agree, it is a bug. It was OK in v1 but was already buggy in
v2.

Conny can you fix this up as well please?

Thanks in advance!

Halil

Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-18 13:01:03 +0200]:

> 
> 
> On 10/18/2017 12:02 PM, Cornelia Huck wrote:
> > On Wed, 18 Oct 2017 12:00:05 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> > 
> >> On 17.10.2017 16:04, Halil Pasic wrote:
> >>> Simplify the error handling of the MSCH.  Let the code detecting the
> >>> condition tell (in a less ambiguous way) how it's to be handled. No
> >>> changes in behavior.  
> >>
> >> ok, so you claim no changes in behavior ...
> >>
> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>> ---
> >>>  hw/s390x/css.c         | 18 +++++-------------
> >>>  include/hw/s390x/css.h |  2 +-
> >>>  target/s390x/ioinst.c  | 23 ++++-------------------
> >>>  3 files changed, 10 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index b9e0329825..30fc236946 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
> >>>      }
> >>>  }
> >>>  
> >>> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> >>> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> >>>  {
> >>>      SCSW *s = &sch->curr_status.scsw;
> >>>      PMCW *p = &sch->curr_status.pmcw;
> >>>      uint16_t oldflags;
> >>> -    int ret;
> >>>      SCHIB schib;
> >>>  
> >>>      if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
> >>> -        ret = 0;
> >>> -        goto out;
> >>> +        return IOINST_CC_EXPECTED;
> >>>      }
> >>>  
> >>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> >>> -        ret = -EINPROGRESS;
> >>> -        goto out;
> >>> +        return IOINST_CC_STATUS_PRESENT;
> >>>      }
> >>>  
> >>>      if (s->ctrl &
> >>>          (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
> >>> -        ret = -EBUSY;
> >>> -        goto out;
> >>> +        return IOINST_CC_STATUS_PRESENT;
> >>>      }  
> >>
> >> ... but here you change -EBUSY (which got mapped to CC=2) to
> >> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
> >> this is either a bug, or you should update the patch description with a
> >> justification for this change in behavior.
> > 
> > Indeed, that's a bug. We still want cc 2.
> > 
> 
> Agree, it is a bug. It was OK in v1 but was already buggy in
> v2.
> 
> Conny can you fix this up as well please?
> 
> Thanks in advance!
> 
I saw Conny fixed this in her branch. So:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:46 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Abstract
> =======
> 
> The basic idea is: tell how to handle an unusual condition where it's
> identified, instead of mapping it to an errno (more or less arbitrarily),
> then possibly mapping these errnos around, to finally (mentally) map the
> errno back to the condition and take appropriate action.
> 
> According to Dong Jia the patch-set also has a functional value: for ccw
> pass-through, he is planing to pass-through the instruction completion
> information (cc or interruption condition) from the kernel, and this
> patch set can pretty much be seen as a preparation for that.
> 
> Changelog
> =========
> 
> Patch 1 should be already applied to Conny's tree. I've included it
> nevertheless so guys working on top of current master have everything in
> place.
> 
> v2 -> v3:
> * somewhat uwillingly traded the type safe struct to a somewhat
>   type safe enum (because considered ugly) (Thomas, Conny)
> * dropped 'template approach' patch which intended to further
>   consolidate IO inst. handlers having lot's of logic and code in
>   common (Conny)
> * added warning if vfio-ccw ORB does not have the flags required
>   by the vfio-ccw implementation as suggested (Dong Jia)
> * got rid of some unintentional changes (Dong Jia)
> * reworded some stuff (comments, commit messages) (Dong Jia)
> 
> v1 -> v2:
> * use assert if do_subchannel_work without any functions being
>   accepted 
> * generate unit-exception if ccw-vfio can't handle an otherwise
>   good channel program (due to extra limitations) 
> * keep using return values opposed to recording into SubchDev
> * split out 'add infrastructure' from 'refactor first handler'
> * reworded some commit messanges and comments
> * rebased on top of current master
> * dropped r-b's and acks because of the magnitude of the
>   changes
> 
> Testing
> =======
> 
> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> testing, especially regarding the changes in vfio-ccw (patch #3).

Looks sane to me (if needed, I can fix up the minor things I found).

In addition to some testing, I'd appreciate some review from others as
well.

> 
> Halil Pasic (7):
>   s390x/css: be more consistent if broken beyond repair
>   s390x/css: IO instr handler ending control
>   s390x: improve error handling for SSCH and RSCH
>   s390x: refactor error handling for XSCH handler
>   s390x: refactor error handling for CSCH handler
>   s390x: refactor error handling for HSCH handler
>   s390x: refactor error handling for MSCH handler
> 
>  hw/s390x/css.c              | 163 ++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         |  11 ++-
>  hw/vfio/ccw.c               |  28 ++++++--
>  include/hw/s390x/css.h      |  47 ++++++++++---
>  include/hw/s390x/s390-ccw.h |   2 +-
>  target/s390x/ioinst.c       | 136 +++++++-----------------------------
>  6 files changed, 132 insertions(+), 255 deletions(-)
> 

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Halil Pasic]

On 10/17/2017 05:13 PM, Cornelia Huck wrote:
> On Tue, 17 Oct 2017 16:04:46 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Abstract
>> =======
>>
>> The basic idea is: tell how to handle an unusual condition where it's
>> identified, instead of mapping it to an errno (more or less arbitrarily),
>> then possibly mapping these errnos around, to finally (mentally) map the
>> errno back to the condition and take appropriate action.
>>
>> According to Dong Jia the patch-set also has a functional value: for ccw
>> pass-through, he is planing to pass-through the instruction completion
>> information (cc or interruption condition) from the kernel, and this
>> patch set can pretty much be seen as a preparation for that.
>>
>> Changelog
>> =========
>>
>> Patch 1 should be already applied to Conny's tree. I've included it
>> nevertheless so guys working on top of current master have everything in
>> place.
>>
>> v2 -> v3:
>> * somewhat uwillingly traded the type safe struct to a somewhat
>>   type safe enum (because considered ugly) (Thomas, Conny)
>> * dropped 'template approach' patch which intended to further
>>   consolidate IO inst. handlers having lot's of logic and code in
>>   common (Conny)
>> * added warning if vfio-ccw ORB does not have the flags required
>>   by the vfio-ccw implementation as suggested (Dong Jia)
>> * got rid of some unintentional changes (Dong Jia)
>> * reworded some stuff (comments, commit messages) (Dong Jia)
>>
>> v1 -> v2:
>> * use assert if do_subchannel_work without any functions being
>>   accepted 
>> * generate unit-exception if ccw-vfio can't handle an otherwise
>>   good channel program (due to extra limitations) 
>> * keep using return values opposed to recording into SubchDev
>> * split out 'add infrastructure' from 'refactor first handler'
>> * reworded some commit messanges and comments
>> * rebased on top of current master
>> * dropped r-b's and acks because of the magnitude of the
>>   changes
>>
>> Testing
>> =======
>>
>> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
>> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
>> testing, especially regarding the changes in vfio-ccw (patch #3).
> 
> Looks sane to me (if needed, I can fix up the minor things I found).
> 
> In addition to some testing, I'd appreciate some review from others as
> well.
> 

Of course, I'm fine with the fixes (won't answer individually). I think
both Dong Jia and Pierre have already put enough work in this to be credited
with a tag, so I really hope they will get around to review this. I would
be especially happy with an Tested-by: Dong Jia since this series is quite
under-tested, and the changes in vfio-ccw aren't just minor.

Of course I could come up with a test setup myself, but I hope Dong Jia
already has one, and he is certainly more involved with vfio-ccw.

Regards,
Halil

>>
>> Halil Pasic (7):
>>   s390x/css: be more consistent if broken beyond repair
>>   s390x/css: IO instr handler ending control
>>   s390x: improve error handling for SSCH and RSCH
>>   s390x: refactor error handling for XSCH handler
>>   s390x: refactor error handling for CSCH handler
>>   s390x: refactor error handling for HSCH handler
>>   s390x: refactor error handling for MSCH handler
>>
>>  hw/s390x/css.c              | 163 ++++++++++++--------------------------------
>>  hw/s390x/s390-ccw.c         |  11 ++-
>>  hw/vfio/ccw.c               |  28 ++++++--
>>  include/hw/s390x/css.h      |  47 ++++++++++---
>>  include/hw/s390x/s390-ccw.h |   2 +-
>>  target/s390x/ioinst.c       | 136 +++++++-----------------------------
>>  6 files changed, 132 insertions(+), 255 deletions(-)
>>
> 
> 

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Cornelia Huck]

On Tue, 17 Oct 2017 18:19:20 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 10/17/2017 05:13 PM, Cornelia Huck wrote:
> > On Tue, 17 Oct 2017 16:04:46 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> Abstract
> >> =======
> >>
> >> The basic idea is: tell how to handle an unusual condition where it's
> >> identified, instead of mapping it to an errno (more or less arbitrarily),
> >> then possibly mapping these errnos around, to finally (mentally) map the
> >> errno back to the condition and take appropriate action.
> >>
> >> According to Dong Jia the patch-set also has a functional value: for ccw
> >> pass-through, he is planing to pass-through the instruction completion
> >> information (cc or interruption condition) from the kernel, and this
> >> patch set can pretty much be seen as a preparation for that.
> >>
> >> Changelog
> >> =========
> >>
> >> Patch 1 should be already applied to Conny's tree. I've included it
> >> nevertheless so guys working on top of current master have everything in
> >> place.
> >>
> >> v2 -> v3:
> >> * somewhat uwillingly traded the type safe struct to a somewhat
> >>   type safe enum (because considered ugly) (Thomas, Conny)
> >> * dropped 'template approach' patch which intended to further
> >>   consolidate IO inst. handlers having lot's of logic and code in
> >>   common (Conny)
> >> * added warning if vfio-ccw ORB does not have the flags required
> >>   by the vfio-ccw implementation as suggested (Dong Jia)
> >> * got rid of some unintentional changes (Dong Jia)
> >> * reworded some stuff (comments, commit messages) (Dong Jia)
> >>
> >> v1 -> v2:
> >> * use assert if do_subchannel_work without any functions being
> >>   accepted 
> >> * generate unit-exception if ccw-vfio can't handle an otherwise
> >>   good channel program (due to extra limitations) 
> >> * keep using return values opposed to recording into SubchDev
> >> * split out 'add infrastructure' from 'refactor first handler'
> >> * reworded some commit messanges and comments
> >> * rebased on top of current master
> >> * dropped r-b's and acks because of the magnitude of the
> >>   changes
> >>
> >> Testing
> >> =======
> >>
> >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> >> testing, especially regarding the changes in vfio-ccw (patch #3).  
> > 
> > Looks sane to me (if needed, I can fix up the minor things I found).
> > 
> > In addition to some testing, I'd appreciate some review from others as
> > well.
> >   
> 
> Of course, I'm fine with the fixes (won't answer individually). I think
> both Dong Jia and Pierre have already put enough work in this to be credited
> with a tag, so I really hope they will get around to review this. I would
> be especially happy with an Tested-by: Dong Jia since this series is quite
> under-tested, and the changes in vfio-ccw aren't just minor.
> 
> Of course I could come up with a test setup myself, but I hope Dong Jia
> already has one, and he is certainly more involved with vfio-ccw.

FTR: I'll wait until tomorrow for more tags and then go ahead and apply
(well, if no problem comes up in the meantime). I need to get a pull
request out of the door this week.

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 18:19:20 +0200]:

[...]

> >> Testing
> >> =======
> >>
> >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> >> testing, especially regarding the changes in vfio-ccw (patch #3).
> > 
> > Looks sane to me (if needed, I can fix up the minor things I found).
> > 
> > In addition to some testing, I'd appreciate some review from others as
> > well.
> > 
> 
> Of course, I'm fine with the fixes (won't answer individually). I think
> both Dong Jia and Pierre have already put enough work in this to be credited
> with a tag, so I really hope they will get around to review this. I would
> be especially happy with an Tested-by: Dong Jia since this series is quite
> under-tested, and the changes in vfio-ccw aren't just minor.
> 
> Of course I could come up with a test setup myself, but I hope Dong Jia
> already has one, and he is certainly more involved with vfio-ccw.
> 
Using Conny's s390-next branch + this series (#2-#7), I didn't notice
any obvious problem during my fio test.  So for the vfio-ccw related
part:
Tested-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

[...]

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Cornelia Huck]

On Wed, 18 Oct 2017 16:23:47 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 18:19:20 +0200]:
> 
> [...]
> 
> > >> Testing
> > >> =======
> > >>
> > >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> > >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> > >> testing, especially regarding the changes in vfio-ccw (patch #3).  
> > > 
> > > Looks sane to me (if needed, I can fix up the minor things I found).
> > > 
> > > In addition to some testing, I'd appreciate some review from others as
> > > well.
> > >   
> > 
> > Of course, I'm fine with the fixes (won't answer individually). I think
> > both Dong Jia and Pierre have already put enough work in this to be credited
> > with a tag, so I really hope they will get around to review this. I would
> > be especially happy with an Tested-by: Dong Jia since this series is quite
> > under-tested, and the changes in vfio-ccw aren't just minor.
> > 
> > Of course I could come up with a test setup myself, but I hope Dong Jia
> > already has one, and he is certainly more involved with vfio-ccw.
> >   
> Using Conny's s390-next branch + this series (#2-#7), I didn't notice
> any obvious problem during my fio test.  So for the vfio-ccw related
> part:
> Tested-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

Thanks!

To which patches may I add the tag? :)

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-10-18 11:53:10 +0200]:

> On Wed, 18 Oct 2017 16:23:47 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 18:19:20 +0200]:
> > 
> > [...]
> > 
> > > >> Testing
> > > >> =======
> > > >>
> > > >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> > > >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> > > >> testing, especially regarding the changes in vfio-ccw (patch #3).  
> > > > 
> > > > Looks sane to me (if needed, I can fix up the minor things I found).
> > > > 
> > > > In addition to some testing, I'd appreciate some review from others as
> > > > well.
> > > >   
> > > 
> > > Of course, I'm fine with the fixes (won't answer individually). I think
> > > both Dong Jia and Pierre have already put enough work in this to be credited
> > > with a tag, so I really hope they will get around to review this. I would
> > > be especially happy with an Tested-by: Dong Jia since this series is quite
> > > under-tested, and the changes in vfio-ccw aren't just minor.
> > > 
> > > Of course I could come up with a test setup myself, but I hope Dong Jia
> > > already has one, and he is certainly more involved with vfio-ccw.
> > >   
> > Using Conny's s390-next branch + this series (#2-#7), I didn't notice
> > any obvious problem during my fio test.  So for the vfio-ccw related
> > part:
> > Tested-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 
> Thanks!
> 
> To which patches may I add the tag? :)
> 
The test should cover the vfio-ccw related part in patch #3, I assume.
So add it there?

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:46 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Abstract
> =======
> 
> The basic idea is: tell how to handle an unusual condition where it's
> identified, instead of mapping it to an errno (more or less arbitrarily),
> then possibly mapping these errnos around, to finally (mentally) map the
> errno back to the condition and take appropriate action.
> 
> According to Dong Jia the patch-set also has a functional value: for ccw
> pass-through, he is planing to pass-through the instruction completion
> information (cc or interruption condition) from the kernel, and this
> patch set can pretty much be seen as a preparation for that.
> 
> Changelog
> =========
> 
> Patch 1 should be already applied to Conny's tree. I've included it
> nevertheless so guys working on top of current master have everything in
> place.
> 
> v2 -> v3:
> * somewhat uwillingly traded the type safe struct to a somewhat
>   type safe enum (because considered ugly) (Thomas, Conny)
> * dropped 'template approach' patch which intended to further
>   consolidate IO inst. handlers having lot's of logic and code in
>   common (Conny)
> * added warning if vfio-ccw ORB does not have the flags required
>   by the vfio-ccw implementation as suggested (Dong Jia)
> * got rid of some unintentional changes (Dong Jia)
> * reworded some stuff (comments, commit messages) (Dong Jia)
> 
> v1 -> v2:
> * use assert if do_subchannel_work without any functions being
>   accepted 
> * generate unit-exception if ccw-vfio can't handle an otherwise
>   good channel program (due to extra limitations) 
> * keep using return values opposed to recording into SubchDev
> * split out 'add infrastructure' from 'refactor first handler'
> * reworded some commit messanges and comments
> * rebased on top of current master
> * dropped r-b's and acks because of the magnitude of the
>   changes
> 
> Testing
> =======
> 
> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> testing, especially regarding the changes in vfio-ccw (patch #3).
> 
> Halil Pasic (7):
>   s390x/css: be more consistent if broken beyond repair
>   s390x/css: IO instr handler ending control
>   s390x: improve error handling for SSCH and RSCH
>   s390x: refactor error handling for XSCH handler
>   s390x: refactor error handling for CSCH handler
>   s390x: refactor error handling for HSCH handler
>   s390x: refactor error handling for MSCH handler
> 
>  hw/s390x/css.c              | 163 ++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         |  11 ++-
>  hw/vfio/ccw.c               |  28 ++++++--
>  include/hw/s390x/css.h      |  47 ++++++++++---
>  include/hw/s390x/s390-ccw.h |   2 +-
>  target/s390x/ioinst.c       | 136 +++++++-----------------------------
>  6 files changed, 132 insertions(+), 255 deletions(-)
> 

FYI: I pushed out what I currently have to

git://github.com/cohuck/qemu ioinst-retcode

Feedback appreciated.

Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:46 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Abstract
> =======
> 
> The basic idea is: tell how to handle an unusual condition where it's
> identified, instead of mapping it to an errno (more or less arbitrarily),
> then possibly mapping these errnos around, to finally (mentally) map the
> errno back to the condition and take appropriate action.
> 
> According to Dong Jia the patch-set also has a functional value: for ccw
> pass-through, he is planing to pass-through the instruction completion
> information (cc or interruption condition) from the kernel, and this
> patch set can pretty much be seen as a preparation for that.
> 
> Changelog
> =========
> 
> Patch 1 should be already applied to Conny's tree. I've included it
> nevertheless so guys working on top of current master have everything in
> place.
> 
> v2 -> v3:
> * somewhat uwillingly traded the type safe struct to a somewhat
>   type safe enum (because considered ugly) (Thomas, Conny)
> * dropped 'template approach' patch which intended to further
>   consolidate IO inst. handlers having lot's of logic and code in
>   common (Conny)
> * added warning if vfio-ccw ORB does not have the flags required
>   by the vfio-ccw implementation as suggested (Dong Jia)
> * got rid of some unintentional changes (Dong Jia)
> * reworded some stuff (comments, commit messages) (Dong Jia)
> 
> v1 -> v2:
> * use assert if do_subchannel_work without any functions being
>   accepted 
> * generate unit-exception if ccw-vfio can't handle an otherwise
>   good channel program (due to extra limitations) 
> * keep using return values opposed to recording into SubchDev
> * split out 'add infrastructure' from 'refactor first handler'
> * reworded some commit messanges and comments
> * rebased on top of current master
> * dropped r-b's and acks because of the magnitude of the
>   changes
> 
> Testing
> =======
> 
> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2
> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper
> testing, especially regarding the changes in vfio-ccw (patch #3).
> 
> Halil Pasic (7):
>   s390x/css: be more consistent if broken beyond repair
>   s390x/css: IO instr handler ending control
>   s390x: improve error handling for SSCH and RSCH
>   s390x: refactor error handling for XSCH handler
>   s390x: refactor error handling for CSCH handler
>   s390x: refactor error handling for HSCH handler
>   s390x: refactor error handling for MSCH handler
> 
>  hw/s390x/css.c              | 163 ++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         |  11 ++-
>  hw/vfio/ccw.c               |  28 ++++++--
>  include/hw/s390x/css.h      |  47 ++++++++++---
>  include/hw/s390x/s390-ccw.h |   2 +-
>  target/s390x/ioinst.c       | 136 +++++++-----------------------------
>  6 files changed, 132 insertions(+), 255 deletions(-)
> 

Thanks, applied.