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 <th...@redhat.com> 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 <pa...@linux.vnet.ibm.com>
[...]
>>>>> @@ -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
