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

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* 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