[Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Halil Pasic]

There were some missing checks. There might be some more missing.  For details,
see the individual patches.

Regarding testing: did not do much more than a simple smoke test
with virtio-ccw.

Halil Pasic (2):
  s390x/css: check ccw address validity
  s390x/css: fix bits must be zero check for TIC

 hw/s390x/css.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

-- 
2.11.2

[Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Halil Pasic]

According to the PoP channel command words (CCW) must be doubleword
aligned and 31 bit addressable for format 1 and 24 bit addressable for
format 0 CCWs.

If the channel subsystem encounters ccw address which does not satisfy
this alignment requirement a program-check condition is recognised.

The situation with 31 bit addressable is a bit more complicated: both the
ORB and a format 1 CCW TIC hold the address of the (rest of) the channel
program, that is the address of the next CCW in a word, and the PoP
mandates that bit 0 of that word shall be zero -- or a program-check
condition is to be recognized -- and does not belong to the field holding
the ccw address.

Since in code the corresponding fields span across the whole word (unlike
in PoP where these are defined as 31 bit wide) we can check this by
applying a mask. The 24 addressable case isn't affecting TIC because the
address is composed of a halfword and a byte portion (no additional zero
bit requirements) and just slightly complicates the ORB case where also
bits 1-7 need to be zero.

Let's make our CSS implementation follow the AR more closely.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
Note: Checking for 31 bit addressable ain't strictly necessary:
According to the AR the all zero fields of the ORB may or may not be
checked during the execution of SSCH. We do check the corresponding
single bit field of the ORB and respond to it accordingly. Using
the same mask for TIC and for ORB does not hurt.
---
 hw/s390x/css.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..d17e21b7af 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -24,6 +24,9 @@
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 
+/* CCWs are doubleword aligned and addressable by 31 bit */
+#define CCW1_ADDR_MASK 0x80000007
+
 typedef struct CrwContainer {
     CRW crw;
     QTAILQ_ENTRY(CrwContainer) sibling;
@@ -885,6 +888,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
             ret = -EINVAL;
             break;
         }
+        if (ccw.cda & CCW1_ADDR_MASK) {
+            ret = -EINVAL;
+            break;
+        }
         sch->channel_prog = ccw.cda;
         ret = -EAGAIN;
         break;
@@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
         suspend_allowed = true;
     }
     sch->last_cmd_valid = false;
+    if (sch->channel_prog & (CCW1_ADDR_MASK |
+                             sch->ccw_fmt_1 ? 0 : 0xff000000)) {
+            /* generate channel program check */
+            s->ctrl &= ~SCSW_ACTL_START_PEND;
+            s->cstat = SCSW_CSTAT_PROG_CHECK;
+            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
+            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
+            s->cpa = sch->channel_prog + 8;
+            return;
+    }
     do {
         ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
         switch (ret) {
-- 
2.11.2

[Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Halil Pasic]

According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
contain zeros.  Bits 0-3 are already covered by cmd_code validity
checking, and bit 32 is covered by the CCW address checking.

Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
check for the absence of certain flags.  Let's fix this.

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

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index d17e21b7af..1f04ce4a1b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
             ret = -EINVAL;
             break;
         }
-        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
+        if (ccw.flags || ccw.count) {
+            /* We have already sanitized these if fmt 0. */
             ret = -EINVAL;
             break;
         }
-- 
2.11.2

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Dong Jia Shi]

Hello Halil,

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:42 +0200]:

> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> checking, and bit 32 is covered by the CCW address checking.
> 
> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> check for the absence of certain flags.  Let's fix this.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index d17e21b7af..1f04ce4a1b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              ret = -EINVAL;
>              break;
>          }
> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> +        if (ccw.flags || ccw.count) {
> +            /* We have already sanitized these if fmt 0. */
ccw0 does not have the same restrictions as ccw1. We don't sanitize
these for ccw0.

(This comment is still here. Did I misunderstand things? :)

>              ret = -EINVAL;
>              break;
>          }
> -- 
> 2.11.2
> 

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

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:41 +0200]:

> According to the PoP channel command words (CCW) must be doubleword
> aligned and 31 bit addressable for format 1 and 24 bit addressable for
> format 0 CCWs.
> 
> If the channel subsystem encounters ccw address which does not satisfy
> this alignment requirement a program-check condition is recognised.
> 
> The situation with 31 bit addressable is a bit more complicated: both the
> ORB and a format 1 CCW TIC hold the address of the (rest of) the channel
                                              ^^^^^^^^^^^^^^^^^^^^
