[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [qemu-s390x] [PATCH v3 2/7] s390x/css: IO instr handler ending contr
From: |
Cornelia Huck |
Subject: |
Re: [qemu-s390x] [PATCH v3 2/7] s390x/css: IO instr handler ending control |
Date: |
Wed, 18 Oct 2017 11:34:15 +0200 |
On Wed, 18 Oct 2017 10:45:28 +0200
Thomas Huth <address@hidden> 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 <address@hidden>
> > ---
> > 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 <address@hidden>