[Qemu-devel] [PATCH v2 0/8] improve error handling for IO instr

[Qemu-devel] [PATCH v2 0/8] improve error handling for IO instr

[Halil Pasic]

This is the v2 of my infamous nameless series (archive link:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06076.html).

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
=========

There was a fair amount of discussion regarding during the review of v1.
For certain issues we have decided to address them differently (e.g. use
unit-exception for VFIO if extra constraints are not respected, get rid
of the addressing check, assert if do_subchannel_work called without
improperly) and for some we decided to not address them for now (e.g.
use if callbacks are null).

Although this was not part of the v1 discussion, I have also decided to
go back to return values (opposed to adding new state to SubchDev). There
are multiple reasons for doing so: 
* Most of, but not all IO instructions
operate on a subchannel, thus having the instruction ending control in
SubchDev is a bit weird. 
* Get both the compiler and the good habits on ours side (e.g. non-void
control reaches end of non-void function warning (-Wreturn-type), no
overwrites possible, instead of having to check a certain member of a
certain struct after a function call just don't ignore return values).

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
=======

Regarding vfio-ccw Dong Jia did some quick testing with an equivalent
version. Regarding virtual ccw I did only some rudimentary testing.
Should the series be deemed promising more testing would make
sense.

Halil Pasic (8):
  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
  s390x: factor out common ioinst handler logic

 hw/s390x/css.c              | 162 ++++++++++++------------------------------
 hw/s390x/s390-ccw.c         |  11 ++-
 hw/vfio/ccw.c               |  30 ++++++--
 include/hw/s390x/css.h      |  44 +++++++++---
 include/hw/s390x/s390-ccw.h |   2 +-
 target/s390x/ioinst.c       | 169 +++++++-------------------------------------
 6 files changed, 131 insertions(+), 287 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v2 1/8] 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>
---
 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 c59be1aad1..4f47dbc8b0 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1067,9 +1067,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;
@@ -1077,22 +1074,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;
@@ -1100,11 +1092,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 v2 1/8] s390x/css: be more consistent if broken beyond repair

[Dong Jia Shi]

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

> Calling do_subchannel_work with no function control flags set in SCSW is
> a programming error. Currently the handle this differently in
?:
s/the/we/

> 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>
> ---
>  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 c59be1aad1..4f47dbc8b0 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1067,9 +1067,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;
> @@ -1077,22 +1074,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;
A bit surprise to see these. I'm fine with these changes though.

>      } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
>          ret = sch_handle_start_func_passthrough(sch);
> -    } else {
> -        /* Cannot happen. */
> -        return -ENODEV;
>      }
> 
>      return ret;
> @@ -1100,11 +1092,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)

With the fix of the message:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia Shi

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

[Cornelia Huck]

On Wed,  4 Oct 2017 17:41:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> 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>
> ---
>  hw/s390x/css.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)

Thanks, applied.

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

[Halil Pasic]

On 10/10/2017 03:25 PM, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 17:41:37 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> 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>
>> ---
>>  hw/s390x/css.c | 16 ++++------------
>>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> Thanks, applied.
> 

Thank you!

Halil

[Qemu-devel] [PATCH v2 2/8] 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 struct 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 0653d3c9be..66916b6546 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -75,6 +75,18 @@ typedef struct CMBE {
     uint32_t reserved[7];
 } QEMU_PACKED CMBE;
 
+/* IO instructions conclude according this */
+typedef struct IOInstEnding {
+        /*
+         * General semantic of cc codes of IO instructions is (brief):
+         * 0 -- produced expected result
+         * 1 --  status conditions were present or produced alternate result
+         * 2 -- ineffective, because busy with previously initiated function
+         * 3 -- ineffective, not operational
+         */
+        int cc;
+} IOInstEnding;
+
 typedef struct SubchDev SubchDev;
 struct SubchDev {
     /* channel-subsystem related things: */
-- 
2.13.5

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

[Thomas Huth]

On 04.10.2017 17:41, Halil Pasic wrote:
> CSS code needs to tell the IO instruction handlers located in how should

located in how?

> 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 struct 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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..66916b6546 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,18 @@ typedef struct CMBE {
>      uint32_t reserved[7];
>  } QEMU_PACKED CMBE;
>  
> +/* IO instructions conclude according this */
> +typedef struct IOInstEnding {
> +        /*
> +         * General semantic of cc codes of IO instructions is (brief):
> +         * 0 -- produced expected result
> +         * 1 --  status conditions were present or produced alternate result
> +         * 2 -- ineffective, because busy with previously initiated function
> +         * 3 -- ineffective, not operational
> +         */
> +        int cc;
> +} IOInstEnding;

Why do you need a struct for this? Do you plan to extend it later? If
so, I think you should mention that in the patch description. If not,
please use a named enum or a "typedef unsigned int IOInstEnding" instead.

 Thomas

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

[Halil Pasic]

On 10/09/2017 10:20 AM, Thomas Huth wrote:
> On 04.10.2017 17:41, Halil Pasic wrote:
>> CSS code needs to tell the IO instruction handlers located in how should
> 
> located in how?
> 

First, thanks for your review!

Wanted to say: in target/s390x/ioinst.c just forgot to copy paste.

>> 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 struct 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 | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index 0653d3c9be..66916b6546 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
>> @@ -75,6 +75,18 @@ typedef struct CMBE {
>>      uint32_t reserved[7];
>>  } QEMU_PACKED CMBE;
>>  
>> +/* IO instructions conclude according this */
>> +typedef struct IOInstEnding {
>> +        /*
>> +         * General semantic of cc codes of IO instructions is (brief):
>> +         * 0 -- produced expected result
>> +         * 1 --  status conditions were present or produced alternate result
>> +         * 2 -- ineffective, because busy with previously initiated function
>> +         * 3 -- ineffective, not operational
>> +         */
>> +        int cc;
>> +} IOInstEnding;
> 
> Why do you need a struct for this? Do you plan to extend it later? If
> so, I think you should mention that in the patch description. If not,
> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
> 
>  Thomas

We may, we may not. In the previous version we also had to support
do end a certain instruction with an addressing exception, but this
is going away in patch #3. Honestly I don't expect this being extended.

I have other reasons for the struct. Type safety and clear semantics,
and frankly at least for s390 and linux I don't see any downsides given
what is written in the "zSeries ELF Application Binary Interface Supplement".
Can you please explain to me what is the problem with using this struct, and
what is the benefit switching to a unsigned int?

Regards,
Halil
> 

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

[Thomas Huth]

On 09.10.2017 12:54, Halil Pasic wrote:
> 
> 
> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>> On 04.10.2017 17:41, Halil Pasic wrote:
>>> CSS code needs to tell the IO instruction handlers located in how should
>>
>> located in how?
>>
> 
> First, thanks for your review!
> 
> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste.
> 
>>> 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 struct 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 | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 0653d3c9be..66916b6546 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -75,6 +75,18 @@ typedef struct CMBE {
>>>      uint32_t reserved[7];
>>>  } QEMU_PACKED CMBE;
>>>  
>>> +/* IO instructions conclude according this */
>>> +typedef struct IOInstEnding {
>>> +        /*
>>> +         * General semantic of cc codes of IO instructions is (brief):
>>> +         * 0 -- produced expected result
>>> +         * 1 --  status conditions were present or produced alternate result
>>> +         * 2 -- ineffective, because busy with previously initiated function
>>> +         * 3 -- ineffective, not operational
>>> +         */
>>> +        int cc;
>>> +} IOInstEnding;
>>
>> Why do you need a struct for this? Do you plan to extend it later? If
>> so, I think you should mention that in the patch description. If not,
>> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
>>
>>  Thomas
> 
> We may, we may not. In the previous version we also had to support
> do end a certain instruction with an addressing exception, but this
> is going away in patch #3. Honestly I don't expect this being extended.
> 
> I have other reasons for the struct. Type safety and clear semantics,
> and frankly at least for s390 and linux I don't see any downsides given
> what is written in the "zSeries ELF Application Binary Interface Supplement".
> Can you please explain to me what is the problem with using this struct, and
> what is the benefit switching to a unsigned int?

First, returning a struct is ugly in most cases, since it might need to
be passed on the stack if it is bigger than 8 bytes. Ok, that's likely
not the case here (if the compiler / ABI is smart enough - I did not
check), but still, if I see something like this, there is an alarm
signal somewhere in my head that starts to ring...

Then, in the follow up patches, you do something like this:

   return (IOInstEnding){.cc = 0};

... and that just looks very, very ugly in my eyes. The more I look at
it, the more I think we really want to have a named enum instead. That
will give you some sort of basic type safety and semantics, too, and
we'll also get proper names for those magic values - otherwise I'll
always have to look up what cc = 2 or cc = 3 means... (I always keep
forgetting what each value means...)

 Thomas

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

[Halil Pasic]

On 10/09/2017 01:07 PM, Thomas Huth wrote:
> On 09.10.2017 12:54, Halil Pasic wrote:
>>
>>
>> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>>> On 04.10.2017 17:41, Halil Pasic wrote:
>>>> CSS code needs to tell the IO instruction handlers located in how should
>>>
>>> located in how?
>>>
>>
>> First, thanks for your review!
>>
>> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste.
>>
>>>> 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 struct 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 | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>>> index 0653d3c9be..66916b6546 100644
>>>> --- a/include/hw/s390x/css.h
>>>> +++ b/include/hw/s390x/css.h
>>>> @@ -75,6 +75,18 @@ typedef struct CMBE {
>>>>      uint32_t reserved[7];
>>>>  } QEMU_PACKED CMBE;
>>>>  
>>>> +/* IO instructions conclude according this */
>>>> +typedef struct IOInstEnding {
>>>> +        /*
>>>> +         * General semantic of cc codes of IO instructions is (brief):
>>>> +         * 0 -- produced expected result
>>>> +         * 1 --  status conditions were present or produced alternate result
>>>> +         * 2 -- ineffective, because busy with previously initiated function
>>>> +         * 3 -- ineffective, not operational
>>>> +         */
>>>> +        int cc;
>>>> +} IOInstEnding;
>>>
>>> Why do you need a struct for this? Do you plan to extend it later? If
>>> so, I think you should mention that in the patch description. If not,
>>> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
>>>
>>>  Thomas
>>
>> We may, we may not. In the previous version we also had to support
>> do end a certain instruction with an addressing exception, but this
>> is going away in patch #3. Honestly I don't expect this being extended.
>>
>> I have other reasons for the struct. Type safety and clear semantics,
>> and frankly at least for s390 and linux I don't see any downsides given
>> what is written in the "zSeries ELF Application Binary Interface Supplement".
>> Can you please explain to me what is the problem with using this struct, and
>> what is the benefit switching to a unsigned int?
> 
> First, returning a struct is ugly in most cases, since it might need to
> be passed on the stack if it is bigger than 8 bytes. Ok, that's likely
> not the case here (if the compiler / ABI is smart enough - I did not
> check), but still, if I see something like this, there is an alarm
> signal somewhere in my head that starts to ring...
>

Yeah, the ABI is smart enough (where it matters) and this one is obviously
less that 8 bytes. I read this as you  assumed that the return won't be
passed via register (general purpose register 2 for a z host + ELF assumed),
and that would have been ugly indeed.

Btw I have seen putting an integral into a struct for type checking
in the linux kernel too. For instance from:
arch/s390/include/asm/page.h

"""
/*
 * These are used to make use of C type-checking..
 */

typedef struct { unsigned long pgprot; } pgprot_t;
typedef struct { unsigned long pgste; } pgste_t;
typedef struct { unsigned long pte; } pte_t;
typedef struct { unsigned long pmd; } pmd_t;
typedef struct { unsigned long pud; } pud_t;
typedef struct { unsigned long pgd; } pgd_t;
typedef pte_t *pgtable_t;

#define pgprot_val(x)   ((x).pgprot)
#define pgste_val(x)    ((x).pgste)
#define pte_val(x)      ((x).pte)
#define pmd_val(x)      ((x).pmd)
#define pud_val(x)      ((x).pud)
#define pgd_val(x)      ((x).pgd)

#define __pgste(x)      ((pgste_t) { (x) } )
#define __pte(x)        ((pte_t) { (x) } )
#define __pmd(x)        ((pmd_t) { (x) } )
#define __pud(x)        ((pud_t) { (x) } )
#define __pgd(x)        ((pgd_t) { (x) } )
#define __pgprot(x)     ((pgprot_t) { (x) } )
"""

If you think, I could add a similar comment indicating that my
struct is also just for type-checking.

> Then, in the follow up patches, you do something like this:
> 
>    return (IOInstEnding){.cc = 0};
> 
> ... and that just looks very, very ugly in my eyes. The more I look at

Interesting, I found this quite expressive.

> it, the more I think we really want to have a named enum instead. That
> will give you some sort of basic type safety and semantics, too, and

AFAIK we don't have strongly typed enums in C, at least not from
the language perspective. In c++ we got enum class and enum struct
with c++11 for that reason.

> we'll also get proper names for those magic values - otherwise I'll
> always have to look up what cc = 2 or cc = 3 means... (I always keep
> forgetting what each value means...)

Can you suggest some proper names for those magic values? Unfortunately
the PoP refers to this stuff as setting condition code [0-3], so in my
reading the numbers are the canonical names for this stuff. The best
'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3.

IMHO better names and type safety are two different problems, so if you
do come up with better names, we can use that regardless of the type
safety solution.

I'm also not sure when do we tend to use defines and when enums. This
is clearly a the value associated with the tag matters mightily situation
so I would at least want to assign explicit values to the individual
enumerator constants.

Sorry, I may be a bit to persistent on this one: I don't think it's
a huge difference, but I don't feel great about changing something to
what I think is (slightly) worse without being first convinced that
I was wrong.

Regards,
Halil

> 
>  Thomas
> 

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

[Thomas Huth]

On 09.10.2017 17:00, Halil Pasic wrote:
> 
> 
> On 10/09/2017 01:07 PM, Thomas Huth wrote:
>> On 09.10.2017 12:54, Halil Pasic wrote:
>>>
>>>
>>> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>>>> On 04.10.2017 17:41, Halil Pasic wrote:
>>>>> CSS code needs to tell the IO instruction handlers located in how should
>>>>
>>>> located in how?
>>>>
>>>
>>> First, thanks for your review!
>>>
>>> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste.
>>>
>>>>> 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 struct 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 | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>>>> index 0653d3c9be..66916b6546 100644
>>>>> --- a/include/hw/s390x/css.h
>>>>> +++ b/include/hw/s390x/css.h
>>>>> @@ -75,6 +75,18 @@ typedef struct CMBE {
>>>>>      uint32_t reserved[7];
>>>>>  } QEMU_PACKED CMBE;
>>>>>  
>>>>> +/* IO instructions conclude according this */
>>>>> +typedef struct IOInstEnding {
>>>>> +        /*
>>>>> +         * General semantic of cc codes of IO instructions is (brief):
>>>>> +         * 0 -- produced expected result
>>>>> +         * 1 --  status conditions were present or produced alternate result
>>>>> +         * 2 -- ineffective, because busy with previously initiated function
>>>>> +         * 3 -- ineffective, not operational
>>>>> +         */
>>>>> +        int cc;
>>>>> +} IOInstEnding;
>>>>
>>>> Why do you need a struct for this? Do you plan to extend it later? If
>>>> so, I think you should mention that in the patch description. If not,
>>>> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
>>>>
>>>>  Thomas
>>>
>>> We may, we may not. In the previous version we also had to support
>>> do end a certain instruction with an addressing exception, but this
>>> is going away in patch #3. Honestly I don't expect this being extended.
>>>
>>> I have other reasons for the struct. Type safety and clear semantics,
>>> and frankly at least for s390 and linux I don't see any downsides given
>>> what is written in the "zSeries ELF Application Binary Interface Supplement".
>>> Can you please explain to me what is the problem with using this struct, and
>>> what is the benefit switching to a unsigned int?
>>
>> First, returning a struct is ugly in most cases, since it might need to
>> be passed on the stack if it is bigger than 8 bytes. Ok, that's likely
>> not the case here (if the compiler / ABI is smart enough - I did not
>> check), but still, if I see something like this, there is an alarm
>> signal somewhere in my head that starts to ring...
>>
> 
> Yeah, the ABI is smart enough (where it matters) and this one is obviously
> less that 8 bytes. I read this as you  assumed that the return won't be
> passed via register (general purpose register 2 for a z host + ELF assumed),
> and that would have been ugly indeed.
> 
> Btw I have seen putting an integral into a struct for type checking
> in the linux kernel too. For instance from:
> arch/s390/include/asm/page.h
> 
> """
> /*
>  * These are used to make use of C type-checking..
>  */
> 
> typedef struct { unsigned long pgprot; } pgprot_t;
> typedef struct { unsigned long pgste; } pgste_t;
> typedef struct { unsigned long pte; } pte_t;
> typedef struct { unsigned long pmd; } pmd_t;
> typedef struct { unsigned long pud; } pud_t;
> typedef struct { unsigned long pgd; } pgd_t;
> typedef pte_t *pgtable_t;
> 
> #define pgprot_val(x)   ((x).pgprot)
> #define pgste_val(x)    ((x).pgste)
> #define pte_val(x)      ((x).pte)
> #define pmd_val(x)      ((x).pmd)
> #define pud_val(x)      ((x).pud)
> #define pgd_val(x)      ((x).pgd)
> 
> #define __pgste(x)      ((pgste_t) { (x) } )
> #define __pte(x)        ((pte_t) { (x) } )
> #define __pmd(x)        ((pmd_t) { (x) } )
> #define __pud(x)        ((pud_t) { (x) } )
> #define __pgd(x)        ((pgd_t) { (x) } )
> #define __pgprot(x)     ((pgprot_t) { (x) } )
> """
> 
> If you think, I could add a similar comment indicating that my
> struct is also just for type-checking.

No, I think you've got the reason here slightly wrong. The kernel only
uses this since the pte_t and friends need to be urgently portable
between the different architectures. Have a look at the kernel
Documentation/CodingStyle file, it explicitly mentions this in chapter 5
("Typedefs").

>> Then, in the follow up patches, you do something like this:
>>
>>    return (IOInstEnding){.cc = 0};
>>
>> ... and that just looks very, very ugly in my eyes. The more I look at
> 
> Interesting, I found this quite expressive.

C'mon, we're writing C code, not Java ;-)

