* 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)
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 <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?
> + /* 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
