Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH

[Dong Jia Shi]

* Halil Pasic <address@hidden> [2017-10-10 12:06:23 +0200]:

> 
> 
> On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> > * Halil Pasic <address@hidden> [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.
> 
[...]

> >> @@ -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.
Got it.

Marked. I will look back after this series.

[...]

> 
> 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).

I replied to the patch thread - the main part looks good to me.

I will save my r-b for the next round. ;)

-- 
Dong Jia Shi