On 10/11/2017 05:47 AM, Dong Jia Shi wrote:
> * Halil Pasic <pa...@linux.vnet.ibm.com> [2017-10-04 17:41:39 +0200]:
>
>> 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.
>>
>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
>> ---
>>
>> AFAIR we decided in the previous round to rather do transformation
>> and fixing in one patch than touch stuff twice. Hence this patch
>> ain't pure transformation any more.
>> ---
>> hw/s390x/css.c | 83
>> +++++++++++++--------------------------------
>> hw/s390x/s390-ccw.c | 11 +++---
>> hw/vfio/ccw.c | 30 ++++++++++++----
>> include/hw/s390x/css.h | 24 +++++++++----
>> include/hw/s390x/s390-ccw.h | 2 +-
>> target/s390x/ioinst.c | 53 ++++-------------------------
>> 6 files changed, 77 insertions(+), 126 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 4f47dbc8b0..b2978c3bae 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev
>> *sch)
>>
>> }
>>
>> -static int sch_handle_start_func_passthrough(SubchDev *sch)
>> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>> {
>>
>> PMCW *p = &sch->curr_status.pmcw;
>> SCSW *s = &sch->curr_status.scsw;
>> - int ret;
>>
>> ORB *orb = &sch->orb;
>> if (!(s->ctrl & SCSW_ACTL_SUSP)) {
>> @@ -1022,31 +1021,11 @@ static int
>> sch_handle_start_func_passthrough(SubchDev *sch)
>> */
>> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> - return -EINVAL;
>> + sch_gen_unit_exception(sch);
>> + css_inject_io_interrupt(sch);
>> + return (IOInstEnding){.cc = 0};
> This behavior change is not mentioned in the commit message. Right?
>
No this particular change is not. Should I mention it explicitly?
Maybe "Same goes for unexpected error codes and absence of required
ORB flags."
> [...]
>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index a8baadf57a..dbb5b201de 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -23,6 +23,7 @@
>> #include "hw/s390x/s390-ccw.h"
>> #include "hw/s390x/ccw-device.h"
>> #include "qemu/error-report.h"
>> +#include "cpu.h"
> We need this because?
>
No idea -- was not intentional. Probably slipped in when rebasing,
or I don't know.
>>
>> #define TYPE_VFIO_CCW "vfio-ccw"
>> typedef struct VFIOCCWDevice {
>> @@ -47,9 +48,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
>> .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
>> };
>>
>> -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
>> +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>> {
>> - S390CCWDevice *cdev = data;
>> + S390CCWDevice *cdev = sch->driver_data;
>> VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>> struct ccw_io_region *region = vcdev->io_region;
>> int ret;
>> @@ -60,8 +61,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw,
>> void *data)
>>
>> memset(region, 0, sizeof(*region));
>>
>> - memcpy(region->orb_area, orb, sizeof(ORB));
>> - memcpy(region->scsw_area, scsw, sizeof(SCSW));
>> + memcpy(region->orb_area, &sch->orb, sizeof(ORB));
>> + memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
>>
>> again:
>> ret = pwrite(vcdev->vdev.fd, region,
>> @@ -71,10 +72,25 @@ again:
>> goto again;
>> }
>> error_report("vfio-ccw: wirte I/O region failed with errno=%d",
>> errno);
>> - return -errno;
>> + ret = -errno;
>> + } else {
>> + ret = region->ret_code;
>> + }
>> + switch (-ret) {
>> + /* Currently we don't update control block and just return the cc code.
>> */
> This is not true anymore? At least for the EFAULT case.
>
Right I shall remove this.
>> + case 0:
>> + return (IOInstEnding){.cc = 0};
>> + case EBUSY:
>> + return (IOInstEnding){.cc = 2};
>> + case ENODEV:
>> + case EACCES:
>> + return (IOInstEnding){.cc = 3};
>> + case EFAULT:
>> + default:
>> + sch_gen_unit_exception(sch);
>> + css_inject_io_interrupt(sch);
>> + return (IOInstEnding){.cc = 0};
>> }
>> -
>> - return region->ret_code;
>> }
>>
>> static void vfio_ccw_reset(DeviceState *dev)
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index 66916b6546..2116c6b3c7 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
> [...]
>
>> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid,
>> uint8_t ssid,
>> uint16_t schid);
>> bool css_subch_visible(SubchDev *sch);
>> void css_conditional_io_interrupt(SubchDev *sch);
>> +
> Extra change.
>
Should be removed.
>> int css_do_stsch(SubchDev *sch, SCHIB *schib);
>> bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>> int css_do_msch(SubchDev *sch, const SCHIB *schib);
> [...]
>