Meant to be (?):
of the (rest of the)

Maybe just saying:
the address of a channel probram

> program, that is the address of the next CCW in a word, and the PoP
> mandates that bit 0 of that word shall be zero -- or a program-check
> condition is to be recognized -- and does not belong to the field holding
> the ccw address.
> 
> Since in code the corresponding fields span across the whole word (unlike
> in PoP where these are defined as 31 bit wide) we can check this by
> applying a mask. The 24 addressable case isn't affecting TIC because the
> address is composed of a halfword and a byte portion (no additional zero
> bit requirements) and just slightly complicates the ORB case where also
> bits 1-7 need to be zero.
> 
> Let's make our CSS implementation follow the AR more closely.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> Note: Checking for 31 bit addressable ain't strictly necessary:
> According to the AR the all zero fields of the ORB may or may not be
> checked during the execution of SSCH. We do check the corresponding
> single bit field of the ORB and respond to it accordingly. Using
> the same mask for TIC and for ORB does not hurt.
> ---
>  hw/s390x/css.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6a42b95cee..d17e21b7af 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -24,6 +24,9 @@
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/s390-virtio-ccw.h"
> 
> +/* CCWs are doubleword aligned and addressable by 31 bit */
> +#define CCW1_ADDR_MASK 0x80000007
> +
Move this hunk to ioinst.h?

>  typedef struct CrwContainer {
>      CRW crw;
>      QTAILQ_ENTRY(CrwContainer) sibling;
> @@ -885,6 +888,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              ret = -EINVAL;
>              break;
>          }
> +        if (ccw.cda & CCW1_ADDR_MASK) {
> +            ret = -EINVAL;
> +            break;
> +        }
>          sch->channel_prog = ccw.cda;
>          ret = -EAGAIN;
>          break;
> @@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>          suspend_allowed = true;
>      }
>      sch->last_cmd_valid = false;
> +    if (sch->channel_prog & (CCW1_ADDR_MASK |
> +                             sch->ccw_fmt_1 ? 0 : 0xff000000)) {
I have problem in recognizing the operator precedence here:
    (CCW1_ADDR_MASK | sch->ccw_fmt_1 ? 0 : 0xff000000)

Bitwise '|' has higher precedence than '?:', so the above equals to:
    (CCW1_ADDR_MASK | sch->ccw_fmt_1) ? 0 : 0xff000000

So you will always get a '0', no?

> +            /* generate channel program check */
> +            s->ctrl &= ~SCSW_ACTL_START_PEND;
> +            s->cstat = SCSW_CSTAT_PROG_CHECK;
> +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> +            s->cpa = sch->channel_prog + 8;
> +            return;
> +    }
I think you could let css_interpret_ccw() do the sanity check on its
@ccw_addr parameter when (sch->last_cmd_valid == false), to reuse the
code of generating channel program check.

>      do {
>          ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
>          switch (ret) {
> -- 
> 2.11.2
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Halil Pasic]

On 07/26/2017 05:01 AM, Dong Jia Shi wrote:
> Hello Halil,
> 
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:42 +0200]:
> 
>> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
>> contain zeros.  Bits 0-3 are already covered by cmd_code validity
>> checking, and bit 32 is covered by the CCW address checking.
>>
>> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
>> check for the absence of certain flags.  Let's fix this.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index d17e21b7af..1f04ce4a1b 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
>> +        if (ccw.flags || ccw.count) {
>> +            /* We have already sanitized these if fmt 0. */
> ccw0 does not have the same restrictions as ccw1. We don't sanitize
> these for ccw0.
> 

Yes you have misunderstood. For fmt 1 these bits have to be zero
otherwise a program-check condition is to be recognized. Thus we don't
sanitize for fmt 1.

For fmt 0 these bits are ignored. We sanitize them in
for some time now by setting them to zero when making a CCW1
out of an CCW0. If we would recognize a program-check for
fmt 0 that would be wrong.

The comment tells us why this code is good for CCW0 too,
and why can we omit sch->ccw_fmt_1 from the conditon.

Regards,
Halil

> (This comment is still here. Did I misunderstand things? :)
> 
>>              ret = -EINVAL;
>>              break;
>>          }
>> -- 
>> 2.11.2
>>
> 
> With the comment removed:
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> 

Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Halil Pasic]

