On Wed, 30 Aug 2017 18:36:04 +0200 Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
> Simplify the error handling of the SSCH and RSCH handler avoiding > arbitrary and cryptic error codes being mapped to what a subchannel is > supposed to do. Let the code detecting the condition tell how it's to be > handled in a less ambiguous way. It's best to handle SSCH and RSCH in > one go as the emulation of the two shares a lot of code. So determining the return code at the point in time where we can instead of trying to map to return codes and back again makes quite a bit of sense, but I'll have to look at the rest of this. For one thing, would a better split to introduce the cc-reporting infrastructure first and then convert the different I/O functions? > > Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> > Acked-by: Pierre Morel<pmo...@linux.vnet.ibm.com> > > --- > Notes: > Funny, we had a different swich-case for SSCH and RSCH. For > virtual it did not matter, but for passtrough it could. In practice > -EINVAL from the kernel would have been mapped to cc 2 in case of > RSCH and to cc 1 in case of SSHC which is absurd. Same goes for > -EBUSY from kernel which is correctly mapped to cc 2 in case of > SSCH, but for RSCH it gets mapped to cc 1 which is also absurd. > --- > hw/s390x/css.c | 86 > ++++++++++++++------------------------------- > hw/s390x/s390-ccw.c | 8 ++--- > hw/vfio/ccw.c | 32 +++++++++++++---- > include/hw/s390x/css.h | 30 ++++++++++++---- > include/hw/s390x/s390-ccw.h | 2 +- > target/s390x/ioinst.c | 61 +++++++++----------------------- > 6 files changed, 97 insertions(+), 122 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index bc47bf5b20..1102642c10 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1015,12 +1015,11 @@ static void sch_handle_start_func_virtual(SubchDev > *sch) > > } > > -static int sch_handle_start_func_passthrough(SubchDev *sch) > +static void 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)) { > @@ -1034,28 +1033,10 @@ static int sch_handle_start_func_passthrough(SubchDev > *sch) > */ > if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) || > !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) { > - return -ENODEV; > + sch->iret.cc = 3; Same as already commented: I don't think cc 3 is a good match. > } > > - 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 -EFAULT: > - break; > - case -EACCES: > - /* Let's reflect an inaccessible host device by cc 3. */ > - default: > - /* Let's make all other return codes map to cc 3. */ > - ret = -ENODEV; > - }; > - > - return ret; > + s390_ccw_cmd_request(sch); As you change the handling anyway: Don't change this logic in prior patches? > } > > /* > @@ -1064,7 +1045,7 @@ static int sch_handle_start_func_passthrough(SubchDev > *sch) > * read/writes) asynchronous later on if we start supporting more than > * our current very simple devices. > */ > -int do_subchannel_work_virtual(SubchDev *sch) > +void do_subchannel_work_virtual(SubchDev *sch) > { > > SCSW *s = &sch->curr_status.scsw; > @@ -1078,41 +1059,35 @@ int do_subchannel_work_virtual(SubchDev *sch) > sch_handle_start_func_virtual(sch); > } else { > /* Cannot happen. */ > - return -ENODEV; > + sch->iret.cc = 3; See comment for the last patch. > } > css_inject_io_interrupt(sch); > - return 0; > } > > -int do_subchannel_work_passthrough(SubchDev *sch) > +void do_subchannel_work_passthrough(SubchDev *sch) > { > - int ret; > 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); > + sch_handle_start_func_passthrough(sch); > } else { > /* Cannot happen. */ > - return -ENODEV; > + sch->iret.cc = 3; ftcl == 0 should have been rejected already by the function handlers here as well, shouldn't it? > } > - > - return ret; > } > > -static int do_subchannel_work(SubchDev *sch) > +static void do_subchannel_work(SubchDev *sch) > { > if (sch->do_subchannel_work) { > - return sch->do_subchannel_work(sch); > + sch->do_subchannel_work(sch); > } else { > - return -ENODEV; > + sch->iret.cc = 3; See my comment for a prior patch. > } > } > (...) > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 5c5fe6b202..d093181a9e 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -94,13 +94,31 @@ struct SubchDev { > /* transport-provided data: */ > int (*ccw_cb) (SubchDev *, CCW1); > void (*disable_cb)(SubchDev *); > - int (*do_subchannel_work) (SubchDev *); > + void (*do_subchannel_work) (SubchDev *); > SenseId id; > void *driver_data; > + /* io instructions conclude according to iret */ > + struct { > + /* > + * General semantic of cc codes of IO instructions is (brief): > + * 0 -- produced expected result > + * 1 -- produced alternate result > + * 2 -- ineffective, because busy with previously initiated function > + * 3 -- ineffective, not operational I'm not sure you can summarize this that way in all cases. Also, what does "ineffective" mean? If I get a cc 1 for, say, ssch, I don't expect something either as the subchannel was already status pending. > + */ > + uint32_t cc:4; > + bool pgm_chk:1; This looks weird? > + uint32_t irq_code; > + } iret; > }; > > extern const VMStateDescription vmstate_subch_dev; (...) > @@ -238,33 +236,17 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, > uint32_t ipb) > } > trace_ioinst_sch_id("ssch", cssid, ssid, schid); > sch = css_find_subch(m, cssid, ssid, schid); > - if (sch && css_subch_visible(sch)) { > - ret = css_do_ssch(sch, &orb); > + if (!sch || !css_subch_visible(sch)) { > + setcc(cpu, 3); > + return; > } > - switch (ret) { > - case -ENODEV: > - cc = 3; > - break; > - case -EBUSY: > - cc = 2; > - break; > - case -EFAULT: > - /* > - * TODO: > - * I'm wondering whether there is something better > - * to do for us here (like setting some device or > - * subchannel status). > - */ You removed the TODO :( There still might be a better way to reflect this... > - program_interrupt(env, PGM_ADDRESSING, 4); > + css_subch_clear_iret(sch); > + css_do_ssch(sch, &orb); > + if (sch->iret.pgm_chk) { > + program_interrupt(env, sch->iret.irq_code, 4); > return; > - case 0: > - cc = 0; > - break; > - default: > - cc = 1; > - break; > } > - setcc(cpu, cc); > + setcc(cpu, sch->iret.cc); > } > > void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb)