| Message ID | 20170919182745.90280-5-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | add CCW indirect data access support | expand |
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]: > The architecture mandates the addresses to be accessed on the first > indirection level (that is, the data addresses without IDA, and the > (M)IDAW addresses with (M)IDA) to be checked against an CCW format > dependent limit maximum address. If a violation is detected, the storage > access is not to be performed and a channel program check needs to be > generated. As of today, we fail to do this check. > > Let us stick even closer to the architecture specification. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 10 ++++++++++ > include/hw/s390x/css.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 6b0cd8861b..2d37a9ddde 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len) > return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; > } > > +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1) > +{ > + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24)); > +} > + > static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, > CcwDataStreamOp op) > { > @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, > if (ret <= 0) { > return ret; > } > + if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) { > + return -EINVAL; /* channel program check */ > + } > if (op == CDS_OP_A) { > goto incr; > } > @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > g_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) | > + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | This reminds me one more question: Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane. > (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); > + > cds->count = ccw->count; > cds->cda_orig = ccw->cda; > ccw_dstream_rewind(cds); > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 078356e94c..69b374730e 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -87,6 +87,7 @@ typedef struct CcwDataStream { > #define CDS_F_MIDA 0x02 > #define CDS_F_I2K 0x04 > #define CDS_F_C64 0x08 > +#define CDS_F_FMT 0x10 /* CCW format-1 */ > #define CDS_F_STREAM_BROKEN 0x80 > uint8_t flags; > uint8_t at_idaw; > -- > 2.13.5 >
On Tue, 19 Sep 2017 20:27:44 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > The architecture mandates the addresses to be accessed on the first > indirection level (that is, the data addresses without IDA, and the > (M)IDAW addresses with (M)IDA) to be checked against an CCW format > dependent limit maximum address. If a violation is detected, the storage > access is not to be performed and a channel program check needs to be > generated. As of today, we fail to do this check. > > Let us stick even closer to the architecture specification. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/s390x/css.c | 10 ++++++++++ > include/hw/s390x/css.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 6b0cd8861b..2d37a9ddde 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len) > return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; > } > > +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1) cds_cda_limit_ok? > +{ > + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24)); > +} > + > static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, > CcwDataStreamOp op) > { Looks good.
On Wed, 20 Sep 2017 15:47:51 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]: > > @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > > g_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) | > > + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | > This reminds me one more question: > Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a > fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane. That's just a consequence of us translating everything to format-1 ccws. A bit confusing, but no problem if we pay attention to the format bit everywhere it makes a difference.
On 09/20/2017 10:25 AM, Cornelia Huck wrote: > On Wed, 20 Sep 2017 15:47:51 +0800 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]: > >>> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) >>> g_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) | >>> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | >> This reminds me one more question: >> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a >> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane. > > That's just a consequence of us translating everything to format-1 > ccws. A bit confusing, but no problem if we pay attention to the format > bit everywhere it makes a difference. > Agree. Halil
On 09/20/2017 10:06 AM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 20:27:44 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> The architecture mandates the addresses to be accessed on the first >> indirection level (that is, the data addresses without IDA, and the >> (M)IDAW addresses with (M)IDA) to be checked against an CCW format >> dependent limit maximum address. If a violation is detected, the storage >> access is not to be performed and a channel program check needs to be >> generated. As of today, we fail to do this check. >> >> Let us stick even closer to the architecture specification. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/s390x/css.c | 10 ++++++++++ >> include/hw/s390x/css.h | 1 + >> 2 files changed, 11 insertions(+) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 6b0cd8861b..2d37a9ddde 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len) >> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; >> } >> >> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1) > > cds_cda_limit_ok? > I use cda to point to the 2 level in case of IDA. This is about level 1 (addressed by the ccw directly). That's why I used ccw_addrs but if you think cds_cda_limit_ok is better I can live with that. We could also think about renaming cds->cda. Btw what does cda stand for (channel data address is my guess)? Regards, Halil >> +{ >> + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24)); >> +} >> + >> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, >> CcwDataStreamOp op) >> { > > Looks good. >
On Wed, 20 Sep 2017 13:34:21 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/20/2017 10:06 AM, Cornelia Huck wrote: > > On Tue, 19 Sep 2017 20:27:44 +0200 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> The architecture mandates the addresses to be accessed on the first > >> indirection level (that is, the data addresses without IDA, and the > >> (M)IDAW addresses with (M)IDA) to be checked against an CCW format > >> dependent limit maximum address. If a violation is detected, the storage > >> access is not to be performed and a channel program check needs to be > >> generated. As of today, we fail to do this check. > >> > >> Let us stick even closer to the architecture specification. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> hw/s390x/css.c | 10 ++++++++++ > >> include/hw/s390x/css.h | 1 + > >> 2 files changed, 11 insertions(+) > >> > >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >> index 6b0cd8861b..2d37a9ddde 100644 > >> --- a/hw/s390x/css.c > >> +++ b/hw/s390x/css.c > >> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len) > >> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; > >> } > >> > >> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1) > > > > cds_cda_limit_ok? > > > > I use cda to point to the 2 level in case of IDA. This is about > level 1 (addressed by the ccw directly). That's why I used ccw_addrs > but if you think cds_cda_limit_ok is better I can live with that. I don't care that much, tbh. > > We could also think about renaming cds->cda. Btw what does cda stand > for (channel data address is my guess)? Yes, cda should stand for 'channel data address'. Its usage in cds->cda is probably the source of this minor confusion. But, as said, I don't really care that much; so unless one of the other folks has a strong opinion, feel free to leave as-is.
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 13:02:59 +0200]: > > > On 09/20/2017 10:25 AM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 15:47:51 +0800 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]: > > > >>> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) > >>> g_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) | > >>> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | > >> This reminds me one more question: > >> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a > >> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane. > > > > That's just a consequence of us translating everything to format-1 > > ccws. A bit confusing, but no problem if we pay attention to the format > > bit everywhere it makes a difference. > > > > Agree. Ok. I'm fine with this. > > Halil
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 6b0cd8861b..2d37a9ddde 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len) return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len; } +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1) +{ + return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24)); +} + static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, CcwDataStreamOp op) { @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len, if (ret <= 0) { return ret; } + if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) { + return -EINVAL; /* channel program check */ + } if (op == CDS_OP_A) { goto incr; } @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) g_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) | + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0); + cds->count = ccw->count; cds->cda_orig = ccw->cda; ccw_dstream_rewind(cds); diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 078356e94c..69b374730e 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -87,6 +87,7 @@ typedef struct CcwDataStream { #define CDS_F_MIDA 0x02 #define CDS_F_I2K 0x04 #define CDS_F_C64 0x08 +#define CDS_F_FMT 0x10 /* CCW format-1 */ #define CDS_F_STREAM_BROKEN 0x80 uint8_t flags; uint8_t at_idaw;
The architecture mandates the addresses to be accessed on the first indirection level (that is, the data addresses without IDA, and the (M)IDAW addresses with (M)IDA) to be checked against an CCW format dependent limit maximum address. If a violation is detected, the storage access is not to be performed and a channel program check needs to be generated. As of today, we fail to do this check. Let us stick even closer to the architecture specification. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/s390x/css.c | 10 ++++++++++ include/hw/s390x/css.h | 1 + 2 files changed, 11 insertions(+)