ccw interpretation AR compliance improvements

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

Posted by Halil Pasic 8 years, 6 months ago

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

Posted by Dong Jia Shi 8 years, 6 months ago

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 2/2] s390x/css: fix bits must be zero check for TIC

Posted by Halil Pasic 8 years, 6 months ago


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 2/2] s390x/css: fix bits must be zero check for TIC

Posted by Dong Jia Shi 8 years, 6 months ago

* 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 2/2] s390x/css: fix bits must be zero check for TIC

Posted by Cornelia Huck 8 years, 6 months ago

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 2/2] s390x/css: fix bits must be zero check for TIC

Posted by Halil Pasic 8 years, 6 months ago


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

Posted by Halil Pasic 8 years, 6 months ago

[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

Posted by Cornelia Huck 8 years, 6 months ago

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 2/2] s390x/css: fix bits must be zero check for TIC

Posted by Cornelia Huck 8 years, 6 months ago

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

Posted by Dong Jia Shi 8 years, 6 months ago

* 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