>> it, the more I think we really want to have a named enum instead. That
>> will give you some sort of basic type safety and semantics, too, and
> 
> AFAIK we don't have strongly typed enums in C, at least not from
> the language perspective. In c++ we got enum class and enum struct
> with c++11 for that reason.
> 
>> we'll also get proper names for those magic values - otherwise I'll
>> always have to look up what cc = 2 or cc = 3 means... (I always keep
>> forgetting what each value means...)
> 
> Can you suggest some proper names for those magic values? Unfortunately
> the PoP refers to this stuff as setting condition code [0-3], so in my
> reading the numbers are the canonical names for this stuff. The best
> 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3.

Well, you already gave a description in your comment in the  struct
IOInstEnding, so maybe something similar? Or maybe this could even be
merged with the definitions for the SIGP status codes:

#define SIGP_CC_ORDER_CODE_ACCEPTED 0
#define SIGP_CC_STATUS_STORED       1
#define SIGP_CC_BUSY                2
#define SIGP_CC_NOT_OPERATIONAL     3

?

> Sorry, I may be a bit to persistent on this one: I don't think it's
> a huge difference, but I don't feel great about changing something to
> what I think is (slightly) worse without being first convinced that
> I was wrong.

In the end, the code has to be accepted by the maintainers, so let's
leave the decision up to them whether they like this typedef struct
IOInstEnding or not...

 Thomas

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

[Cornelia Huck]

On Tue, 10 Oct 2017 12:28:35 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 09.10.2017 17:00, Halil Pasic wrote:
> > 
> > 
> > On 10/09/2017 01:07 PM, Thomas Huth wrote:  

> >> Then, in the follow up patches, you do something like this:
> >>
> >>    return (IOInstEnding){.cc = 0};
> >>
> >> ... and that just looks very, very ugly in my eyes. The more I look at  
> > 
> > Interesting, I found this quite expressive.  
> 
> C'mon, we're writing C code, not Java ;-)

Every time I read that construct, I die a little bit inside...

> Well, you already gave a description in your comment in the  struct
> IOInstEnding, so maybe something similar? Or maybe this could even be
> merged with the definitions for the SIGP status codes:
> 
> #define SIGP_CC_ORDER_CODE_ACCEPTED 0
> #define SIGP_CC_STATUS_STORED       1
> #define SIGP_CC_BUSY                2
> #define SIGP_CC_NOT_OPERATIONAL     3

I'd rather not reuse the definitions for a different instruction, even
if they are similar in semantics.

> > Sorry, I may be a bit to persistent on this one: I don't think it's
> > a huge difference, but I don't feel great about changing something to
> > what I think is (slightly) worse without being first convinced that
> > I was wrong.  
> 
> In the end, the code has to be accepted by the maintainers, so let's
> leave the decision up to them whether they like this typedef struct
> IOInstEnding or not...

Here's a strong 'do not like' from me... using an enum or define is
fine with me.

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

[Halil Pasic]

On 10/10/2017 01:39 PM, Cornelia Huck wrote:
> On Tue, 10 Oct 2017 12:28:35 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 09.10.2017 17:00, Halil Pasic wrote:
>>>
>>>
>>> On 10/09/2017 01:07 PM, Thomas Huth wrote:  
> 
>>>> Then, in the follow up patches, you do something like this:
>>>>
>>>>    return (IOInstEnding){.cc = 0};
>>>>
>>>> ... and that just looks very, very ugly in my eyes. The more I look at  
>>>
>>> Interesting, I found this quite expressive.  
>>
>> C'mon, we're writing C code, not Java ;-)
> 
> Every time I read that construct, I die a little bit inside...
> 
>> Well, you already gave a description in your comment in the  struct
>> IOInstEnding, so maybe something similar? Or maybe this could even be
>> merged with the definitions for the SIGP status codes:
>>
>> #define SIGP_CC_ORDER_CODE_ACCEPTED 0
>> #define SIGP_CC_STATUS_STORED       1
>> #define SIGP_CC_BUSY                2
>> #define SIGP_CC_NOT_OPERATIONAL     3
> 
> I'd rather not reuse the definitions for a different instruction, even
> if they are similar in semantics.
> 
>>> Sorry, I may be a bit to persistent on this one: I don't think it's
>>> a huge difference, but I don't feel great about changing something to
>>> what I think is (slightly) worse without being first convinced that
>>> I was wrong.  
>>
>> In the end, the code has to be accepted by the maintainers, so let's
>> leave the decision up to them whether they like this typedef struct
>> IOInstEnding or not...
> 
> Here's a strong 'do not like' from me... using an enum or define is
> fine with me.
> 

Got the message. Could we first reach an agreement on the rest of the
series? As I've said, I might need to go back to indicating exceptions
too (depending on how do we like #3), and that would mean a changed
situation. If the price for getting this in is sacrificing my strongly
type checked condition code type I can live with that.

Halil
t

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

[Halil Pasic]

[..]
>>
>> Yeah, the ABI is smart enough (where it matters) and this one is obviously
>> less that 8 bytes. I read this as you  assumed that the return won't be
>> passed via register (general purpose register 2 for a z host + ELF assumed),
>> and that would have been ugly indeed.
>>
>> Btw I have seen putting an integral into a struct for type checking
>> in the linux kernel too. For instance from:
>> arch/s390/include/asm/page.h
>>
>> """
>> /*
>>  * These are used to make use of C type-checking..
>>  */
>>
>> typedef struct { unsigned long pgprot; } pgprot_t;
>> typedef struct { unsigned long pgste; } pgste_t;
>> typedef struct { unsigned long pte; } pte_t;
>> typedef struct { unsigned long pmd; } pmd_t;
>> typedef struct { unsigned long pud; } pud_t;
>> typedef struct { unsigned long pgd; } pgd_t;
>> typedef pte_t *pgtable_t;
>>
>> #define pgprot_val(x)   ((x).pgprot)
>> #define pgste_val(x)    ((x).pgste)
>> #define pte_val(x)      ((x).pte)
>> #define pmd_val(x)      ((x).pmd)
>> #define pud_val(x)      ((x).pud)
>> #define pgd_val(x)      ((x).pgd)
>>
>> #define __pgste(x)      ((pgste_t) { (x) } )
>> #define __pte(x)        ((pte_t) { (x) } )
>> #define __pmd(x)        ((pmd_t) { (x) } )
>> #define __pud(x)        ((pud_t) { (x) } )
>> #define __pgd(x)        ((pgd_t) { (x) } )
>> #define __pgprot(x)     ((pgprot_t) { (x) } )
>> """
>>
>> If you think, I could add a similar comment indicating that my
>> struct is also just for type-checking.
> 
> No, I think you've got the reason here slightly wrong. The kernel only
> uses this since the pte_t and friends need to be urgently portable
> between the different architectures. Have a look at the kernel
> Documentation/CodingStyle file, it explicitly mentions this in chapter 5
> ("Typedefs").
>

Disclaimer: I agree with your conclusion, the maintainers will have
to make the final call here. I will still reply point by point for
the fun of pointless academic discussions.

IMHO we don't talk about typedefs here but about type checking. Btw QEMU
has the exact opposite policy regarding typedefs and structs than Linux.
You want to say that the comment at the top of my quote is wrong, and
that it should be rather "These are used for hiding representation.."
that "These are used to make use of C type-checking.."?

I'm also curious which of the rules would your original suggestion of
doing "typedef unsigned int IOInstEnding" match (from chapter 5
"Typedefs")? ;)

 
>>> Then, in the follow up patches, you do something like this:
>>>
>>>    return (IOInstEnding){.cc = 0};
>>>
>>> ... and that just looks very, very ugly in my eyes. The more I look at
>>
>> Interesting, I found this quite expressive.
> 
> C'mon, we're writing C code, not Java ;-)

Yeah, me written much C++ and also Java before might be a part
of the problem. Btw there is no such construct in Java AFAIR :P.
> 
>>> it, the more I think we really want to have a named enum instead. That
>>> will give you some sort of basic type safety and semantics, too, and
>>
>> AFAIK we don't have strongly typed enums in C, at least not from
>> the language perspective. In c++ we got enum class and enum struct
>> with c++11 for that reason.
>>
>>> we'll also get proper names for those magic values - otherwise I'll
>>> always have to look up what cc = 2 or cc = 3 means... (I always keep
>>> forgetting what each value means...)
>>
>> Can you suggest some proper names for those magic values? Unfortunately
>> the PoP refers to this stuff as setting condition code [0-3], so in my
>> reading the numbers are the canonical names for this stuff. The best
>> 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3.
> 
> Well, you already gave a description in your comment in the  struct
> IOInstEnding, so maybe something similar? Or maybe this could even be
> merged with the definitions for the SIGP status codes:
> 
> #define SIGP_CC_ORDER_CODE_ACCEPTED 0
> #define SIGP_CC_STATUS_STORED       1
> #define SIGP_CC_BUSY                2
> #define SIGP_CC_NOT_OPERATIONAL     3
> 
> ?
> 

Maybe.

>> Sorry, I may be a bit to persistent on this one: I don't think it's
>> a huge difference, but I don't feel great about changing something to
>> what I think is (slightly) worse without being first convinced that
>> I was wrong.
> 
> In the end, the code has to be accepted by the maintainers, so let's
> leave the decision up to them whether they like this typedef struct
> IOInstEnding or not...

