On 08/31/2017 09:50 AM, Thomas Huth wrote:
> On 30.08.2017 18:36, Halil Pasic wrote:
>> According to the POP a start subchannel instruction (SSCH) returning with
>> cc 1 implies that the subchannel was status pending when SSCH executed.
>>
>> Due to a somewhat confusing error handling, where error codes are mapped
>> to cc value, sane looking error codes result in non AR compliant
>> behavior.
>>
>> Let's fix this! Instead of cc 1 we use cc 3 which means device not
>> operational, and is much closer to the truth in the given cases.
>>
>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
>> Acked-by: Pierre Morel<pmo...@linux.vnet.ibm.com>
>> ---
>>
>> This patch turned out quite controversial. We did not reach a consensus
>> during the internal review.
>>
>> The most of the discussion revolved around the ORB flag which
>> architecturally must be supported, but are currently not supported by
>> vfio-ccw (not yet, or can't be). The idea showing the most promise for
>> consensus was to handle this via device status (along the lines better a
>> strange acting device than a non-conform machine) but since it's a
>> radical change we decided to first discuss upstream and then do whatever
>> needs to be done.
>> ---
>> hw/s390x/css.c | 15 ++++++---------
>> hw/s390x/s390-ccw.c | 2 +-
>> 2 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index a50fb0727e..0822538cde 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1034,7 +1034,7 @@ static int sch_handle_start_func_passthrough(SubchDev
>> *sch)
>> */
>> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> - return -EINVAL;
>> + return -ENODEV;
>
> I don't really like ENODEV in this case (since the device is apparently
> there)... but well, since you're later change it again to set cc=3
> directly, I guess the temporary ENODEV is ok.
>
>> }
>>
>> ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
>> @@ -1046,16 +1046,13 @@ static int
>> sch_handle_start_func_passthrough(SubchDev *sch)
>> break;
>> case -ENODEV:
>> break;
>> + case -EFAULT:
>> + break;
>
> I think you should mention this in the patch description. Why is EFAULT
> suddenly handled here?
It is not suddenly :) If you examine ioinst_handle_ssch which really
handles the error codes (here we are just mapping them around) you see:
switch (ret) {
case -ENODEV:
cc = 3;
break;
case -EBUSY:
cc = 2;
break;
case -EFAULT:
/*
* TODO:
* I'm wondering whether there is something better
* to do for us here (like setting some device or
* subchannel status).
*/
program_interrupt(env, PGM_ADDRESSING, 4);
return;
case 0:
cc = 0;
break;
default:
cc = 1;
break;
}
That is -EFAULT is handled with a program interrupt, and I want to keep
that. Hence break, that is keep unchanged.
What I do want to change is the other not explicitly handled error codes
(which actually should not happen) should be cc 3 and not cc 1.
So the default branch sets ret to -ENODEV.
>
>> case -EACCES:
>> /* Let's reflect an inaccessible host device by cc 3. */
>> - ret = -ENODEV;
>> - break;
>> default:
>> - /*
>> - * All other return codes will trigger a program check,
>> - * or set cc to 1.
>> - */
>> - break;
>> + /* Let's make all other return codes map to cc 3. */
>> + ret = -ENODEV;
>> };
>>
>> return ret;
>> @@ -1115,7 +1112,7 @@ static int do_subchannel_work(SubchDev *sch)
>> if (sch->do_subchannel_work) {
>> return sch->do_subchannel_work(sch);
>> } else {
>> - return -EINVAL;
>> + return -ENODEV;
>> }
>> }
>>
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 8614dda6f8..2b0741741c 100644
>> --- a/hw/s390x/s390-ccw.c
>> +++ b/hw/s390x/s390-ccw.c
>> @@ -25,7 +25,7 @@ int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
>> if (cdc->handle_request) {
>> return cdc->handle_request(orb, scsw, data);
>> } else {
>> - return -ENOSYS;
>> + return -ENODEV;
>> }
>> }
>
> Thomas
>
