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