I agree. Especially if the change in behavior in #3 does not get trough
I will have to revert indicating exceptions too (like in v1), and then
I really need the struct (which can be still less than 8 bytes).

I assume you have seen my reply to Connie about the benefit: among others
it prevents using something like -EFAULT as a cc by accident which neither
an enum nor a typedef does.

In practice, I think either of the proposed solutions will do.

Halil
 
> 
>  Thomas
> 

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

[Thomas Huth]

On 10.10.2017 13:41, Halil Pasic wrote:
> [..]
>>>
>>> Yeah, the ABI is smart enough (where it matters) and this one is obviously
>>> less that 8 bytes. I read this as you  assumed that the return won't be
>>> passed via register (general purpose register 2 for a z host + ELF assumed),
>>> and that would have been ugly indeed.
>>>
>>> Btw I have seen putting an integral into a struct for type checking
>>> in the linux kernel too. For instance from:
>>> arch/s390/include/asm/page.h
>>>
>>> """
>>> /*
>>>  * These are used to make use of C type-checking..
>>>  */
>>>
>>> typedef struct { unsigned long pgprot; } pgprot_t;
>>> typedef struct { unsigned long pgste; } pgste_t;
>>> typedef struct { unsigned long pte; } pte_t;
>>> typedef struct { unsigned long pmd; } pmd_t;
>>> typedef struct { unsigned long pud; } pud_t;
>>> typedef struct { unsigned long pgd; } pgd_t;
>>> typedef pte_t *pgtable_t;
>>>
>>> #define pgprot_val(x)   ((x).pgprot)
>>> #define pgste_val(x)    ((x).pgste)
>>> #define pte_val(x)      ((x).pte)
>>> #define pmd_val(x)      ((x).pmd)
>>> #define pud_val(x)      ((x).pud)
>>> #define pgd_val(x)      ((x).pgd)
>>>
>>> #define __pgste(x)      ((pgste_t) { (x) } )
>>> #define __pte(x)        ((pte_t) { (x) } )
>>> #define __pmd(x)        ((pmd_t) { (x) } )
>>> #define __pud(x)        ((pud_t) { (x) } )
>>> #define __pgd(x)        ((pgd_t) { (x) } )
>>> #define __pgprot(x)     ((pgprot_t) { (x) } )
>>> """
>>>
>>> If you think, I could add a similar comment indicating that my
>>> struct is also just for type-checking.
>>
>> No, I think you've got the reason here slightly wrong. The kernel only
>> uses this since the pte_t and friends need to be urgently portable
>> between the different architectures. Have a look at the kernel
>> Documentation/CodingStyle file, it explicitly mentions this in chapter 5
>> ("Typedefs").
> 
> IMHO we don't talk about typedefs here but about type checking. Btw QEMU
> has the exact opposite policy regarding typedefs and structs than Linux.
> You want to say that the comment at the top of my quote is wrong, and
> that it should be rather "These are used for hiding representation.."
> that "These are used to make use of C type-checking.."?

I certainly did not want to say that you should change the comment. I
just wanted to point you to the fact that these typedefs in the kernel
are likely rather used for hiding representation indeed, and not so much
for type checking, so using them as an example for introducing something
like type checking here in QEMU is quite a bad example.

> I'm also curious which of the rules would your original suggestion of
> doing "typedef unsigned int IOInstEnding" match (from chapter 5
> "Typedefs")? ;)

None, of course. That's the difference between the kernel and QEMU -
though I have to say that I rather prefer the kernel philosophy nowadays.

So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
think the best match for QEMU would be a

typedef enum IOInstEnding {
    CC_...,
    CC_...,
    CC_...,
    CC_...
} IOInstEnding;

You then also should at least get some compiler checking thanks to
-Wswitch and -Wenum-compare, shouldn't you?

> I assume you have seen my reply to Connie about the benefit: among others
> it prevents using something like -EFAULT as a cc by accident which neither
> an enum nor a typedef does.

A confused developer could still do something like "return
(IOInstEnding){.cc = -EFAULT};", so this is also not completely safe.

 Thomas

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

[Halil Pasic]

On 10/12/2017 08:58 AM, Thomas Huth wrote:
> On 10.10.2017 13:41, Halil Pasic wrote:
>> [..]
>>>>
>>>> Yeah, the ABI is smart enough (where it matters) and this one is obviously
>>>> less that 8 bytes. I read this as you  assumed that the return won't be
>>>> passed via register (general purpose register 2 for a z host + ELF assumed),
>>>> and that would have been ugly indeed.
>>>>
>>>> Btw I have seen putting an integral into a struct for type checking
>>>> in the linux kernel too. For instance from:
>>>> arch/s390/include/asm/page.h
>>>>
>>>> """
>>>> /*
>>>>  * These are used to make use of C type-checking..
>>>>  */
>>>>
>>>> typedef struct { unsigned long pgprot; } pgprot_t;
>>>> typedef struct { unsigned long pgste; } pgste_t;
>>>> typedef struct { unsigned long pte; } pte_t;
>>>> typedef struct { unsigned long pmd; } pmd_t;
>>>> typedef struct { unsigned long pud; } pud_t;
>>>> typedef struct { unsigned long pgd; } pgd_t;
>>>> typedef pte_t *pgtable_t;
>>>>
>>>> #define pgprot_val(x)   ((x).pgprot)
>>>> #define pgste_val(x)    ((x).pgste)
>>>> #define pte_val(x)      ((x).pte)
>>>> #define pmd_val(x)      ((x).pmd)
>>>> #define pud_val(x)      ((x).pud)
>>>> #define pgd_val(x)      ((x).pgd)
>>>>
>>>> #define __pgste(x)      ((pgste_t) { (x) } )
>>>> #define __pte(x)        ((pte_t) { (x) } )
>>>> #define __pmd(x)        ((pmd_t) { (x) } )
>>>> #define __pud(x)        ((pud_t) { (x) } )
>>>> #define __pgd(x)        ((pgd_t) { (x) } )
>>>> #define __pgprot(x)     ((pgprot_t) { (x) } )
>>>> """
>>>>
>>>> If you think, I could add a similar comment indicating that my
>>>> struct is also just for type-checking.
>>>
>>> No, I think you've got the reason here slightly wrong. The kernel only
>>> uses this since the pte_t and friends need to be urgently portable
>>> between the different architectures. Have a look at the kernel
>>> Documentation/CodingStyle file, it explicitly mentions this in chapter 5
>>> ("Typedefs").
>>
>> IMHO we don't talk about typedefs here but about type checking. Btw QEMU
>> has the exact opposite policy regarding typedefs and structs than Linux.
>> You want to say that the comment at the top of my quote is wrong, and
>> that it should be rather "These are used for hiding representation.."
>> that "These are used to make use of C type-checking.."?
> 
> I certainly did not want to say that you should change the comment. I
> just wanted to point you to the fact that these typedefs in the kernel
> are likely rather used for hiding representation indeed, and not so much
> for type checking, so using them as an example for introducing something
> like type checking here in QEMU is quite a bad example.
> 

I've noted. I won't look for another example. I don't understand
your logic, but I'm afraid I've already taken too much of your
precious time. 

>> I'm also curious which of the rules would your original suggestion of
>> doing "typedef unsigned int IOInstEnding" match (from chapter 5
>> "Typedefs")? ;)
> 
> None, of course. That's the difference between the kernel and QEMU -
> though I have to say that I rather prefer the kernel philosophy nowadays.
> 
> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
> think the best match for QEMU would be a
> 
> typedef enum IOInstEnding {
>     CC_...,
>     CC_...,
>     CC_...,
>     CC_...
> } IOInstEnding;
> 

I also prefer this over #define NAME val.

> You then also should at least get some compiler checking thanks to
> -Wswitch and -Wenum-compare, shouldn't you?

None that matter. The switches are going away. And I also don't
expect compares. The for me interesting wrong propagation scenario
(e.g. return func_ret_errno()) isn't covered. It is not a big thing
though.

I've compiled with:
~/git/qemu/configure --target-list=s390x-softmmu --extra-cflags="-Wswitch -Wenum-compare"
and gcc version 4.8.5

> 
>> I assume you have seen my reply to Connie about the benefit: among others
>> it prevents using something like -EFAULT as a cc by accident which neither
>> an enum nor a typedef does.
> 
> A confused developer could still do something like "return
> (IOInstEnding){.cc = -EFAULT};", so this is also not completely safe.

Of course,but that would be a very confused programmer, who does not
bother what his newly written code does, and does not care to look at the
definition of  IOInstEnding which states the valid values of cc.
Since we don't write Ada I was under the impression that just stating the
valid values would be enough.

#define IOINST_CC(v) (assert(!((v) & ~0x03ULL)), (IOInstEnding){.cc = (v)})

or even better

#define IOINST_CC(cv) ({QEMU_BUILD_BUG_ON((cv) & ~0x03ULL);\                      
                        (IOInstEnding){.cc = (cv)};})

and then

return IOINST_CC(-EFAULT); 

would fail with an runtime assert or at compile time respectively while
good code would look like
return IOINST_CC(1);

Of course that would still eave the possibility of doing
"IOInstEnding){.cc = -EFAULT}" directly. Now this is where my
C skill ends (I don't know how to prohibit that, because we need
the type public).

In the end I found that such a mistake is unlikely enough, and
I'm still keeping my opinion.

