On 07/26/2017 02:05 PM, Halil Pasic wrote: > > > On 07/26/2017 05:31 AM, Dong Jia Shi wrote: >> * Halil Pasic <pa...@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 <pa...@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 >>> >> > >