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

[Dong Jia Shi]

* Halil Pasic <address@hidden> [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 <address@hidden>
> ---
> 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
> 

-- 
Dong Jia Shi