I think I've understood your opinion: type checking is an overkill
in this particular use-case. It's a legit opinion -- one I don't
share, but one I can't argue with.

Again, sorry for taking so much of your time. I'm bad at letting
loose.

Regards,
Halil


> 
>  Thomas
> 

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

[Halil Pasic]

On 10/12/2017 01:44 PM, Halil Pasic wrote:
> 
> 
> On 10/12/2017 08:58 AM, Thomas Huth wrote:
>> On 10.10.2017 13:41, Halil Pasic wrote:
[..]
>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
>> think the best match for QEMU would be a
>>
>> typedef enum IOInstEnding {
>>     CC_...,
>>     CC_...,
>>     CC_...,
>>     CC_...
>> } IOInstEnding;
>>
> 
> I also prefer this over #define NAME val.
> 

@Conny @Thomas

I'm almost done with v3, but I've realized we did not agree on the
names for the enum constants, so before posting something to ugly
again, I would like to inquire your opinion.

My current version of the enum is the following (but I can easily change
to whatever you like with sed):

+/*
+ * 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 {
+    IOINST_CC_0 = 0, /* produced expected result */
+    IOINST_CC_1 = 1, /* status conditions were present, or alternate result */
+    IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */
+    IOINST_CC_3 = 3  /* ineffective, not operational */
+} IOInstEnding;
+

Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, 
IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical
code (e.g. just IONIST_CC_EXPECTED).

Regards,
Halil



[..]

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/8] s390x/css: IO instr handler ending control

[Thomas Huth]

On 17.10.2017 13:10, Halil Pasic wrote:
> 
> 
> On 10/12/2017 01:44 PM, Halil Pasic wrote:
>>
>>
>> On 10/12/2017 08:58 AM, Thomas Huth wrote:
>>> On 10.10.2017 13:41, Halil Pasic wrote:
> [..]
>>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
>>> think the best match for QEMU would be a
>>>
>>> typedef enum IOInstEnding {
>>>     CC_...,
>>>     CC_...,
>>>     CC_...,
>>>     CC_...
>>> } IOInstEnding;
>>>
>>
>> I also prefer this over #define NAME val.
>>
> 
> @Conny @Thomas
> 
> I'm almost done with v3, but I've realized we did not agree on the
> names for the enum constants, so before posting something to ugly
> again, I would like to inquire your opinion.
> 
> My current version of the enum is the following (but I can easily change
> to whatever you like with sed):
> 
> +/*
> + * 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 {
> +    IOINST_CC_0 = 0, /* produced expected result */
> +    IOINST_CC_1 = 1, /* status conditions were present, or alternate result */
> +    IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */
> +    IOINST_CC_3 = 3  /* ineffective, not operational */
> +} IOInstEnding;
> +
> 
> Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, 
> IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical
> code (e.g. just IONIST_CC_EXPECTED).

FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED).

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/8] s390x/css: IO instr handler ending control

[Cornelia Huck]

On Tue, 17 Oct 2017 13:28:57 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17.10.2017 13:10, Halil Pasic wrote:
> > 
> > 
> > On 10/12/2017 01:44 PM, Halil Pasic wrote:  
> >>
> >>
> >> On 10/12/2017 08:58 AM, Thomas Huth wrote:  
> >>> On 10.10.2017 13:41, Halil Pasic wrote:  
> > [..]  
> >>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
> >>> think the best match for QEMU would be a
> >>>
> >>> typedef enum IOInstEnding {
> >>>     CC_...,
> >>>     CC_...,
> >>>     CC_...,
> >>>     CC_...
> >>> } IOInstEnding;
> >>>  
> >>
> >> I also prefer this over #define NAME val.
> >>  
> > 
> > @Conny @Thomas
> > 
> > I'm almost done with v3, but I've realized we did not agree on the
> > names for the enum constants, so before posting something to ugly
> > again, I would like to inquire your opinion.
> > 
> > My current version of the enum is the following (but I can easily change
> > to whatever you like with sed):
> > 
> > +/*
> > + * 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 {
> > +    IOINST_CC_0 = 0, /* produced expected result */
> > +    IOINST_CC_1 = 1, /* status conditions were present, or alternate result */
> > +    IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */
> > +    IOINST_CC_3 = 3  /* ineffective, not operational */
> > +} IOInstEnding;
> > +
> > 
> > Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, 
> > IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical
> > code (e.g. just IONIST_CC_EXPECTED).  
> 
> FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED).

Either IOINST_CC_0 or IOINST_CC_EXPECTED, whatever you like best.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/8] s390x/css: IO instr handler ending control

[Halil Pasic]

On 10/17/2017 02:13 PM, Cornelia Huck wrote:
> On Tue, 17 Oct 2017 13:28:57 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17.10.2017 13:10, Halil Pasic wrote:
>>>
>>>
>>> On 10/12/2017 01:44 PM, Halil Pasic wrote:  
>>>>
>>>>
>>>> On 10/12/2017 08:58 AM, Thomas Huth wrote:  
>>>>> On 10.10.2017 13:41, Halil Pasic wrote:  
>>> [..]  
>>>>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I
>>>>> think the best match for QEMU would be a
>>>>>
>>>>> typedef enum IOInstEnding {
>>>>>     CC_...,
>>>>>     CC_...,
>>>>>     CC_...,
>>>>>     CC_...
>>>>> } IOInstEnding;
>>>>>  
>>>>
>>>> I also prefer this over #define NAME val.
>>>>  
>>>
>>> @Conny @Thomas
>>>
>>> I'm almost done with v3, but I've realized we did not agree on the
>>> names for the enum constants, so before posting something to ugly
>>> again, I would like to inquire your opinion.
>>>
>>> My current version of the enum is the following (but I can easily change
>>> to whatever you like with sed):
>>>
>>> +/*
>>> + * 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 {
>>> +    IOINST_CC_0 = 0, /* produced expected result */
>>> +    IOINST_CC_1 = 1, /* status conditions were present, or alternate result */
>>> +    IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */
>>> +    IOINST_CC_3 = 3  /* ineffective, not operational */
>>> +} IOInstEnding;
>>> +
>>>
>>> Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, 
>>> IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical
>>> code (e.g. just IONIST_CC_EXPECTED).  
>>
>> FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED).
> 
> Either IOINST_CC_0 or IOINST_CC_EXPECTED, whatever you like best.
> 

