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