Re: [Qemu-devel] [PATCH v2 1/8] s390x/css: be more consistent if broken beyond repair

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

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

-- 
Dong Jia Shi