| Message ID | 20170725224442.13383-3-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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> >
* 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> > > > >
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.
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.
* 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 ;)
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-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.
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?
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; }
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(-)