On 07/26/2017 05:31 AM, Dong Jia Shi wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:41 +0200]:
> 
>> According to the PoP channel command words (CCW) must be doubleword
>> aligned and 31 bit addressable for format 1 and 24 bit addressable for
>> format 0 CCWs.
>>
>> If the channel subsystem encounters ccw address which does not satisfy
>> this alignment requirement a program-check condition is recognised.
>>
>> The situation with 31 bit addressable is a bit more complicated: both the
>> ORB and a format 1 CCW TIC hold the address of the (rest of) the channel
>                                               ^^^^^^^^^^^^^^^^^^^^
> Meant to be (?):
> of the (rest of the)

Yes.

> 
> Maybe just saying:
> the address of a channel probram
> 

"the rest of the channel program" was supposed to refer to the TIC scenario,
and "the channel program" to ORB.

>> program, that is the address of the next CCW in a word, and the PoP
>> mandates that bit 0 of that word shall be zero -- or a program-check
>> condition is to be recognized -- and does not belong to the field holding
>> the ccw address.
>>
>> Since in code the corresponding fields span across the whole word (unlike
>> in PoP where these are defined as 31 bit wide) we can check this by
>> applying a mask. The 24 addressable case isn't affecting TIC because the
>> address is composed of a halfword and a byte portion (no additional zero
>> bit requirements) and just slightly complicates the ORB case where also
>> bits 1-7 need to be zero.
>>
>> Let's make our CSS implementation follow the AR more closely.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> Note: Checking for 31 bit addressable ain't strictly necessary:
>> According to the AR the all zero fields of the ORB may or may not be
>> checked during the execution of SSCH. We do check the corresponding
>> single bit field of the ORB and respond to it accordingly. Using
>> the same mask for TIC and for ORB does not hurt.
>> ---
>>  hw/s390x/css.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 6a42b95cee..d17e21b7af 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -24,6 +24,9 @@
>>  #include "hw/s390x/s390_flic.h"
>>  #include "hw/s390x/s390-virtio-ccw.h"
>>
>> +/* CCWs are doubleword aligned and addressable by 31 bit */
>> +#define CCW1_ADDR_MASK 0x80000007
>> +
> Move this hunk to ioinst.h?
> 
>>  typedef struct CrwContainer {
>>      CRW crw;
>>      QTAILQ_ENTRY(CrwContainer) sibling;
>> @@ -885,6 +888,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>              ret = -EINVAL;
>>              break;
>>          }
>> +        if (ccw.cda & CCW1_ADDR_MASK) {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>>          sch->channel_prog = ccw.cda;
>>          ret = -EAGAIN;
>>          break;
>> @@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>          suspend_allowed = true;
>>      }
>>      sch->last_cmd_valid = false;
>> +    if (sch->channel_prog & (CCW1_ADDR_MASK |
>> +                             sch->ccw_fmt_1 ? 0 : 0xff000000)) {
> I have problem in recognizing the operator precedence here:
>     (CCW1_ADDR_MASK | sch->ccw_fmt_1 ? 0 : 0xff000000)
> 
> Bitwise '|' has higher precedence than '?:', so the above equals to:
>     (CCW1_ADDR_MASK | sch->ccw_fmt_1) ? 0 : 0xff000000
> 
> So you will always get a '0', no?
> 

I'm afraid you are right. Good catch! This was supposed to be
(CCW1_ADDR_MASK | (sch->ccw_fmt_1 ? 0 : 0xff000000))


>> +            /* generate channel program check */
>> +            s->ctrl &= ~SCSW_ACTL_START_PEND;
>> +            s->cstat = SCSW_CSTAT_PROG_CHECK;
>> +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
>> +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
>> +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>> +            s->cpa = sch->channel_prog + 8;
>> +            return;
>> +    }
> I think you could let css_interpret_ccw() do the sanity check on its
> @ccw_addr parameter when (sch->last_cmd_valid == false), to reuse the
> code of generating channel program check.
>

I'm working on factoring out the code manipulating SCSW (among others). If we
do that this will look nicer. What you propose is certainly viable, althoug
maybe little less straight forward.

Yet another option would be to use a label and jump into the loop (AFAIR that
would be also valid).

Let us see what is Connie's opinion.

Regards,
Halil 
 
