On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> * Halil Pasic <pa...@linux.vnet.ibm.com> [2017-10-04 17:41:39 +0200]:
>
> [...]
>
>> 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);
> Last cycle, we agreed to add some log here. Sth. like:
> warn_report("vfio-ccw requires PFCH and C64 flags set...");
>
> I promised to do a fix for this piece of code. But since this patch
> already fixed it, I guess what I have to do is to add the log only? Or
> you would like to add it by yourself? ;)
>
I think I forgot this one. Should there be a v3 I could add this too.
Otherwise I would not mind if you do it on top.
>> + return (IOInstEnding){.cc = 0};
>> }
>> -
>> - ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
>> - switch (ret) {
>> - /* Currently we don't update control block and just return the cc code.
>> */
>> - case 0:
>> - break;
>> - case -EBUSY:
>> - break;
>> - case -ENODEV:
>> - break;
>> - 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;
>> - };
>> -
>> - return ret;
>> + return s390_ccw_cmd_request(sch);
>> }
>>
>> /*
> [...]
>
>> @@ -1084,16 +1063,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
>> /* TODO: Halt handling */
>> sch_handle_halt_func(sch);
>> } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
>> - ret = sch_handle_start_func_passthrough(sch);
>> + return sch_handle_start_func_passthrough(sch);
>> }
>> -
>> - return ret;
>> + return (IOInstEnding){.cc = 0};
>> }
>>
>> -static int do_subchannel_work(SubchDev *sch)
>> +static IOInstEnding do_subchannel_work(SubchDev *sch)
>> {
>> if (!sch->do_subchannel_work) {
>> - return -EINVAL;
>> + return (IOInstEnding){.cc = 1};
> This keeps the logic here as-is, so it is right.
>
Yep.
> Anybody agrees that also adding an assert() here?
With automated regression testing in place I'm for it, without
I feel uncomfortable doing it myself. You could do this
on top if you like.
>
> [...]
>
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 8614dda6f8..5d2c305b71 100644
>> --- a/hw/s390x/s390-ccw.c
>> +++ b/hw/s390x/s390-ccw.c
>> @@ -18,15 +18,14 @@
>> #include "hw/s390x/css-bridge.h"
>> #include "hw/s390x/s390-ccw.h"
>>
>> -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
>> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
>> {
>> - S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
>> + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>>
>> - if (cdc->handle_request) {
>> - return cdc->handle_request(orb, scsw, data);
>> - } else {
>> - return -ENOSYS;
>> + if (!cdc->handle_request) {
>> + return (IOInstEnding){.cc = 1};
> Same consideration as the last comment.
Same here.
>
>> }
>> + return cdc->handle_request(sch);
>> }
>>
>> static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>
> [...]
>
Except for the missing warning are you OK with the rest
of the patch? I would like to re-claim your r-b (dropped
because changes weren't just minor).
Halil
