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

[Halil Pasic]

On 07/26/2017 05:31 AM, Dong Jia Shi wrote:
> * 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)

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 <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?
> 

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.

Regards,
Halil 
 
>>      do {
>>          ret = css_interpret_ccw(sch, sch->channel_prog, suspend_allowed);
>>          switch (ret) {
>> -- 
>> 2.11.2
>>
>