>>      do {
>>          ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
>>          switch (ret) {
>> -- 
>> 2.11.2
>>
> 

Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Halil Pasic]

On 07/26/2017 02:05 PM, Halil Pasic wrote:
> 
> 
> On 07/26/2017 05:31 AM, Dong Jia Shi wrote:
>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:41 +0200]:
>>
>>> According to the PoP channel command words (CCW) must be doubleword
>>> aligned and 31 bit addressable for format 1 and 24 bit addressable for
>>> format 0 CCWs.
>>>
>>> If the channel subsystem encounters ccw address which does not satisfy
>>> this alignment requirement a program-check condition is recognised.
>>>
>>> The situation with 31 bit addressable is a bit more complicated: both the
>>> ORB and a format 1 CCW TIC hold the address of the (rest of) the channel
>>                                               ^^^^^^^^^^^^^^^^^^^^
>> Meant to be (?):
>> of the (rest of the)
> 
> Yes.
> 
>>
>> Maybe just saying:
>> the address of a channel probram
>>
> 
> "the rest of the channel program" was supposed to refer to the TIC scenario,
> and "the channel program" to ORB.
> 
>>> program, that is the address of the next CCW in a word, and the PoP
>>> mandates that bit 0 of that word shall be zero -- or a program-check
>>> condition is to be recognized -- and does not belong to the field holding
>>> the ccw address.
>>>
>>> Since in code the corresponding fields span across the whole word (unlike
>>> in PoP where these are defined as 31 bit wide) we can check this by
>>> applying a mask. The 24 addressable case isn't affecting TIC because the
>>> address is composed of a halfword and a byte portion (no additional zero
>>> bit requirements) and just slightly complicates the ORB case where also
>>> bits 1-7 need to be zero.
>>>
>>> Let's make our CSS implementation follow the AR more closely.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>> Note: Checking for 31 bit addressable ain't strictly necessary:
>>> According to the AR the all zero fields of the ORB may or may not be
>>> checked during the execution of SSCH. We do check the corresponding
>>> single bit field of the ORB and respond to it accordingly. Using
>>> the same mask for TIC and for ORB does not hurt.
>>> ---
>>>  hw/s390x/css.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index 6a42b95cee..d17e21b7af 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -24,6 +24,9 @@
>>>  #include "hw/s390x/s390_flic.h"
>>>  #include "hw/s390x/s390-virtio-ccw.h"
>>>
>>> +/* CCWs are doubleword aligned and addressable by 31 bit */
>>> +#define CCW1_ADDR_MASK 0x80000007
>>> +
>> Move this hunk to ioinst.h?
>>
>>>  typedef struct CrwContainer {
>>>      CRW crw;
>>>      QTAILQ_ENTRY(CrwContainer) sibling;
>>> @@ -885,6 +888,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>>              ret = -EINVAL;
>>>              break;
>>>          }
>>> +        if (ccw.cda & CCW1_ADDR_MASK) {
>>> +            ret = -EINVAL;
>>> +            break;
>>> +        }
>>>          sch->channel_prog = ccw.cda;
>>>          ret = -EAGAIN;
>>>          break;
>>> @@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
>>>          suspend_allowed = true;
>>>      }
>>>      sch->last_cmd_valid = false;
>>> +    if (sch->channel_prog & (CCW1_ADDR_MASK |
>>> +                             sch->ccw_fmt_1 ? 0 : 0xff000000)) {
>> I have problem in recognizing the operator precedence here:
>>     (CCW1_ADDR_MASK | sch->ccw_fmt_1 ? 0 : 0xff000000)
>>
>> Bitwise '|' has higher precedence than '?:', so the above equals to:
>>     (CCW1_ADDR_MASK | sch->ccw_fmt_1) ? 0 : 0xff000000
>>
>> So you will always get a '0', no?
>>
> 
> I'm afraid you are right. Good catch! This was supposed to be
> (CCW1_ADDR_MASK | (sch->ccw_fmt_1 ? 0 : 0xff000000))
> 
> 
>>> +            /* generate channel program check */
>>> +            s->ctrl &= ~SCSW_ACTL_START_PEND;
>>> +            s->cstat = SCSW_CSTAT_PROG_CHECK;
>>> +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
>>> +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
>>> +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
>>> +            s->cpa = sch->channel_prog + 8;
>>> +            return;
>>> +    }
>> I think you could let css_interpret_ccw() do the sanity check on its
>> @ccw_addr parameter when (sch->last_cmd_valid == false), to reuse the
>> code of generating channel program check.
>>
> 
> I'm working on factoring out the code manipulating SCSW (among others). If we
> do that this will look nicer. What you propose is certainly viable, althoug
> maybe little less straight forward.
> 
> Yet another option would be to use a label and jump into the loop (AFAIR that
> would be also valid).
> 
> Let us see what is Connie's opinion.
> 

After re-examining the PoP I'm inclined to say we have to check this on every
iteration because of how "main-storage location is unavailable" is defined in
this context: the definition depends on the ccw format. There is nothing about
this in the ccw chaining section of the pop but it can be found in the
I/O interrupts chapter.