Thanks, I will go with the non-numerical variant as preferred by
Thomas (I've already changed my patches).

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

[Cornelia Huck]

On Mon, 9 Oct 2017 12:54:03 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 10/09/2017 10:20 AM, Thomas Huth wrote:
> > On 04.10.2017 17:41, Halil Pasic wrote:  

> >> +/* IO instructions conclude according this */
> >> +typedef struct IOInstEnding {
> >> +        /*
> >> +         * General semantic of cc codes of IO instructions is (brief):
> >> +         * 0 -- produced expected result
> >> +         * 1 --  status conditions were present or produced alternate result
> >> +         * 2 -- ineffective, because busy with previously initiated function
> >> +         * 3 -- ineffective, not operational
> >> +         */
> >> +        int cc;
> >> +} IOInstEnding;  
> > 
> > Why do you need a struct for this? Do you plan to extend it later? If
> > so, I think you should mention that in the patch description. If not,
> > please use a named enum or a "typedef unsigned int IOInstEnding" instead.
> > 
> >  Thomas  
> 
> We may, we may not. In the previous version we also had to support
> do end a certain instruction with an addressing exception, but this
> is going away in patch #3. Honestly I don't expect this being extended.
> 
> I have other reasons for the struct. Type safety and clear semantics,
> and frankly at least for s390 and linux I don't see any downsides given
> what is written in the "zSeries ELF Application Binary Interface Supplement".
> Can you please explain to me what is the problem with using this struct, and
> what is the benefit switching to a unsigned int?

Honestly, I fail to see the benefit of using a struct here... it's just
a condition code, and while adding a comment what the various codes
mean for I/O instructions is a good idea, I think having to use a
IOInstEnding struct just renders the code less readable.

[I haven't had time to look at the rest of the patches yet.]

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

[Halil Pasic]

On 10/09/2017 01:09 PM, Cornelia Huck wrote:
> On Mon, 9 Oct 2017 12:54:03 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 10/09/2017 10:20 AM, Thomas Huth wrote:
>>> On 04.10.2017 17:41, Halil Pasic wrote:  
> 
>>>> +/* IO instructions conclude according this */
>>>> +typedef struct IOInstEnding {
>>>> +        /*
>>>> +         * General semantic of cc codes of IO instructions is (brief):
>>>> +         * 0 -- produced expected result
>>>> +         * 1 --  status conditions were present or produced alternate result
>>>> +         * 2 -- ineffective, because busy with previously initiated function
>>>> +         * 3 -- ineffective, not operational
>>>> +         */
>>>> +        int cc;
>>>> +} IOInstEnding;  
>>>
>>> Why do you need a struct for this? Do you plan to extend it later? If
>>> so, I think you should mention that in the patch description. If not,
>>> please use a named enum or a "typedef unsigned int IOInstEnding" instead.
>>>
>>>  Thomas  
>>
>> We may, we may not. In the previous version we also had to support
>> do end a certain instruction with an addressing exception, but this
>> is going away in patch #3. Honestly I don't expect this being extended.
>>
>> I have other reasons for the struct. Type safety and clear semantics,
>> and frankly at least for s390 and linux I don't see any downsides given
>> what is written in the "zSeries ELF Application Binary Interface Supplement".
>> Can you please explain to me what is the problem with using this struct, and
>> what is the benefit switching to a unsigned int?
> 
> Honestly, I fail to see the benefit of using a struct here... it's just


Type safety. For instance had I forgotten let's say a return -ENODEV
somewhere, my version would not compile. On the contrary with an enum (like
Thomas has proposed) it compiles just fine with my setup -- I did not try
what would happen if we call setcc(cpu, -ENODEV)

> a condition code, and while adding a comment what the various codes

Right, but it's a different _type_ of condition code (than the POSIX errno
codes for instance) so that's why it's modeled as a different type.

> mean for I/O instructions is a good idea, I think having to use a
> IOInstEnding struct just renders the code less readable.

It's probably a matter of taste. I find, for example
return (IOInstEnding){.cc = 1}
in this case more readable than
return -ENOSYS
because from the first one I know we have detected a condition
which we want to handle by setting cc 1 for the instruction. The
later requires me figuring out in the context of which instruction
handler am I called, go trough the whole call chain looking for
possible re-mappings (like for -EACCES) and finally examining the
switch statement of the instruction handler carefully.

Could you please tell me what exactly is difficult to read for
you (form what I've proposed).

Regards,
Halil


> 
> [I haven't had time to look at the rest of the patches yet.]
> 

[Qemu-devel] [PATCH v2 3/8] 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.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---

AFAIR we decided in the previous round to rather do transformation
and fixing in one patch than touch stuff twice. Hence this patch
ain't pure transformation any more.
---
 hw/s390x/css.c              | 83 +++++++++++++--------------------------------
 hw/s390x/s390-ccw.c         | 11 +++---
 hw/vfio/ccw.c               | 30 ++++++++++++----
 include/hw/s390x/css.h      | 24 +++++++++----
 include/hw/s390x/s390-ccw.h |  2 +-
 target/s390x/ioinst.c       | 53 ++++-------------------------
 6 files changed, 77 insertions(+), 126 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 4f47dbc8b0..b2978c3bae 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1003,12 +1003,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)) {
@@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
      */
     if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
         !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
-        return -EINVAL;
+        sch_gen_unit_exception(sch);
+        css_inject_io_interrupt(sch);
+        return (IOInstEnding){.cc = 0};
     }
-
-    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);
 }
 
 /*
@@ -1055,7 +1034,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;
@@ -1069,12 +1048,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 (IOInstEnding){.cc = 0};
 }
 
-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) {
@@ -1084,16 +1063,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 (IOInstEnding){.cc = 0};
 }
 
-static int do_subchannel_work(SubchDev *sch)
+static IOInstEnding do_subchannel_work(SubchDev *sch)
 {
     if (!sch->do_subchannel_work) {
-        return -EINVAL;
+        return (IOInstEnding){.cc = 1};
     }
     g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
     return sch->do_subchannel_work(sch);
@@ -1383,27 +1361,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 (IOInstEnding){.cc = 3};
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return (IOInstEnding){.cc = 1};
     }
 
     if (s->ctrl & (SCSW_FCTL_START_FUNC |
                    SCSW_FCTL_HALT_FUNC |
                    SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return (IOInstEnding){.cc = 2};
     }
 
     /* If monitoring is active, update counter. */
@@ -1416,10 +1390,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,
@@ -1666,27 +1637,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 (IOInstEnding){.cc = 3};
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return (IOInstEnding){.cc = 1};
     }
 
     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 (IOInstEnding){.cc = 2};
     }
 
     /* If monitoring is active, update counter. */
@@ -1695,11 +1662,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..5d2c305b71 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 (IOInstEnding){.cc = 1};
     }
+    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 a8baadf57a..dbb5b201de 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -23,6 +23,7 @@
 #include "hw/s390x/s390-ccw.h"
 #include "hw/s390x/ccw-device.h"
 #include "qemu/error-report.h"
+#include "cpu.h"
 
 #define TYPE_VFIO_CCW "vfio-ccw"
 typedef struct VFIOCCWDevice {
@@ -47,9 +48,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 +61,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 +72,25 @@ 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) {
+    /* Currently we don't update control block and just return the cc code. */
+    case 0:
+        return (IOInstEnding){.cc = 0};
+    case EBUSY:
+        return (IOInstEnding){.cc = 2};
+    case ENODEV:
+    case EACCES:
+        return (IOInstEnding){.cc = 3};
+    case EFAULT:
+    default:
+        sch_gen_unit_exception(sch);
+        css_inject_io_interrupt(sch);
+        return (IOInstEnding){.cc = 0};
     }
-
-    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 66916b6546..2116c6b3c7 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -107,11 +107,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;
 
 /*
@@ -170,9 +181,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,
@@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
                          uint16_t schid);
 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);
 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);
@@ -214,7 +226,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..582287ff6c 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).cc);
 }
 
 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).cc);
 }
 
 #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)
-- 
2.13.5

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

[Dong Jia Shi]

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

[...]

> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 4f47dbc8b0..b2978c3bae 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1003,12 +1003,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)) {
> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>       */
>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -        return -EINVAL;
> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
Last cycle, we agreed to add some log here. Sth. like:
warn_report("vfio-ccw requires PFCH and C64 flags set...");

I promised to do a fix for this piece of code. But since this patch
already fixed it, I guess what I have to do is to add the log only? Or
you would like to add it by yourself? ;)

> +        return (IOInstEnding){.cc = 0};
>      }
> -
> -    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);
>  }
> 
>  /*
[...]

> @@ -1084,16 +1063,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 (IOInstEnding){.cc = 0};
>  }
> 
> -static int do_subchannel_work(SubchDev *sch)
> +static IOInstEnding do_subchannel_work(SubchDev *sch)
>  {
>      if (!sch->do_subchannel_work) {
> -        return -EINVAL;
> +        return (IOInstEnding){.cc = 1};
This keeps the logic here as-is, so it is right.

Anybody agrees that also adding an assert() here?

[...]

> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 8614dda6f8..5d2c305b71 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 (IOInstEnding){.cc = 1};
Same consideration as the last comment.

>      }
> +    return cdc->handle_request(sch);
>  }
> 
>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,

[...]

-- 
Dong Jia Shi

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

[Halil Pasic]

On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +0200]:
> 
> [...]
> 
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 4f47dbc8b0..b2978c3bae 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1003,12 +1003,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)) {
>> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>>       */
>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> -        return -EINVAL;
>> +        sch_gen_unit_exception(sch);
>> +        css_inject_io_interrupt(sch);
> Last cycle, we agreed to add some log here. Sth. like:
> warn_report("vfio-ccw requires PFCH and C64 flags set...");
> 
> I promised to do a fix for this piece of code. But since this patch
> already fixed it, I guess what I have to do is to add the log only? Or
> you would like to add it by yourself? ;)
> 

I think I forgot this one. Should there be a v3 I could add this too.
Otherwise I would not mind if you do it on top.

>> +        return (IOInstEnding){.cc = 0};
>>      }
>> -
>> -    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);
>>  }
>>
>>  /*
> [...]
> 
>> @@ -1084,16 +1063,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 (IOInstEnding){.cc = 0};
>>  }
>>
>> -static int do_subchannel_work(SubchDev *sch)
>> +static IOInstEnding do_subchannel_work(SubchDev *sch)
>>  {
>>      if (!sch->do_subchannel_work) {
>> -        return -EINVAL;
>> +        return (IOInstEnding){.cc = 1};
> This keeps the logic here as-is, so it is right.
> 

Yep.

> Anybody agrees that also adding an assert() here?

With automated regression testing in place I'm for it, without
I feel uncomfortable doing it myself. You could do this
on top if you like.

> 
> [...]
> 
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 8614dda6f8..5d2c305b71 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 (IOInstEnding){.cc = 1};
> Same consideration as the last comment.

Same here.

> 
>>      }
>> +    return cdc->handle_request(sch);
>>  }
>>
>>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> 
> [...]
> 

Except for the missing warning are you OK with the rest
of the patch? I would like to re-claim your r-b (dropped
because changes weren't just minor).

Halil

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

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-10 12:06:23 +0200]:

> 
> 
> On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +0200]:
> > 
> > [...]
> > 
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c3bae 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1003,12 +1003,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)) {
> >> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
> >>       */
> >>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> -        return -EINVAL;
> >> +        sch_gen_unit_exception(sch);
> >> +        css_inject_io_interrupt(sch);
> > Last cycle, we agreed to add some log here. Sth. like:
> > warn_report("vfio-ccw requires PFCH and C64 flags set...");
> > 
> > I promised to do a fix for this piece of code. But since this patch
> > already fixed it, I guess what I have to do is to add the log only? Or
> > you would like to add it by yourself? ;)
> > 
> 
> I think I forgot this one. Should there be a v3 I could add this too.
> Otherwise I would not mind if you do it on top.
> 
[...]

