| Message ID | 20170905111645.18068-2-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 5 Sep 2017 13:16:41 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > This is a preparation for introducing handling for indirect data > addressing and modified indirect data addressing (CCW). Here we introduce > an interface which should make the addressing scheme transparent for the > client code. Here we implement only basic scheme (no IDA or MIDA). s/basic scheme/the basic scheme/ > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ > include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 122 insertions(+) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 1880b1a0ff..87d913f81c 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) > } > return ret; > } > +/** > + * If out of bounds marks the stream broken. If broken returns -EINVAL, > + * otherwise the requested length (may be zero) s/requested length/requested length/ > + */ > +static inline int cds_check_len(CcwDataStream *cds, int len) > +{ > + if (cds->at_byte + len > cds->count) { > + cds->flags |= CDS_F_STREAM_BROKEN; > + } > + return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; > +} > + > +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, > + CcwDataStreamOp op) > +{ > + int ret; > + > + ret = cds_check_len(cds, len); > + if (ret <= 0) { > + return ret; > + } > + if (op == CDS_OP_A) { > + goto incr; > + } > + ret = address_space_rw(&address_space_memory, cds->cda, > + MEMTXATTRS_UNSPECIFIED, buff, len, op); > + if (ret != MEMTX_OK) { > + cds->flags |= CDS_F_STREAM_BROKEN; Do we want to distinguish between different reasons for the stream being broken? I.e, is there a case where we want to signal different errors back to the caller? > + return -EINVAL; > + } > +incr: > + cds->at_byte += len; > + cds->cda += len; > + return 0; > +} > + > +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > +{ > + /* > + * We don't support MIDA (an optional facility) yet and we > + * catch this earlier. Just for expressing the precondition. > + */ > + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); I don't know, this is infrastructure, should it trust its callers? If you keep the assert, please make it g_assert(). > + cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) | > + (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) | > + (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); > + cds->count = ccw->count; > + cds->cda_orig = ccw->cda; > + ccw_dstream_rewind(cds); > + if (!(cds->flags & CDS_F_IDA)) { > + cds->op_handler = ccw_dstream_rw_noflags; > + } else { > + assert(false); Same here. Or should we make this return an error and have the callers deal with that? (I still need to look at the users.) > + } > +} > > static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, > bool suspend_allowed)
On 09/06/2017 02:18 PM, Cornelia Huck wrote: > On Tue, 5 Sep 2017 13:16:41 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> This is a preparation for introducing handling for indirect data >> addressing and modified indirect data addressing (CCW). Here we introduce >> an interface which should make the addressing scheme transparent for the >> client code. Here we implement only basic scheme (no IDA or MIDA). > > s/basic scheme/the basic scheme/ > Nod. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ >> include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 122 insertions(+) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 1880b1a0ff..87d913f81c 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) >> } >> return ret; >> } >> +/** >> + * If out of bounds marks the stream broken. If broken returns -EINVAL, >> + * otherwise the requested length (may be zero) > > s/requested length/requested length/ > Nod. >> + */ >> +static inline int cds_check_len(CcwDataStream *cds, int len) >> +{ >> + if (cds->at_byte + len > cds->count) { >> + cds->flags |= CDS_F_STREAM_BROKEN; >> + } >> + return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; >> +} >> + >> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, >> + CcwDataStreamOp op) >> +{ >> + int ret; >> + >> + ret = cds_check_len(cds, len); >> + if (ret <= 0) { >> + return ret; >> + } >> + if (op == CDS_OP_A) { >> + goto incr; >> + } >> + ret = address_space_rw(&address_space_memory, cds->cda, >> + MEMTXATTRS_UNSPECIFIED, buff, len, op); >> + if (ret != MEMTX_OK) { >> + cds->flags |= CDS_F_STREAM_BROKEN; > > Do we want to distinguish between different reasons for the stream > being broken? I.e, is there a case where we want to signal different > errors back to the caller? > Hm, after I've done the error handling it seems that basically everything is to be handled with a program check. The stream records the place where the problem was encountered, so for debug we would not have to search for long. There seems to be no need to distinguish. >> + return -EINVAL; >> + } >> +incr: >> + cds->at_byte += len; >> + cds->cda += len; >> + return 0; >> +} >> + >> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) >> +{ >> + /* >> + * We don't support MIDA (an optional facility) yet and we >> + * catch this earlier. Just for expressing the precondition. >> + */ >> + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); > > I don't know, this is infrastructure, should it trust its callers? If > you keep the assert, please make it g_assert(). Why g_assert? I think g_assert comes form a test framework, this is not test code. I feel more comfortable having this assert around. > >> + cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) | >> + (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) | >> + (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); >> + cds->count = ccw->count; >> + cds->cda_orig = ccw->cda; >> + ccw_dstream_rewind(cds); >> + if (!(cds->flags & CDS_F_IDA)) { >> + cds->op_handler = ccw_dstream_rw_noflags; >> + } else { >> + assert(false); > > Same here. > > Or should we make this return an error and have the callers deal with > that? (I still need to look at the users.) > This assert is going away soon (patch 4). I'm not sure doing much more here is justified. >> + } >> +} >> >> static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, >> bool suspend_allowed) >
On Wed, 6 Sep 2017 14:40:48 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/06/2017 02:18 PM, Cornelia Huck wrote: > > On Tue, 5 Sep 2017 13:16:41 +0200 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, > >> + CcwDataStreamOp op) > >> +{ > >> + int ret; > >> + > >> + ret = cds_check_len(cds, len); > >> + if (ret <= 0) { > >> + return ret; > >> + } > >> + if (op == CDS_OP_A) { > >> + goto incr; > >> + } > >> + ret = address_space_rw(&address_space_memory, cds->cda, > >> + MEMTXATTRS_UNSPECIFIED, buff, len, op); > >> + if (ret != MEMTX_OK) { > >> + cds->flags |= CDS_F_STREAM_BROKEN; > > > > Do we want to distinguish between different reasons for the stream > > being broken? I.e, is there a case where we want to signal different > > errors back to the caller? > > > > Hm, after I've done the error handling it seems that basically > everything is to be handled with a program check. The stream > records the place where the problem was encountered, so for debug > we would not have to search for long. > > There seems to be no need to distinguish. OK, makes sense. Let's keep it as it is. > > >> + return -EINVAL; > >> + } > >> +incr: > >> + cds->at_byte += len; > >> + cds->cda += len; > >> + return 0; > >> +} > >> + > >> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > >> +{ > >> + /* > >> + * We don't support MIDA (an optional facility) yet and we > >> + * catch this earlier. Just for expressing the precondition. > >> + */ > >> + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); > > > > I don't know, this is infrastructure, should it trust its callers? If > > you keep the assert, please make it g_assert(). > > > Why g_assert? I think g_assert comes form a test framework, this is not > test code. g_assert() is glib, no? > > I feel more comfortable having this assert around. Let's revisit that when we add mida support :) I don't really object to it. > > > > >> + cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) | > >> + (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) | > >> + (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); > >> + cds->count = ccw->count; > >> + cds->cda_orig = ccw->cda; > >> + ccw_dstream_rewind(cds); > >> + if (!(cds->flags & CDS_F_IDA)) { > >> + cds->op_handler = ccw_dstream_rw_noflags; > >> + } else { > >> + assert(false); > > > > Same here. > > > > Or should we make this return an error and have the callers deal with > > that? (I still need to look at the users.) > > > > This assert is going away soon (patch 4). I'm not sure doing much more here > is justified. OK, if it is transient anyway...
On 09/06/2017 02:51 PM, Cornelia Huck wrote: >>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) >>>> +{ >>>> + /* >>>> + * We don't support MIDA (an optional facility) yet and we >>>> + * catch this earlier. Just for expressing the precondition. >>>> + */ >>>> + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); >>> I don't know, this is infrastructure, should it trust its callers? If >>> you keep the assert, please make it g_assert(). >> >> Why g_assert? I think g_assert comes form a test framework, this is not >> test code. > g_assert() is glib, no? > It lives in GLib > GLib Utilities > Testing: https://developer.gnome.org/glib/stable/glib-Testing.html The description of "Testing" starts like this: "GLib provides a framework for writing and maintaining unit tests in parallel to the code they are testing. The API is designed according to established concepts found in the other test frameworks (JUnit, NUnit, RUnit), which in turn is based on smalltalk unit testing concepts." So yes, it's both glib and testing framework. This is why I ask why should one use g_assert in not-unit-test code. Halil
On Mon, 11 Sep 2017 18:36:00 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/06/2017 02:51 PM, Cornelia Huck wrote: > >>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > >>>> +{ > >>>> + /* > >>>> + * We don't support MIDA (an optional facility) yet and we > >>>> + * catch this earlier. Just for expressing the precondition. > >>>> + */ > >>>> + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); > >>> I don't know, this is infrastructure, should it trust its callers? If > >>> you keep the assert, please make it g_assert(). > >> > >> Why g_assert? I think g_assert comes form a test framework, this is not > >> test code. > > g_assert() is glib, no? > > > > It lives in GLib > GLib Utilities > Testing: > https://developer.gnome.org/glib/stable/glib-Testing.html > > The description of "Testing" starts like this: "GLib provides a framework > for writing and maintaining unit tests in parallel to the code they are > testing. The API is designed according to established concepts found in > the other test frameworks (JUnit, NUnit, RUnit), which in turn is based > on smalltalk unit testing concepts." > > So yes, it's both glib and testing framework. This is why I > ask why should one use g_assert in not-unit-test code. I have searched the archives, but unfortunately was not able to come to a conclusion. Checkpatch advises against using anything but g_assert or g_assert_not_reached in anything but test code, but that is because those other g_asserts can be made non-fatal. g_assert_not_reached does not seem to have a non-glib equivalent. I have it somewhere in the back of my mind that g_assert should be preferred...
On 09/13/2017 11:53 AM, Cornelia Huck wrote: > On Mon, 11 Sep 2017 18:36:00 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 09/06/2017 02:51 PM, Cornelia Huck wrote: >>>>>> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) >>>>>> +{ >>>>>> + /* >>>>>> + * We don't support MIDA (an optional facility) yet and we >>>>>> + * catch this earlier. Just for expressing the precondition. >>>>>> + */ >>>>>> + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); >>>>> I don't know, this is infrastructure, should it trust its callers? If >>>>> you keep the assert, please make it g_assert(). >>>> >>>> Why g_assert? I think g_assert comes form a test framework, this is not >>>> test code. >>> g_assert() is glib, no? >>> >> >> It lives in GLib > GLib Utilities > Testing: >> https://developer.gnome.org/glib/stable/glib-Testing.html >> >> The description of "Testing" starts like this: "GLib provides a framework >> for writing and maintaining unit tests in parallel to the code they are >> testing. The API is designed according to established concepts found in >> the other test frameworks (JUnit, NUnit, RUnit), which in turn is based >> on smalltalk unit testing concepts." >> >> So yes, it's both glib and testing framework. This is why I >> ask why should one use g_assert in not-unit-test code. > > I have searched the archives, but unfortunately was not able to come to > a conclusion. Checkpatch advises against using anything but g_assert or > g_assert_not_reached in anything but test code, but that is because > those other g_asserts can be made non-fatal. g_assert_not_reached does > not seem to have a non-glib equivalent. > I think the standard library equivalent of g_assert_not_reached is assert(false). Yeah, checkpatch does not advise against using g_assert and g_assert_not_reached in non-test code, but it does not advise against using standard lib assert. > I have it somewhere in the back of my mind that g_assert should be > preferred... > OK, I will use g_assert.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 1880b1a0ff..87d913f81c 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -783,6 +783,61 @@ static CCW1 copy_ccw_from_guest(hwaddr addr, bool fmt1) } return ret; } +/** + * If out of bounds marks the stream broken. If broken returns -EINVAL, + * otherwise the requested length (may be zero) + */ +static inline int cds_check_len(CcwDataStream *cds, int len) +{ + if (cds->at_byte + len > cds->count) { + cds->flags |= CDS_F_STREAM_BROKEN; + } + return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; +} + +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, + CcwDataStreamOp op) +{ + int ret; + + ret = cds_check_len(cds, len); + if (ret <= 0) { + return ret; + } + if (op == CDS_OP_A) { + goto incr; + } + ret = address_space_rw(&address_space_memory, cds->cda, + MEMTXATTRS_UNSPECIFIED, buff, len, op); + if (ret != MEMTX_OK) { + cds->flags |= CDS_F_STREAM_BROKEN; + return -EINVAL; + } +incr: + cds->at_byte += len; + cds->cda += len; + return 0; +} + +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) +{ + /* + * We don't support MIDA (an optional facility) yet and we + * catch this earlier. Just for expressing the precondition. + */ + assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); + cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) | + (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) | + (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); + cds->count = ccw->count; + cds->cda_orig = ccw->cda; + ccw_dstream_rewind(cds); + if (!(cds->flags & CDS_F_IDA)) { + cds->op_handler = ccw_dstream_rw_noflags; + } else { + assert(false); + } +} static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr, bool suspend_allowed) diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 5c5fe6b202..7b7207f9a2 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -74,6 +74,29 @@ typedef struct CMBE { uint32_t reserved[7]; } QEMU_PACKED CMBE; +typedef enum CcwDataStreamOp { + CDS_OP_R = 0, + CDS_OP_W = 1, + CDS_OP_A = 2 +} CcwDataStreamOp; + +/** normal usage is via SuchchDev.cds instead of instantiating */ +typedef struct CcwDataStream { +#define CDS_F_IDA 0x01 +#define CDS_F_MIDA 0x02 +#define CDS_F_I2K 0x04 +#define CDS_F_C64 0x08 +#define CDS_F_STREAM_BROKEN 0x80 + uint8_t flags; + uint8_t at_idaw; + uint16_t at_byte; + uint16_t count; + uint32_t cda_orig; + int (*op_handler)(struct CcwDataStream *cds, void *buff, int len, + CcwDataStreamOp op); + hwaddr cda; +} CcwDataStream; + typedef struct SubchDev SubchDev; struct SubchDev { /* channel-subsystem related things: */ @@ -91,6 +114,7 @@ struct SubchDev { uint8_t ccw_no_data_cnt; uint16_t migrated_schid; /* used for missmatch detection */ ORB orb; + CcwDataStream cds; /* transport-provided data: */ int (*ccw_cb) (SubchDev *, CCW1); void (*disable_cb)(SubchDev *); @@ -238,4 +262,47 @@ SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss, /** Turn on css migration */ void css_register_vmstate(void); + +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb); + +static inline void ccw_dstream_rewind(CcwDataStream *cds) +{ + cds->at_byte = 0; + cds->at_idaw = 0; + cds->cda = cds->cda_orig; +} + +static inline bool ccw_dstream_good(CcwDataStream *cds) +{ + return !(cds->flags & CDS_F_STREAM_BROKEN); +} + +static inline uint16_t ccw_dstream_residual_count(CcwDataStream *cds) +{ + return cds->count - cds->at_byte; +} + +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds) +{ + return ccw_dstream_good(cds) ? ccw_dstream_residual_count(cds) : 0; +} + +static inline int ccw_dstream_advance(CcwDataStream *cds, int len) +{ + return cds->op_handler(cds, NULL, len, CDS_OP_A); +} + +static inline int ccw_dstream_write_buf(CcwDataStream *cds, void *buff, int len) +{ + return cds->op_handler(cds, buff, len, CDS_OP_W); +} + +static inline int ccw_dstream_read_buf(CcwDataStream *cds, void *buff, int len) +{ + return cds->op_handler(cds, buff, len, CDS_OP_R); +} + +#define ccw_dstream_read(cds, v) ccw_dstream_read_buf((cds), &(v), sizeof(v)) +#define ccw_dstream_write(cds, v) ccw_dstream_write_buf((cds), &(v), sizeof(v)) + #endif
This is a preparation for introducing handling for indirect data addressing and modified indirect data addressing (CCW). Here we introduce an interface which should make the addressing scheme transparent for the client code. Here we implement only basic scheme (no IDA or MIDA). Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 55 +++++++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)