I think I will have to redo this patch :/

Regards,
Halil

> 
>>>      do {
>>>          ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
>>>          switch (ret) {
>>> -- 
>>> 2.11.2
>>>
>>
> 
> 

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 13:38:33 +0200]:

> 
> 
> On 07/26/2017 05:01 AM, Dong Jia Shi wrote:
> > Hello Halil,
> > 
> > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 00:44:42 +0200]:
> > 
> >> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> >> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> >> checking, and bit 32 is covered by the CCW address checking.
> >>
> >> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> >> check for the absence of certain flags.  Let's fix this.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/css.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index d17e21b7af..1f04ce4a1b 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >>              ret = -EINVAL;
> >>              break;
> >>          }
> >> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> >> +        if (ccw.flags || ccw.count) {
> >> +            /* We have already sanitized these if fmt 0. */
> > ccw0 does not have the same restrictions as ccw1. We don't sanitize
> > these for ccw0.
> > 
> 
> Yes you have misunderstood. For fmt 1 these bits have to be zero
> otherwise a program-check condition is to be recognized. Thus we don't
> sanitize for fmt 1.
> 
> For fmt 0 these bits are ignored. We sanitize them in
> for some time now by setting them to zero when making a CCW1
> out of an CCW0. If we would recognize a program-check for
> fmt 0 that would be wrong.
Yup, I know this.

> 
> The comment tells us why this code is good for CCW0 too,
> and why can we omit sch->ccw_fmt_1 from the conditon.
Ahh, I see the point now. Yes, I misunderstood.

Another point is we have translated ccw0 to ccw1. So here we only focus
on handling ccw1 stuff. Mentioning ccw0 seems a little redundant.

Anyway, I will leave this to you to decide. No problem from my side now.

> 
> Regards,
> Halil
> 
> > (This comment is still here. Did I misunderstand things? :)
> > 
> >>              ret = -EINVAL;
> >>              break;
> >>          }
> >> -- 
> >> 2.11.2
> >>
> > 
> > With the comment removed:
> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > 
> 
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-26 18:45:34 +0200]:

[...]

> >>> @@ -946,6 +953,17 @@ static void sch_handle_start_func_virtual(SubchDev *sch)
> >>>          suspend_allowed = true;
> >>>      }
> >>>      sch->last_cmd_valid = false;
> >>> +    if (sch->channel_prog & (CCW1_ADDR_MASK |
> >>> +                             sch->ccw_fmt_1 ? 0 : 0xff000000)) {
> >> I have problem in recognizing the operator precedence here:
> >>     (CCW1_ADDR_MASK | sch->ccw_fmt_1 ? 0 : 0xff000000)
> >>
> >> Bitwise '|' has higher precedence than '?:', so the above equals to:
> >>     (CCW1_ADDR_MASK | sch->ccw_fmt_1) ? 0 : 0xff000000
> >>
> >> So you will always get a '0', no?
> >>
> > 
> > I'm afraid you are right. Good catch! This was supposed to be
> > (CCW1_ADDR_MASK | (sch->ccw_fmt_1 ? 0 : 0xff000000))
> > 
> > 
> >>> +            /* generate channel program check */
> >>> +            s->ctrl &= ~SCSW_ACTL_START_PEND;
> >>> +            s->cstat = SCSW_CSTAT_PROG_CHECK;
> >>> +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> >>> +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> >>> +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> >>> +            s->cpa = sch->channel_prog + 8;
> >>> +            return;
> >>> +    }
> >> I think you could let css_interpret_ccw() do the sanity check on its
> >> @ccw_addr parameter when (sch->last_cmd_valid == false), to reuse the
> >> code of generating channel program check.
> >>
> > 
> > I'm working on factoring out the code manipulating SCSW (among others). If we
> > do that this will look nicer. What you propose is certainly viable, althoug
> > maybe little less straight forward.
> > 
> > Yet another option would be to use a label and jump into the loop (AFAIR that
> > would be also valid).
> > 
> > Let us see what is Connie's opinion.
> > 
> 
> After re-examining the PoP I'm inclined to say we have to check this on every
> iteration because of how "main-storage location is unavailable" is defined in
> this context: the definition depends on the ccw format.
Sounds natural!

> There is nothing about this in the ccw chaining section of the pop but
> it can be found in the I/O interrupts chapter.
> 
> I think I will have to redo this patch :/
Ok.

