| Message ID | 20171017140453.51099-3-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve error handling for IO instr | expand |
On Tue, 17 Oct 2017 16:04:48 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > CSS code needs to tell the IO instruction handlers located in how should > the emulated instruction be ended. Currently this is done by returning > generic (POSIX) error codes, and mapping them to outcomes like condition > codes. This makes bugs easy to create and hard to recognise. > > As a preparation for moving a way form (mis)using generic error codes for s/form/from/ > flow control let us introduce a type which tells the instruction > handler function how to end the instruction, in a more straight-forward > and less ambiguous way. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > include/hw/s390x/css.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 69b374730e..7e0dbd162f 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -99,6 +99,22 @@ typedef struct CcwDataStream { > hwaddr cda; > } CcwDataStream; > > +/* > + * IO instructions conclude according this. Currently we have only s/this/to this/ > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for blanks between numbers? > + * IO instructions is described briefly. For more details consult the PoP. > + */ > +typedef enum IOInstEnding { > + /* produced expected result */ > + IOINST_CC_EXPECTED = 0, > + /* status conditions were present or produced alternate result */ > + IOINST_CC_STATUS_PRESENT = 1, > + /* inst. ineffective because busy with previously initiated function */ > + IOINST_CC_BUSY = 2, > + /* inst. ineffective because not operational */ > + IOINST_CC_NOT_OPERATIONAL = 3 > +} IOInstEnding; This looks a bit odd for some I/O instructions (STCRW, TPI, TSCH), but is fine for the others. But as the PoP also defines the meanings as above, it should be fine (and not confusing). > + > typedef struct SubchDev SubchDev; > struct SubchDev { > /* channel-subsystem related things: */
On 10/17/2017 04:58 PM, Cornelia Huck wrote: > On Tue, 17 Oct 2017 16:04:48 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> CSS code needs to tell the IO instruction handlers located in how should >> the emulated instruction be ended. Currently this is done by returning >> generic (POSIX) error codes, and mapping them to outcomes like condition >> codes. This makes bugs easy to create and hard to recognise. also s/recognise/recognize/ To on mix American and British. >> >> As a preparation for moving a way form (mis)using generic error codes for > > s/form/from/ > Sorry this seems to be a recurring typo of me (not detected by spell check because both valid). >> flow control let us introduce a type which tells the instruction >> handler function how to end the instruction, in a more straight-forward >> and less ambiguous way. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> include/hw/s390x/css.h | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >> index 69b374730e..7e0dbd162f 100644 >> --- a/include/hw/s390x/css.h >> +++ b/include/hw/s390x/css.h >> @@ -99,6 +99,22 @@ typedef struct CcwDataStream { >> hwaddr cda; >> } CcwDataStream; >> >> +/* >> + * IO instructions conclude according this. Currently we have only > > s/this/to this/ > >> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for > > blanks between numbers? Overrated. Just joking. Definitely blanks between numbers. > >> + * IO instructions is described briefly. For more details consult the PoP. >> + */ >> +typedef enum IOInstEnding { >> + /* produced expected result */ >> + IOINST_CC_EXPECTED = 0, >> + /* status conditions were present or produced alternate result */ >> + IOINST_CC_STATUS_PRESENT = 1, >> + /* inst. ineffective because busy with previously initiated function */ >> + IOINST_CC_BUSY = 2, >> + /* inst. ineffective because not operational */ >> + IOINST_CC_NOT_OPERATIONAL = 3 >> +} IOInstEnding; > > This looks a bit odd for some I/O instructions (STCRW, TPI, TSCH), but > is fine for the others. But as the PoP also defines the meanings as > above, it should be fine (and not confusing). Yeah. The original idea was to keep stuff abstract, but I decided to go with the names carrying meaning because Thomas seems to be more reliable than me when it comes to matters of taste, and also for consistency with SIGP_CC_*. I think, in the end it does not matter that much. > >> + >> typedef struct SubchDev SubchDev; >> struct SubchDev { >> /* channel-subsystem related things: */ > >
On 17.10.2017 16:04, Halil Pasic wrote: > CSS code needs to tell the IO instruction handlers located in how should locate in? ... sorry, I've got trouble to parse this sentence ... > the emulated instruction be ended. Currently this is done by returning > generic (POSIX) error codes, and mapping them to outcomes like condition > codes. This makes bugs easy to create and hard to recognise. > > As a preparation for moving a way form (mis)using generic error codes for s/a way/away/ ? > flow control let us introduce a type which tells the instruction > handler function how to end the instruction, in a more straight-forward > and less ambiguous way. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > include/hw/s390x/css.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 69b374730e..7e0dbd162f 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -99,6 +99,22 @@ typedef struct CcwDataStream { > hwaddr cda; > } CcwDataStream; > > +/* > + * IO instructions conclude according this. Currently we have only Maybe rather "terminate like this" or "finish like this"? I'm not a native speaker, but "conclude" sounds a little bit strange to me here. > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for > + * IO instructions is described briefly. For more details consult the PoP. > + */ > +typedef enum IOInstEnding { > + /* produced expected result */ > + IOINST_CC_EXPECTED = 0, > + /* status conditions were present or produced alternate result */ > + IOINST_CC_STATUS_PRESENT = 1, > + /* inst. ineffective because busy with previously initiated function */ > + IOINST_CC_BUSY = 2, > + /* inst. ineffective because not operational */ > + IOINST_CC_NOT_OPERATIONAL = 3 > +} IOInstEnding; > + > typedef struct SubchDev SubchDev; > struct SubchDev { > /* channel-subsystem related things: */ > With the patch description fixed: Reviewed-by: Thomas Huth <thuth@redhat.com>
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:48 +0200]: [...] > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 69b374730e..7e0dbd162f 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -99,6 +99,22 @@ typedef struct CcwDataStream { > hwaddr cda; > } CcwDataStream; > > +/* > + * IO instructions conclude according this. Currently we have only > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for > + * IO instructions is described briefly. For more details consult the PoP. > + */ > +typedef enum IOInstEnding { > + /* produced expected result */ > + IOINST_CC_EXPECTED = 0, > + /* status conditions were present or produced alternate result */ > + IOINST_CC_STATUS_PRESENT = 1, > + /* inst. ineffective because busy with previously initiated function */ > + IOINST_CC_BUSY = 2, > + /* inst. ineffective because not operational */ > + IOINST_CC_NOT_OPERATIONAL = 3 > +} IOInstEnding; > + > typedef struct SubchDev SubchDev; > struct SubchDev { > /* channel-subsystem related things: */ > -- > 2.13.5 > With what has been pointed out by Conny and Thomas addressed: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
On Wed, 18 Oct 2017 10:45:28 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 17.10.2017 16:04, Halil Pasic wrote: > > CSS code needs to tell the IO instruction handlers located in how should > > locate in? ... sorry, I've got trouble to parse this sentence ... "located in ioinst.c", I guess. And maybe also move "should" a bit later in the sentence. > > > the emulated instruction be ended. Currently this is done by returning > > generic (POSIX) error codes, and mapping them to outcomes like condition > > codes. This makes bugs easy to create and hard to recognise. > > > > As a preparation for moving a way form (mis)using generic error codes for > > s/a way/away/ ? I can fix that up as well. > > > flow control let us introduce a type which tells the instruction > > handler function how to end the instruction, in a more straight-forward > > and less ambiguous way. > > > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > --- > > include/hw/s390x/css.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > > index 69b374730e..7e0dbd162f 100644 > > --- a/include/hw/s390x/css.h > > +++ b/include/hw/s390x/css.h > > @@ -99,6 +99,22 @@ typedef struct CcwDataStream { > > hwaddr cda; > > } CcwDataStream; > > > > +/* > > + * IO instructions conclude according this. Currently we have only > > Maybe rather "terminate like this" or "finish like this"? I'm not a > native speaker, but "conclude" sounds a little bit strange to me here. I do remember reading "conclude" in various places in s390 documentation, so I think I'll keep this. > > > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for > > + * IO instructions is described briefly. For more details consult the PoP. > > + */ > > +typedef enum IOInstEnding { > > + /* produced expected result */ > > + IOINST_CC_EXPECTED = 0, > > + /* status conditions were present or produced alternate result */ > > + IOINST_CC_STATUS_PRESENT = 1, > > + /* inst. ineffective because busy with previously initiated function */ > > + IOINST_CC_BUSY = 2, > > + /* inst. ineffective because not operational */ > > + IOINST_CC_NOT_OPERATIONAL = 3 > > +} IOInstEnding; > > + > > typedef struct SubchDev SubchDev; > > struct SubchDev { > > /* channel-subsystem related things: */ > > > > With the patch description fixed: > > Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 69b374730e..7e0dbd162f 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -99,6 +99,22 @@ typedef struct CcwDataStream { hwaddr cda; } CcwDataStream; +/* + * IO instructions conclude according this. Currently we have only + * cc codes. Valid values are 0,1,2,3 and the generic semantic for + * IO instructions is described briefly. For more details consult the PoP. + */ +typedef enum IOInstEnding { + /* produced expected result */ + IOINST_CC_EXPECTED = 0, + /* status conditions were present or produced alternate result */ + IOINST_CC_STATUS_PRESENT = 1, + /* inst. ineffective because busy with previously initiated function */ + IOINST_CC_BUSY = 2, + /* inst. ineffective because not operational */ + IOINST_CC_NOT_OPERATIONAL = 3 +} IOInstEnding; + typedef struct SubchDev SubchDev; struct SubchDev { /* channel-subsystem related things: */
CSS code needs to tell the IO instruction handlers located in how should the emulated instruction be ended. Currently this is done by returning generic (POSIX) error codes, and mapping them to outcomes like condition codes. This makes bugs easy to create and hard to recognise. As a preparation for moving a way form (mis)using generic error codes for flow control let us introduce a type which tells the instruction handler function how to end the instruction, in a more straight-forward and less ambiguous way. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- include/hw/s390x/css.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)