| Message ID | 20171004154144.88995-2-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve error handling for IO instr | expand |
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:37 +0200]: > Calling do_subchannel_work with no function control flags set in SCSW is > a programming error. Currently the handle this differently in ?: s/the/we/ > do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be > consistent and guard with a common assert against this programming error. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index c59be1aad1..4f47dbc8b0 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1067,9 +1067,6 @@ int do_subchannel_work_virtual(SubchDev *sch) > } else if (s->ctrl & SCSW_FCTL_START_FUNC) { > /* Triggered by both ssch and rsch. */ > sch_handle_start_func_virtual(sch); > - } else { > - /* Cannot happen. */ > - return 0; > } > css_inject_io_interrupt(sch); > return 0; > @@ -1077,22 +1074,17 @@ int do_subchannel_work_virtual(SubchDev *sch) > > int do_subchannel_work_passthrough(SubchDev *sch) > { > - int ret; > + int ret = 0; > SCSW *s = &sch->curr_status.scsw; > > if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) { > /* TODO: Clear handling */ > sch_handle_clear_func(sch); > - ret = 0; > } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) { > /* TODO: Halt handling */ > sch_handle_halt_func(sch); > - ret = 0; A bit surprise to see these. I'm fine with these changes though. > } else if (s->ctrl & SCSW_FCTL_START_FUNC) { > ret = sch_handle_start_func_passthrough(sch); > - } else { > - /* Cannot happen. */ > - return -ENODEV; > } > > return ret; > @@ -1100,11 +1092,11 @@ int do_subchannel_work_passthrough(SubchDev *sch) > > static int do_subchannel_work(SubchDev *sch) > { > - if (sch->do_subchannel_work) { > - return sch->do_subchannel_work(sch); > - } else { > + if (!sch->do_subchannel_work) { > return -EINVAL; > } > + g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL); > + return sch->do_subchannel_work(sch); > } > > static void copy_pmcw_to_guest(PMCW *dest, const PMCW *src) With the fix of the message: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
On Wed, 4 Oct 2017 17:41:37 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Calling do_subchannel_work with no function control flags set in SCSW is > a programming error. Currently the handle this differently in > do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be > consistent and guard with a common assert against this programming error. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) Thanks, applied.
On 10/10/2017 03:25 PM, Cornelia Huck wrote: > On Wed, 4 Oct 2017 17:41:37 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> Calling do_subchannel_work with no function control flags set in SCSW is >> a programming error. Currently the handle this differently in >> do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be >> consistent and guard with a common assert against this programming error. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/css.c | 16 ++++------------ >> 1 file changed, 4 insertions(+), 12 deletions(-) > > Thanks, applied. > Thank you! Halil
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index c59be1aad1..4f47dbc8b0 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1067,9 +1067,6 @@ int do_subchannel_work_virtual(SubchDev *sch) } else if (s->ctrl & SCSW_FCTL_START_FUNC) { /* Triggered by both ssch and rsch. */ sch_handle_start_func_virtual(sch); - } else { - /* Cannot happen. */ - return 0; } css_inject_io_interrupt(sch); return 0; @@ -1077,22 +1074,17 @@ int do_subchannel_work_virtual(SubchDev *sch) int do_subchannel_work_passthrough(SubchDev *sch) { - int ret; + int ret = 0; SCSW *s = &sch->curr_status.scsw; if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) { /* TODO: Clear handling */ sch_handle_clear_func(sch); - ret = 0; } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) { /* TODO: Halt handling */ sch_handle_halt_func(sch); - ret = 0; } else if (s->ctrl & SCSW_FCTL_START_FUNC) { ret = sch_handle_start_func_passthrough(sch); - } else { - /* Cannot happen. */ - return -ENODEV; } return ret; @@ -1100,11 +1092,11 @@ int do_subchannel_work_passthrough(SubchDev *sch) static int do_subchannel_work(SubchDev *sch) { - if (sch->do_subchannel_work) { - return sch->do_subchannel_work(sch); - } else { + if (!sch->do_subchannel_work) { return -EINVAL; } + g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL); + return sch->do_subchannel_work(sch); } static void copy_pmcw_to_guest(PMCW *dest, const PMCW *src)
Calling do_subchannel_work with no function control flags set in SCSW is a programming error. Currently the handle this differently in do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be consistent and guard with a common assert against this programming error. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)