* Halil Pasic <pa...@linux.vnet.ibm.com> [2017-10-10 12:06:23 +0200]:
>
>
> 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.
>
[...]
> >> @@ -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