> 
> Regards,
> Halil
> 
> > 
> >>>      do {
> >>>          ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
> >>>          switch (ret) {
> >>> -- 
> >>> 2.11.2
> >>>
> >>
> > 
> > 
> 
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Cornelia Huck]

On Wed, 26 Jul 2017 00:44:42 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> checking, and bit 32 is covered by the CCW address checking.
> 
> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> check for the absence of certain flags.  Let's fix this.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index d17e21b7af..1f04ce4a1b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              ret = -EINVAL;
>              break;
>          }
> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> +        if (ccw.flags || ccw.count) {
> +            /* We have already sanitized these if fmt 0. */

I'd tweak that to

/* We have already sanitized these if converted from fmt 0. */

Seems less confusing.

>              ret = -EINVAL;
>              break;
>          }

I'm inclined to pick this as a 2.10 bugfix. Patch 1 still needs work
from what I've seen.

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Cornelia Huck]

On Wed, 26 Jul 2017 00:44:40 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> There were some missing checks. There might be some more missing.  For details,
> see the individual patches.

The checks are not perfect? Colour me shocked ;)

> 
> Regarding testing: did not do much more than a simple smoke test
> with virtio-ccw.

I think we need a way to throw random channel programs at a channel
device...

> 
> Halil Pasic (2):
>   s390x/css: check ccw address validity
>   s390x/css: fix bits must be zero check for TIC
> 
>  hw/s390x/css.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Cornelia Huck]

On Wed, 26 Jul 2017 00:44:42 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> checking, and bit 32 is covered by the CCW address checking.
> 
> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> check for the absence of certain flags.  Let's fix this.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index d17e21b7af..1f04ce4a1b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>              ret = -EINVAL;
>              break;
>          }
> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> +        if (ccw.flags || ccw.count) {
> +            /* We have already sanitized these if fmt 0. */
>              ret = -EINVAL;
>              break;
>          }

Thanks, applied (with tweaked comment).

Dong Jia: I've added your R-b, please let me know if that's not ok.

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-27 10:32:14 +0200]:

> On Wed, 26 Jul 2017 00:44:42 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> > contain zeros.  Bits 0-3 are already covered by cmd_code validity
> > checking, and bit 32 is covered by the CCW address checking.
> > 
> > Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> > check for the absence of certain flags.  Let's fix this.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/css.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index d17e21b7af..1f04ce4a1b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >              ret = -EINVAL;
> >              break;
> >          }
> > -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> > +        if (ccw.flags || ccw.count) {
> > +            /* We have already sanitized these if fmt 0. */
> >              ret = -EINVAL;
> >              break;
> >          }
> 
> Thanks, applied (with tweaked comment).
> 
> Dong Jia: I've added your R-b, please let me know if that's not ok.
Yes, please. That's ok.

(Just cann't help to miss the chance to reply to you ;)

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Halil Pasic]

On 07/27/2017 10:01 AM, Cornelia Huck wrote:
> On Wed, 26 Jul 2017 00:44:42 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
>> contain zeros.  Bits 0-3 are already covered by cmd_code validity
>> checking, and bit 32 is covered by the CCW address checking.
>>
>> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
>> check for the absence of certain flags.  Let's fix this.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index d17e21b7af..1f04ce4a1b 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
>> +        if (ccw.flags || ccw.count) {
>> +            /* We have already sanitized these if fmt 0. */
> 
> I'd tweak that to
> 
> /* We have already sanitized these if converted from fmt 0. */
> 

Fine with me.

> Seems less confusing.
> 
>>              ret = -EINVAL;
>>              break;
>>          }
> 
> I'm inclined to pick this as a 2.10 bugfix. Patch 1 still needs work
> from what I've seen.
> 

Hm. The commit message becomes inaccurate if this goes in before
patch 1. We still have must be zero bits which should be handled
by the address (ccw.cda) checking. I think I can fix patch 1 today.

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Halil Pasic]

On 07/27/2017 10:26 AM, Cornelia Huck wrote:
> On Wed, 26 Jul 2017 00:44:40 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> There were some missing checks. There might be some more missing.  For details,
>> see the individual patches.
> 
> The checks are not perfect? Colour me shocked ;)
> 

With a fedora or without ;)?


>>
>> Regarding testing: did not do much more than a simple smoke test
>> with virtio-ccw.
> 
> I think we need a way to throw random channel programs at a channel
> device...
> 

By random do you mean random random, or do you mean carefully crafted
to provoke (and verify) certain responses. If it's more like the second
case I've actually wrote something (a kernel driver) for 'internal use'
but at the moment it's limited to indirect data access support (no test
cases for invalid invalid channel programs). The 'internal guys' say it
probably ain't interesting for the rest of the world make this external,
but if you are interested I could send you the patch these days.

