Re: [Qemu-devel] [PATCH v3 3/7] s390x: improve error handling for SSCH and RSCH

[Halil Pasic]

On 10/18/2017 01:12 PM, Thomas Huth wrote:
> On 18.10.2017 13:07, Halil Pasic wrote:
>>
>>
>> On 10/18/2017 12:07 PM, Thomas Huth wrote:
>>> On 18.10.2017 11:52, Cornelia Huck wrote:
>>>> On Wed, 18 Oct 2017 11:30:47 +0200
>>>> Thomas Huth <address@hidden> wrote:
>>>>
>>>>> On 17.10.2017 16:04, Halil Pasic wrote:
>>>>>> Simplify the error handling of the SSCH and RSCH handler avoiding
>>>>>> arbitrary and cryptic error codes being used to tell how the instruction
>>>>>> is supposed to end.  Let the code detecting the condition tell how it's
>>>>>> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
>>>>>> in one go as the emulation of the two shares a lot of code.
>>>>>>
>>>>>> For passthrough this change isn't pure refactoring, but changes the way
>>>>>> kernel reported EFAULT is handled. After clarifying the kernel interface
>>>>>> we decided that EFAULT shall be mapped to unit exception.  Same goes for
>>>>>> unexpected error codes and absence of required ORB flags.
>>>>>>
>>>>>> Signed-off-by: Halil Pasic <address@hidden>
> [...]
>>>>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int 
>>>>>> dct, uint64_t mbo)
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> -int css_do_rsch(SubchDev *sch)
>>>>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>>>>  {
>>>>>>      SCSW *s = &sch->curr_status.scsw;
>>>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>>>> -    int ret;
>>>>>>  
>>>>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>>>>> -        ret = -ENODEV;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>>>>      }
>>>>>>  
>>>>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>>>>> -        ret = -EINPROGRESS;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_STATUS_PRESENT;
>>>>>>      }
>>>>>>  
>>>>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>>>>> -        ret = -EINVAL;
>>>>>> -        goto out;
>>>>>> +        return IOINST_CC_BUSY;  
>>>>>
>>>>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>>>>> IOINST_CC_STATUS_PRESENT instead?
>>>>
>>>> No, that is correct (see the PoP for when cc 2 is supposed to be set by
>>>> rsch).
>>>
>>> So if this is on purpose, this change in behavior should also be
>>> mentioned in the patch description, I think.
>>
>> No. have a look at the function ioinst_handle_rsch. It used
>> to map -EINVAL to cc 2 (and was an oddball in this respect) so we
>> keep the old behavior, it's just more obvious whats happening.
> 
> Oh, you're right, I missed that different mapping of the error codes!
> ... so I see, the previous behavior was really confusing and error
> prone, it's really good that you're cleaning this up now!
> 
>  Thomas
> 

Many thanks for the watchful eyes! In the end I did manage to sneak
in a bug (for MSCH). I prefer a lots of false positives that than a
couple of false negatives (for review).

Regards,
Halil