| Message ID | 20171017140453.51099-8-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve error handling for IO instr | expand |
On Tue, 17 Oct 2017 16:04:53 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Simplify the error handling of the MSCH. Let the code detecting the > condition tell (in a less ambiguous way) how it's to be handled. No > changes in behavior. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 18 +++++------------- > include/hw/s390x/css.h | 2 +- > target/s390x/ioinst.c | 23 ++++------------------- > 3 files changed, 10 insertions(+), 33 deletions(-) Looks sane (quoting the whole mail for the benefit of the qemu-s390x list). > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index b9e0329825..30fc236946 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > } > } > > -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > { > SCSW *s = &sch->curr_status.scsw; > PMCW *p = &sch->curr_status.pmcw; > uint16_t oldflags; > - int ret; > SCHIB schib; > > if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { > - ret = 0; > - goto out; > + return IOINST_CC_EXPECTED; > } > > if (s->ctrl & SCSW_STCTL_STATUS_PEND) { > - ret = -EINPROGRESS; > - goto out; > + return IOINST_CC_STATUS_PRESENT; > } > > if (s->ctrl & > (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { > - ret = -EBUSY; > - goto out; > + return IOINST_CC_STATUS_PRESENT; > } > > copy_schib_from_guest(&schib, orig_schib); > @@ -1395,11 +1391,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) { > sch->disable_cb(sch); > } > - > - ret = 0; > - > -out: > - return ret; > + return IOINST_CC_EXPECTED; > } > > IOInstEnding css_do_xsch(SubchDev *sch) > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 0c1efbae39..a74f6cc274 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -239,7 +239,7 @@ bool css_subch_visible(SubchDev *sch); > void css_conditional_io_interrupt(SubchDev *sch); > int css_do_stsch(SubchDev *sch, SCHIB *schib); > bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid); > -int css_do_msch(SubchDev *sch, const SCHIB *schib); > +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib); > IOInstEnding css_do_xsch(SubchDev *sch); > IOInstEnding css_do_csch(SubchDev *sch); > IOInstEnding css_do_hsch(SubchDev *sch); > diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c > index 8e4dacafd8..23962fbebc 100644 > --- a/target/s390x/ioinst.c > +++ b/target/s390x/ioinst.c > @@ -111,8 +111,6 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) > SubchDev *sch; > SCHIB schib; > uint64_t addr; > - int ret = -ENODEV; > - int cc; > CPUS390XState *env = &cpu->env; > uint8_t ar; > > @@ -131,24 +129,11 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) > } > trace_ioinst_sch_id("msch", cssid, ssid, schid); > sch = css_find_subch(m, cssid, ssid, schid); > - if (sch && css_subch_visible(sch)) { > - ret = css_do_msch(sch, &schib); > - } > - switch (ret) { > - case -ENODEV: > - cc = 3; > - break; > - case -EBUSY: > - cc = 2; > - break; > - case 0: > - cc = 0; > - break; > - default: > - cc = 1; > - break; > + if (!sch || !css_subch_visible(sch)) { > + setcc(cpu, 3); > + return; > } > - setcc(cpu, cc); > + setcc(cpu, css_do_msch(sch, &schib)); > } > > static void copy_orb_from_guest(ORB *dest, const ORB *src)
On 17.10.2017 16:04, Halil Pasic wrote: > Simplify the error handling of the MSCH. Let the code detecting the > condition tell (in a less ambiguous way) how it's to be handled. No > changes in behavior. ok, so you claim no changes in behavior ... > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 18 +++++------------- > include/hw/s390x/css.h | 2 +- > target/s390x/ioinst.c | 23 ++++------------------- > 3 files changed, 10 insertions(+), 33 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index b9e0329825..30fc236946 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > } > } > > -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > { > SCSW *s = &sch->curr_status.scsw; > PMCW *p = &sch->curr_status.pmcw; > uint16_t oldflags; > - int ret; > SCHIB schib; > > if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { > - ret = 0; > - goto out; > + return IOINST_CC_EXPECTED; > } > > if (s->ctrl & SCSW_STCTL_STATUS_PEND) { > - ret = -EINPROGRESS; > - goto out; > + return IOINST_CC_STATUS_PRESENT; > } > > if (s->ctrl & > (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { > - ret = -EBUSY; > - goto out; > + return IOINST_CC_STATUS_PRESENT; > } ... but here you change -EBUSY (which got mapped to CC=2) to CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e. this is either a bug, or you should update the patch description with a justification for this change in behavior. Thomas
On Wed, 18 Oct 2017 12:00:05 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 17.10.2017 16:04, Halil Pasic wrote: > > Simplify the error handling of the MSCH. Let the code detecting the > > condition tell (in a less ambiguous way) how it's to be handled. No > > changes in behavior. > > ok, so you claim no changes in behavior ... > > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > --- > > hw/s390x/css.c | 18 +++++------------- > > include/hw/s390x/css.h | 2 +- > > target/s390x/ioinst.c | 23 ++++------------------- > > 3 files changed, 10 insertions(+), 33 deletions(-) > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index b9e0329825..30fc236946 100644 > > --- a/hw/s390x/css.c > > +++ b/hw/s390x/css.c > > @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > > } > > } > > > > -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > > +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > > { > > SCSW *s = &sch->curr_status.scsw; > > PMCW *p = &sch->curr_status.pmcw; > > uint16_t oldflags; > > - int ret; > > SCHIB schib; > > > > if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { > > - ret = 0; > > - goto out; > > + return IOINST_CC_EXPECTED; > > } > > > > if (s->ctrl & SCSW_STCTL_STATUS_PEND) { > > - ret = -EINPROGRESS; > > - goto out; > > + return IOINST_CC_STATUS_PRESENT; > > } > > > > if (s->ctrl & > > (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { > > - ret = -EBUSY; > > - goto out; > > + return IOINST_CC_STATUS_PRESENT; > > } > > ... but here you change -EBUSY (which got mapped to CC=2) to > CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e. > this is either a bug, or you should update the patch description with a > justification for this change in behavior. Indeed, that's a bug. We still want cc 2.
On 10/18/2017 12:02 PM, Cornelia Huck wrote: > On Wed, 18 Oct 2017 12:00:05 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 17.10.2017 16:04, Halil Pasic wrote: >>> Simplify the error handling of the MSCH. Let the code detecting the >>> condition tell (in a less ambiguous way) how it's to be handled. No >>> changes in behavior. >> >> ok, so you claim no changes in behavior ... >> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> hw/s390x/css.c | 18 +++++------------- >>> include/hw/s390x/css.h | 2 +- >>> target/s390x/ioinst.c | 23 ++++------------------- >>> 3 files changed, 10 insertions(+), 33 deletions(-) >>> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index b9e0329825..30fc236946 100644 >>> --- a/hw/s390x/css.c >>> +++ b/hw/s390x/css.c >>> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) >>> } >>> } >>> >>> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) >>> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) >>> { >>> SCSW *s = &sch->curr_status.scsw; >>> PMCW *p = &sch->curr_status.pmcw; >>> uint16_t oldflags; >>> - int ret; >>> SCHIB schib; >>> >>> if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { >>> - ret = 0; >>> - goto out; >>> + return IOINST_CC_EXPECTED; >>> } >>> >>> if (s->ctrl & SCSW_STCTL_STATUS_PEND) { >>> - ret = -EINPROGRESS; >>> - goto out; >>> + return IOINST_CC_STATUS_PRESENT; >>> } >>> >>> if (s->ctrl & >>> (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { >>> - ret = -EBUSY; >>> - goto out; >>> + return IOINST_CC_STATUS_PRESENT; >>> } >> >> ... but here you change -EBUSY (which got mapped to CC=2) to >> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e. >> this is either a bug, or you should update the patch description with a >> justification for this change in behavior. > > Indeed, that's a bug. We still want cc 2. > Agree, it is a bug. It was OK in v1 but was already buggy in v2. Conny can you fix this up as well please? Thanks in advance! Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-18 13:01:03 +0200]: > > > On 10/18/2017 12:02 PM, Cornelia Huck wrote: > > On Wed, 18 Oct 2017 12:00:05 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 17.10.2017 16:04, Halil Pasic wrote: > >>> Simplify the error handling of the MSCH. Let the code detecting the > >>> condition tell (in a less ambiguous way) how it's to be handled. No > >>> changes in behavior. > >> > >> ok, so you claim no changes in behavior ... > >> > >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>> --- > >>> hw/s390x/css.c | 18 +++++------------- > >>> include/hw/s390x/css.h | 2 +- > >>> target/s390x/ioinst.c | 23 ++++------------------- > >>> 3 files changed, 10 insertions(+), 33 deletions(-) > >>> > >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >>> index b9e0329825..30fc236946 100644 > >>> --- a/hw/s390x/css.c > >>> +++ b/hw/s390x/css.c > >>> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > >>> } > >>> } > >>> > >>> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > >>> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > >>> { > >>> SCSW *s = &sch->curr_status.scsw; > >>> PMCW *p = &sch->curr_status.pmcw; > >>> uint16_t oldflags; > >>> - int ret; > >>> SCHIB schib; > >>> > >>> if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { > >>> - ret = 0; > >>> - goto out; > >>> + return IOINST_CC_EXPECTED; > >>> } > >>> > >>> if (s->ctrl & SCSW_STCTL_STATUS_PEND) { > >>> - ret = -EINPROGRESS; > >>> - goto out; > >>> + return IOINST_CC_STATUS_PRESENT; > >>> } > >>> > >>> if (s->ctrl & > >>> (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { > >>> - ret = -EBUSY; > >>> - goto out; > >>> + return IOINST_CC_STATUS_PRESENT; > >>> } > >> > >> ... but here you change -EBUSY (which got mapped to CC=2) to > >> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e. > >> this is either a bug, or you should update the patch description with a > >> justification for this change in behavior. > > > > Indeed, that's a bug. We still want cc 2. > > > > Agree, it is a bug. It was OK in v1 but was already buggy in > v2. > > Conny can you fix this up as well please? > > Thanks in advance! > I saw Conny fixed this in her branch. So: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index b9e0329825..30fc236946 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) } } -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) { SCSW *s = &sch->curr_status.scsw; PMCW *p = &sch->curr_status.pmcw; uint16_t oldflags; - int ret; SCHIB schib; if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) { - ret = 0; - goto out; + return IOINST_CC_EXPECTED; } if (s->ctrl & SCSW_STCTL_STATUS_PEND) { - ret = -EINPROGRESS; - goto out; + return IOINST_CC_STATUS_PRESENT; } if (s->ctrl & (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC)) { - ret = -EBUSY; - goto out; + return IOINST_CC_STATUS_PRESENT; } copy_schib_from_guest(&schib, orig_schib); @@ -1395,11 +1391,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *orig_schib) && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) { sch->disable_cb(sch); } - - ret = 0; - -out: - return ret; + return IOINST_CC_EXPECTED; } IOInstEnding css_do_xsch(SubchDev *sch) diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 0c1efbae39..a74f6cc274 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -239,7 +239,7 @@ bool css_subch_visible(SubchDev *sch); void css_conditional_io_interrupt(SubchDev *sch); int css_do_stsch(SubchDev *sch, SCHIB *schib); bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid); -int css_do_msch(SubchDev *sch, const SCHIB *schib); +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib); IOInstEnding css_do_xsch(SubchDev *sch); IOInstEnding css_do_csch(SubchDev *sch); IOInstEnding css_do_hsch(SubchDev *sch); diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c index 8e4dacafd8..23962fbebc 100644 --- a/target/s390x/ioinst.c +++ b/target/s390x/ioinst.c @@ -111,8 +111,6 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) SubchDev *sch; SCHIB schib; uint64_t addr; - int ret = -ENODEV; - int cc; CPUS390XState *env = &cpu->env; uint8_t ar; @@ -131,24 +129,11 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) } trace_ioinst_sch_id("msch", cssid, ssid, schid); sch = css_find_subch(m, cssid, ssid, schid); - if (sch && css_subch_visible(sch)) { - ret = css_do_msch(sch, &schib); - } - switch (ret) { - case -ENODEV: - cc = 3; - break; - case -EBUSY: - cc = 2; - break; - case 0: - cc = 0; - break; - default: - cc = 1; - break; + if (!sch || !css_subch_visible(sch)) { + setcc(cpu, 3); + return; } - setcc(cpu, cc); + setcc(cpu, css_do_msch(sch, &schib)); } static void copy_orb_from_guest(ORB *dest, const ORB *src)
Simplify the error handling of the MSCH. Let the code detecting the condition tell (in a less ambiguous way) how it's to be handled. No changes in behavior. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 18 +++++------------- include/hw/s390x/css.h | 2 +- target/s390x/ioinst.c | 23 ++++------------------- 3 files changed, 10 insertions(+), 33 deletions(-)