Regards,
Halil

>>
>> Halil Pasic (2):
>>   s390x/css: check ccw address validity
>>   s390x/css: fix bits must be zero check for TIC
>>
>>  hw/s390x/css.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
> 
> 

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Halil Pasic]

[Re-posting my previous reply because I've accidentally
dropped almost all addresses.]

On 07/27/2017 10:26 AM, Cornelia Huck wrote:
> On Wed, 26 Jul 2017 00:44:40 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> There were some missing checks. There might be some more missing.  For details,
>> see the individual patches.
> 
> The checks are not perfect? Colour me shocked ;)
> 

With a fedora or without ;)?

>>
>> Regarding testing: did not do much more than a simple smoke test
>> with virtio-ccw.
> 
> I think we need a way to throw random channel programs at a channel
> device...
> 

By random do you mean random random, or do you mean carefully crafted
to provoke (and verify) certain responses. If it's more like the second
case I've actually wrote something (a kernel driver) for 'internal use'
but at the moment it's limited to indirect data access support (no test
cases for invalid invalid channel programs). The 'internal guys' say it
probably ain't interesting for the rest of the world make this external,
but if you are interested I could send you the patch these days.

Regards,
Halil

>>
>> Halil Pasic (2):
>>   s390x/css: check ccw address validity
>>   s390x/css: fix bits must be zero check for TIC
>>
>>  hw/s390x/css.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
> 
> 

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Cornelia Huck]

On Thu, 27 Jul 2017 15:18:01 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> [Re-posting my previous reply because I've accidentally
> dropped almost all addresses.]
> 
> On 07/27/2017 10:26 AM, Cornelia Huck wrote:
> > On Wed, 26 Jul 2017 00:44:40 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> Regarding testing: did not do much more than a simple smoke test
> >> with virtio-ccw.  
> > 
> > I think we need a way to throw random channel programs at a channel
> > device...
> >   
> 
> By random do you mean random random, or do you mean carefully crafted
> to provoke (and verify) certain responses. If it's more like the second
> case I've actually wrote something (a kernel driver) for 'internal use'
> but at the moment it's limited to indirect data access support (no test
> cases for invalid invalid channel programs).

That's what I've been thinking of. Standalone guest code would be even
better (can be integrated into automatic testing), but it's certainly
useful.

> The 'internal guys' say it
> probably ain't interesting for the rest of the world make this external,
> but if you are interested I could send you the patch these days.

Would be great if you could make this available. It is especially
interesting for me, but possibly for other folks working on s390 as well.

Might be good together with an "eat this" channel device in qemu.

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Halil Pasic]

[Re-posting my previous reply because I've accidentally
dropped almost all addressees.]

On 07/27/2017 10:01 AM, Cornelia Huck wrote:
> On Wed, 26 Jul 2017 00:44:42 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
>> contain zeros.  Bits 0-3 are already covered by cmd_code validity
>> checking, and bit 32 is covered by the CCW address checking.
>>
>> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
>> check for the absence of certain flags.  Let's fix this.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/css.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index d17e21b7af..1f04ce4a1b 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
>> +        if (ccw.flags || ccw.count) {
>> +            /* We have already sanitized these if fmt 0. */
> 
> I'd tweak that to
> 
> /* We have already sanitized these if converted from fmt 0. */
> 

Fine with me.

> Seems less confusing.
> 
>>              ret = -EINVAL;
>>              break;
>>          }
> 
> I'm inclined to pick this as a 2.10 bugfix. Patch 1 still needs work
> from what I've seen.
> 

Hm. The commit message becomes inaccurate if this goes in before
patch 1. We still have must be zero bits which should be handled
by the address (ccw.cda) checking. I think I can fix patch 1 today.

Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC

[Cornelia Huck]

On Thu, 27 Jul 2017 15:40:33 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> [Re-posting my previous reply because I've accidentally
> dropped almost all addressees.]
> 
> On 07/27/2017 10:01 AM, Cornelia Huck wrote:
> > On Wed, 26 Jul 2017 00:44:42 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> >> contain zeros.  Bits 0-3 are already covered by cmd_code validity
> >> checking, and bit 32 is covered by the CCW address checking.
> >>
> >> Bits 8-31 correspond to CCW1.flags and CCW1.count.  Currently we only
> >> check for the absence of certain flags.  Let's fix this.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/css.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index d17e21b7af..1f04ce4a1b 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
> >>              ret = -EINVAL;
> >>              break;
> >>          }
> >> -        if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> >> +        if (ccw.flags || ccw.count) {
> >> +            /* We have already sanitized these if fmt 0. */  
> > 
> > I'd tweak that to
> > 
> > /* We have already sanitized these if converted from fmt 0. */
> >   
> 
> Fine with me.
> 
> > Seems less confusing.
> >   
> >>              ret = -EINVAL;
> >>              break;
> >>          }  
> > 
> > I'm inclined to pick this as a 2.10 bugfix. Patch 1 still needs work
> > from what I've seen.
> >   
> 
> Hm. The commit message becomes inaccurate if this goes in before
> patch 1. We still have must be zero bits which should be handled
> by the address (ccw.cda) checking. I think I can fix patch 1 today.
> 