> >> @@ -1084,16 +1063,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 (IOInstEnding){.cc = 0};
> >>  }
> >>
> >> -static int do_subchannel_work(SubchDev *sch)
> >> +static IOInstEnding do_subchannel_work(SubchDev *sch)
> >>  {
> >>      if (!sch->do_subchannel_work) {
> >> -        return -EINVAL;
> >> +        return (IOInstEnding){.cc = 1};
> > This keeps the logic here as-is, so it is right.
> > 
> 
> Yep.
> 
> > Anybody agrees that also adding an assert() here?
> 
> With automated regression testing in place I'm for it, without
> I feel uncomfortable doing it myself. You could do this
> on top if you like.
Got it.

Marked. I will look back after this series.

[...]

> 
> Except for the missing warning are you OK with the rest
> of the patch? I would like to re-claim your r-b (dropped
> because changes weren't just minor).

I replied to the patch thread - the main part looks good to me.

I will save my r-b for the next round. ;)

-- 
Dong Jia Shi

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

[Cornelia Huck]

On Wed,  4 Oct 2017 17:41:39 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Simplify the error handling of the SSCH and RSCH handler avoiding
> arbitrary and cryptic error codes being used to tell how the instruction
> is supposed to end.  Let the code detecting the condition tell how it's
> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
> in one go as the emulation of the two shares a lot of code.
> 
> For passthrough this change isn't pure refactoring, but changes the
> way kernel reported EFAULT is handled. After clarifying the kernel
> interface we decided that EFAULT shall be mapped to unit exception.
> Same goes for unexpected error codes.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> AFAIR we decided in the previous round to rather do transformation
> and fixing in one patch than touch stuff twice. Hence this patch
> ain't pure transformation any more.
> ---
>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 30 ++++++++++++----
>  include/hw/s390x/css.h      | 24 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++-------------------------
>  6 files changed, 77 insertions(+), 126 deletions(-)

After browsing through this patch, I think the change will work just as
well if you use e.g. #defines instead of the structure, won't it?

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

[Halil Pasic]

On 10/10/2017 03:07 PM, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 17:41:39 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Simplify the error handling of the SSCH and RSCH handler avoiding
>> arbitrary and cryptic error codes being used to tell how the instruction
>> is supposed to end.  Let the code detecting the condition tell how it's
>> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
>> in one go as the emulation of the two shares a lot of code.
>>
>> For passthrough this change isn't pure refactoring, but changes the
>> way kernel reported EFAULT is handled. After clarifying the kernel
>> interface we decided that EFAULT shall be mapped to unit exception.
>> Same goes for unexpected error codes.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> AFAIR we decided in the previous round to rather do transformation
>> and fixing in one patch than touch stuff twice. Hence this patch
>> ain't pure transformation any more.
>> ---
>>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>>  hw/s390x/s390-ccw.c         | 11 +++---
>>  hw/vfio/ccw.c               | 30 ++++++++++++----
>>  include/hw/s390x/css.h      | 24 +++++++++----
>>  include/hw/s390x/s390-ccw.h |  2 +-
>>  target/s390x/ioinst.c       | 53 ++++-------------------------
>>  6 files changed, 77 insertions(+), 126 deletions(-)
> 
> After browsing through this patch, I think the change will work just as
> well if you use e.g. #defines instead of the structure, won't it?
> 

Sure. We just loose type safety. For example if someone ever should try to
propagate a normal errno via return do_something() the compiler won't
catch it we effectively use int for both.

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

[Halil Pasic]

On 10/10/2017 04:36 PM, Halil Pasic wrote:
> 
> 
> On 10/10/2017 03:07 PM, Cornelia Huck wrote:
>> On Wed,  4 Oct 2017 17:41:39 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> Simplify the error handling of the SSCH and RSCH handler avoiding
>>> arbitrary and cryptic error codes being used to tell how the instruction
>>> is supposed to end.  Let the code detecting the condition tell how it's
>>> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
>>> in one go as the emulation of the two shares a lot of code.
>>>
>>> For passthrough this change isn't pure refactoring, but changes the
>>> way kernel reported EFAULT is handled. After clarifying the kernel
>>> interface we decided that EFAULT shall be mapped to unit exception.
>>> Same goes for unexpected error codes.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>
>>> AFAIR we decided in the previous round to rather do transformation
>>> and fixing in one patch than touch stuff twice. Hence this patch
>>> ain't pure transformation any more.
>>> ---
>>>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>>>  hw/s390x/s390-ccw.c         | 11 +++---
>>>  hw/vfio/ccw.c               | 30 ++++++++++++----
>>>  include/hw/s390x/css.h      | 24 +++++++++----
>>>  include/hw/s390x/s390-ccw.h |  2 +-
>>>  target/s390x/ioinst.c       | 53 ++++-------------------------
>>>  6 files changed, 77 insertions(+), 126 deletions(-)
>>
>> After browsing through this patch, I think the change will work just as
>> well if you use e.g. #defines instead of the structure, won't it?
>>
> 
> Sure. We just loose type safety. For example if someone ever should try to
> propagate a normal errno via return do_something() the compiler won't
> catch it we effectively use int for both.
> 
>

@Connie: Discussion seems to have died down quite a bit, and besides
of the struct vs enum vs int and the point of Dong Jia I've seen no
complaints.

I would like to do a re-spin to accommodate the complaints, but before
I abandon the IOInstEnding struct I would like to confirm, that this
patch, and it's behavioral changes are OK. I would hate to do the dance
backwards, because in the next iteration it turns out that we want to
keep the "program_interrupt(env, PGM_ADDRESSING, 4);". Could you please
pass your judgment on this patch (assuming I fix the issues found by
Dong Jia)?

Regards,
Halil

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

[Cornelia Huck]

On Thu, 12 Oct 2017 14:06:42 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

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

I don't think I really have complaints. Currently quite busy with other
things, though.

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

[Halil Pasic]

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

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

Regards,
Halil

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

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +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.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> AFAIR we decided in the previous round to rather do transformation
> and fixing in one patch than touch stuff twice. Hence this patch
> ain't pure transformation any more.
> ---
>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 30 ++++++++++++----
>  include/hw/s390x/css.h      | 24 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++-------------------------
>  6 files changed, 77 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 4f47dbc8b0..b2978c3bae 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1003,12 +1003,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)) {
> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>       */
>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -        return -EINVAL;
> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return (IOInstEnding){.cc = 0};
This behavior change is not mentioned in the commit message. Right?

[...]

> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index a8baadf57a..dbb5b201de 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -23,6 +23,7 @@
>  #include "hw/s390x/s390-ccw.h"
>  #include "hw/s390x/ccw-device.h"
>  #include "qemu/error-report.h"
> +#include "cpu.h"
We need this because?

> 
>  #define TYPE_VFIO_CCW "vfio-ccw"
>  typedef struct VFIOCCWDevice {
> @@ -47,9 +48,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 +61,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 +72,25 @@ 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) {
> +    /* Currently we don't update control block and just return the cc code. */
This is not true anymore? At least for the EFAULT case.

> +    case 0:
> +        return (IOInstEnding){.cc = 0};
> +    case EBUSY:
> +        return (IOInstEnding){.cc = 2};
> +    case ENODEV:
> +    case EACCES:
> +        return (IOInstEnding){.cc = 3};
> +    case EFAULT:
> +    default:
> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return (IOInstEnding){.cc = 0};
>      }
> -
> -    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 66916b6546..2116c6b3c7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
[...]

> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
>                           uint16_t schid);
>  bool css_subch_visible(SubchDev *sch);
>  void css_conditional_io_interrupt(SubchDev *sch);
> +
Extra change.

>  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);
[...]

-- 
Dong Jia Shi

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

[Halil Pasic]

On 10/11/2017 05:47 AM, Dong Jia Shi wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +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.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> AFAIR we decided in the previous round to rather do transformation
>> and fixing in one patch than touch stuff twice. Hence this patch
>> ain't pure transformation any more.
>> ---
>>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>>  hw/s390x/s390-ccw.c         | 11 +++---
>>  hw/vfio/ccw.c               | 30 ++++++++++++----
>>  include/hw/s390x/css.h      | 24 +++++++++----
>>  include/hw/s390x/s390-ccw.h |  2 +-
>>  target/s390x/ioinst.c       | 53 ++++-------------------------
>>  6 files changed, 77 insertions(+), 126 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 4f47dbc8b0..b2978c3bae 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1003,12 +1003,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)) {
>> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>>       */
>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> -        return -EINVAL;
>> +        sch_gen_unit_exception(sch);
>> +        css_inject_io_interrupt(sch);
>> +        return (IOInstEnding){.cc = 0};
> This behavior change is not mentioned in the commit message. Right?
> 
No this particular change is not. Should I mention it explicitly?
Maybe "Same goes for unexpected error codes and absence of required
ORB flags."




> [...]
> 
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index a8baadf57a..dbb5b201de 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -23,6 +23,7 @@
>>  #include "hw/s390x/s390-ccw.h"
>>  #include "hw/s390x/ccw-device.h"
>>  #include "qemu/error-report.h"
>> +#include "cpu.h"
> We need this because?
> 

No idea -- was not intentional. Probably slipped in when rebasing,
or I don't know.

>>
>>  #define TYPE_VFIO_CCW "vfio-ccw"
>>  typedef struct VFIOCCWDevice {
>> @@ -47,9 +48,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 +61,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 +72,25 @@ 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) {
>> +    /* Currently we don't update control block and just return the cc code. */
> This is not true anymore? At least for the EFAULT case.
> 

Right I shall remove this.

>> +    case 0:
>> +        return (IOInstEnding){.cc = 0};
>> +    case EBUSY:
>> +        return (IOInstEnding){.cc = 2};
>> +    case ENODEV:
>> +    case EACCES:
>> +        return (IOInstEnding){.cc = 3};
>> +    case EFAULT:
>> +    default:
>> +        sch_gen_unit_exception(sch);
>> +        css_inject_io_interrupt(sch);
>> +        return (IOInstEnding){.cc = 0};
>>      }
>> -
>> -    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 66916b6546..2116c6b3c7 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
> [...]
> 
>> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
>>                           uint16_t schid);
>>  bool css_subch_visible(SubchDev *sch);
>>  void css_conditional_io_interrupt(SubchDev *sch);
>> +
> Extra change.
> 

