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

[Halil Pasic]

On 07/26/2017 02:05 PM, Halil Pasic wrote:
> 
> 
> 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.
> 

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. 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 :/

Regards,
Halil

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