It's probably a bit much for now.

Can you rather suggest a better commit message?

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Halil Pasic]

On 07/27/2017 03:40 PM, Cornelia Huck wrote:
> On Thu, 27 Jul 2017 15:18:01 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> [Re-posting my previous reply because I've accidentally
>> dropped almost all addresses.]
>>
>> On 07/27/2017 10:26 AM, Cornelia Huck wrote:
>>> On Wed, 26 Jul 2017 00:44:40 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> Regarding testing: did not do much more than a simple smoke test
>>>> with virtio-ccw.  
>>>
>>> I think we need a way to throw random channel programs at a channel
>>> device...
>>>   
>>
>> By random do you mean random random, or do you mean carefully crafted
>> to provoke (and verify) certain responses. If it's more like the second
>> case I've actually wrote something (a kernel driver) for 'internal use'
>> but at the moment it's limited to indirect data access support (no test
>> cases for invalid invalid channel programs).
> 
> That's what I've been thinking of. Standalone guest code would be even
> better (can be integrated into automatic testing), but it's certainly
> useful.
> 
>> The 'internal guys' say it
>> probably ain't interesting for the rest of the world make this external,
>> but if you are interested I could send you the patch these days.
> 
> Would be great if you could make this available. It is especially
> interesting for me, but possibly for other folks working on s390 as well.

It is very wip and I don't feel comfortable with sharing it
with the world yet. But I'm definitely interested in making this
available. I haven't figured out what would be the best way to make
it available, and I hope you can help me with that.

> 
> Might be good together with an "eat this" channel device in qemu.
> 

If course I do have a qemu counterpart.

To sum it up, one I'm done with the next iteration I'm gonna send
you with the patches unless you say otherwise.

Regards,
Halil

Re: [Qemu-devel] [PATCH 0/2] ccw interpretation AR compliance improvements

[Cornelia Huck]

On Thu, 27 Jul 2017 15:52:23 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 07/27/2017 03:40 PM, Cornelia Huck wrote:
> > On Thu, 27 Jul 2017 15:18:01 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> [Re-posting my previous reply because I've accidentally
> >> dropped almost all addresses.]
> >>
> >> On 07/27/2017 10:26 AM, Cornelia Huck wrote:  
> >>> On Wed, 26 Jul 2017 00:44:40 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  
> >   
> >>>> Regarding testing: did not do much more than a simple smoke test
> >>>> with virtio-ccw.    
> >>>
> >>> I think we need a way to throw random channel programs at a channel
> >>> device...
> >>>     
> >>
> >> By random do you mean random random, or do you mean carefully crafted
> >> to provoke (and verify) certain responses. If it's more like the second
> >> case I've actually wrote something (a kernel driver) for 'internal use'
> >> but at the moment it's limited to indirect data access support (no test
> >> cases for invalid invalid channel programs).  
> > 
> > That's what I've been thinking of. Standalone guest code would be even
> > better (can be integrated into automatic testing), but it's certainly
> > useful.
> >   
> >> The 'internal guys' say it
> >> probably ain't interesting for the rest of the world make this external,
> >> but if you are interested I could send you the patch these days.  
> > 
> > Would be great if you could make this available. It is especially
> > interesting for me, but possibly for other folks working on s390 as well.  
> 
> It is very wip and I don't feel comfortable with sharing it
> with the world yet. But I'm definitely interested in making this
> available. I haven't figured out what would be the best way to make
> it available, and I hope you can help me with that.

Do you have a git tree somewhere you can dump it?

> 
> > 
> > Might be good together with an "eat this" channel device in qemu.
> >   
> 
> If course I do have a qemu counterpart.

And of course I'd love to see that as well :)

> 
> To sum it up, one I'm done with the next iteration I'm gonna send
> you with the patches unless you say otherwise.
> 
> Regards,
> Halil
>