[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity
From: |
Dong Jia Shi |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] s390x/css: check ccw address validity |
Date: |
Thu, 27 Jul 2017 09:03:46 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
* Halil Pasic <address@hidden> [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
[Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC, Halil Pasic, 2017/07/25
Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC, Cornelia Huck, 2017/07/27
Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC, Cornelia Huck, 2017/07/27