Should be removed.

>>  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);
> [...]
> 

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

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-11 12:54:51 +0200]:

> 
> 
> On 10/11/2017 05:47 AM, Dong Jia Shi wrote:
> > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +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.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>
> >> AFAIR we decided in the previous round to rather do transformation
> >> and fixing in one patch than touch stuff twice. Hence this patch
> >> ain't pure transformation any more.
> >> ---
> >>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
> >>  hw/s390x/s390-ccw.c         | 11 +++---
> >>  hw/vfio/ccw.c               | 30 ++++++++++++----
> >>  include/hw/s390x/css.h      | 24 +++++++++----
> >>  include/hw/s390x/s390-ccw.h |  2 +-
> >>  target/s390x/ioinst.c       | 53 ++++-------------------------
> >>  6 files changed, 77 insertions(+), 126 deletions(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c3bae 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1003,12 +1003,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)) {
> >> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
> >>       */
> >>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> -        return -EINVAL;
> >> +        sch_gen_unit_exception(sch);
> >> +        css_inject_io_interrupt(sch);
> >> +        return (IOInstEnding){.cc = 0};
> > This behavior change is not mentioned in the commit message. Right?
> > 
> No this particular change is not. Should I mention it explicitly?
I think so.

> Maybe "Same goes for unexpected error codes and absence of required
> ORB flags."
Sounds good for me.

[...]

-- 
Dong Jia Shi

[Qemu-devel] [PATCH v2 4/8] 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 b2978c3bae..175624d1c8 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1223,20 +1223,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 (IOInstEnding){.cc = 3};
     }
 
     if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
-        ret = -EINPROGRESS;
-        goto out;
+        return (IOInstEnding){.cc = 1};
     }
 
     if (!(s->ctrl & SCSW_CTRL_MASK_FCTL) ||
@@ -1244,8 +1241,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 (IOInstEnding){.cc = 2};
     }
 
     /* Cancel the current operation. */
@@ -1257,10 +1253,7 @@ int css_do_xsch(SubchDev *sch)
     sch->last_cmd_valid = false;
     s->dstat = 0;
     s->cstat = 0;
-    ret = 0;
-
-out:
-    return ret;
+    return (IOInstEnding){.cc = 0};
 }
 
 int css_do_csch(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 2116c6b3c7..4c89d77893 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -212,7 +212,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 582287ff6c..85f5be6ca3 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).cc);
 }
 
 void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
-- 
2.13.5

[Qemu-devel] [PATCH v2 5/8] 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 175624d1c8..1839f40408 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1256,26 +1256,20 @@ IOInstEnding css_do_xsch(SubchDev *sch)
     return (IOInstEnding){.cc = 0};
 }
 
-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 (IOInstEnding){.cc = 3};
     }
 
     /* 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 4c89d77893..6ca9999077 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -213,7 +213,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 85f5be6ca3..b12b3d1d41 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).cc);
 }
 
 void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
-- 
2.13.5

[Qemu-devel] [PATCH v2 6/8] 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 1839f40408..6da3e4ea61 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1272,28 +1272,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 (IOInstEnding){.cc = 3};
     }
 
     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 (IOInstEnding){.cc = 1};
     }
 
     if (s->ctrl & (SCSW_FCTL_HALT_FUNC | SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return (IOInstEnding){.cc = 2};
     }
 
     /* Trigger the halt function. */
@@ -1306,11 +1302,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 6ca9999077..a4064711e2 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -214,7 +214,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 b12b3d1d41..12102bab0f 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).cc);
 }
 
 static int ioinst_schib_valid(SCHIB *schib)
-- 
2.13.5

[Qemu-devel] [PATCH v2 7/8] 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 6da3e4ea61..98d0a6123b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1168,28 +1168,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 (IOInstEnding){.cc = 0};
     }
 
     if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
-        ret = -EINPROGRESS;
-        goto out;
+        return (IOInstEnding){.cc = 1};
     }
 
     if (s->ctrl &
         (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) {
-        ret = -EBUSY;
-        goto out;
+        return (IOInstEnding){.cc = 1};
     }
 
     copy_schib_from_guest(&schib, orig_schib);
@@ -1216,11 +1212,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 (IOInstEnding){.cc = 0};
 }
 
 IOInstEnding css_do_xsch(SubchDev *sch)
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index a4064711e2..e3685e73d9 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -211,7 +211,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);
+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 12102bab0f..0c256baa70 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).cc);
 }
 
 static void copy_orb_from_guest(ORB *dest, const ORB *src)
-- 
2.13.5

[Qemu-devel] [PATCH v2 8/8] s390x: factor out common ioinst handler logic

[Halil Pasic]

Some of ioinst the handlers look very much the same: they basically
delegate the work to the appropriate css function (doing some always the
same stuff before and after the call to the appropriate css function).
Let us create a template and get rid of some code.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Suggested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
 1 file changed, 14 insertions(+), 45 deletions(-)

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index 0c256baa70..9c7d6a222c 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -38,7 +38,10 @@ int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
     return 0;
 }
 
-void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
+/* many ionst handlers look the same: they just delegate to a some css func */
+static void ioinst_handler_template_sch(S390CPU *cpu, uint64_t reg1,
+                                      const char *iname,
+                                      IOInstEnding (*handler_css)(SubchDev *))
 {
     int cssid, ssid, schid, m;
     SubchDev *sch;
@@ -47,49 +50,28 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
         program_interrupt(&cpu->env, PGM_OPERAND, 4);
         return;
     }
-    trace_ioinst_sch_id("xsch", cssid, ssid, schid);
+    trace_ioinst_sch_id(iname, cssid, ssid, schid);
     sch = css_find_subch(m, cssid, ssid, schid);
     if (!sch || !css_subch_visible(sch)) {
         setcc(cpu, 3);
         return;
     }
-    setcc(cpu, css_do_xsch(sch).cc);
+    setcc(cpu, handler_css(sch).cc);
 }
 
-void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
+void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
+    ioinst_handler_template_sch(cpu, reg1, "xsch", css_do_xsch);
+}
 
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("csch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_csch(sch).cc);
+void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
+{
+    ioinst_handler_template_sch(cpu, reg1, "csch", css_do_csch);
 }
 
 void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
-
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("hsch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_hsch(sch).cc);
+    ioinst_handler_template_sch(cpu, reg1, "hsch", css_do_hsch);
 }
 
 static int ioinst_schib_valid(SCHIB *schib)
@@ -707,20 +689,7 @@ void ioinst_handle_schm(S390CPU *cpu, uint64_t reg1, uint64_t reg2,
 
 void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1)
 {
-    int cssid, ssid, schid, m;
-    SubchDev *sch;
-
-    if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
-        program_interrupt(&cpu->env, PGM_OPERAND, 4);
-        return;
-    }
-    trace_ioinst_sch_id("rsch", cssid, ssid, schid);
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        setcc(cpu, 3);
-        return;
-    }
-    setcc(cpu, css_do_rsch(sch).cc);
+   ioinst_handler_template_sch(cpu, reg1, "rsch", css_do_rsch);
 }
 
 #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 8/8] s390x: factor out common ioinst handler logic

[Cornelia Huck]

On Wed,  4 Oct 2017 17:41:44 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Some of ioinst the handlers look very much the same: they basically
> delegate the work to the appropriate css function (doing some always the
> same stuff before and after the call to the appropriate css function).
> Let us create a template and get rid of some code.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Suggested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> ---
>  target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
>  1 file changed, 14 insertions(+), 45 deletions(-)

Staring at this patch, I'm not sure I like it, although I can't quite
put a finger on *what* I don't like... maybe the whole 'template'
approach.

Re: [Qemu-devel] [PATCH v2 8/8] s390x: factor out common ioinst handler logic

[Halil Pasic]

On 10/10/2017 03:10 PM, Cornelia Huck wrote:
> On Wed,  4 Oct 2017 17:41:44 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Some of ioinst the handlers look very much the same: they basically
>> delegate the work to the appropriate css function (doing some always the
>> same stuff before and after the call to the appropriate css function).
>> Let us create a template and get rid of some code.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Suggested-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>> ---
>>  target/s390x/ioinst.c | 59 ++++++++++++---------------------------------------
>>  1 file changed, 14 insertions(+), 45 deletions(-)
> 
> Staring at this patch, I'm not sure I like it, although I can't quite
> put a finger on *what* I don't like... maybe the whole 'template'
> approach.
> 

Well that's why I was careful to make it a separate patch.
I'm also a bit ambivalent, and did not want to include it in
this series along the lines can be done later if somebody wants,
but Marc convinced me -- kind of. We can just forget about it.

Halil