| Message ID | 20170919182745.90280-6-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > Let's add indirect data addressing support for our virtual channel > subsystem. This implementation does not bother with any kind of > prefetching. We simply step through the IDAL on demand. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 2d37a9ddde..a3ce6d89b6 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -827,6 +827,121 @@ incr: > return 0; > } > > +/* returns values between 1 and bsz, where bsz is a power of 2 */ > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) > +{ > + return bsz - (cda & (bsz - 1)); > +} > + > +static inline uint64_t ccw_ida_block_size(uint8_t flags) > +{ > + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { > + return 1ULL << 12; > + } > + return 1ULL << 11; > +} > + > +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, > + bool idaw_fmt_2) > +{ > + union {uint64_t fmt2; uint32_t fmt1; } idaw; > + int ret; > + hwaddr idaw_addr; > + > + if (idaw_fmt_2) { > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; > + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > + return -EINVAL; /* channel program check */ > + } > + ret = address_space_rw(&address_space_memory, idaw_addr, Ahh, just got one question here: Do we need to considerate endianess for idaw_addr? > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > + sizeof(idaw.fmt2), false); > + cds->cda = be64_to_cpu(idaw.fmt2); > + } else { > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > + return -EINVAL; /* channel program check */ > + } > + ret = address_space_rw(&address_space_memory, idaw_addr, > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > + sizeof(idaw.fmt1), false); > + cds->cda = be64_to_cpu(idaw.fmt1); Still need to check bit 0x80000000 here I think. > + } > + ++(cds->at_idaw); > + if (ret != MEMTX_OK) { > + /* assume inaccessible address */ > + return -EINVAL; /* channel program check */ > + Extra line. > + } > + return 0; > +} > + > +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, > + CcwDataStreamOp op) > +{ > + uint64_t bsz = ccw_ida_block_size(cds->flags); > + int ret = 0; > + uint16_t cont_left, iter_len; > + const bool idaw_fmt2 = cds->flags & CDS_F_C64; > + bool ccw_fmt1 = cds->flags & CDS_F_FMT; Use 'const bool' either? Although I doubt the value of using const here. ;) > + > + ret = cds_check_len(cds, len); > + if (ret <= 0) { > + return ret; > + } [...]
On Tue, 19 Sep 2017 20:27:45 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Let's add indirect data addressing support for our virtual channel > subsystem. This implementation does not bother with any kind of > prefetching. We simply step through the IDAL on demand. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cohuck@redhat.com> My s-o-b is a bit preliminary ;) > --- > hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 2d37a9ddde..a3ce6d89b6 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -827,6 +827,121 @@ incr: > return 0; > } > > +/* returns values between 1 and bsz, where bsz is a power of 2 */ > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) > +{ > + return bsz - (cda & (bsz - 1)); > +} > + > +static inline uint64_t ccw_ida_block_size(uint8_t flags) > +{ > + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { > + return 1ULL << 12; > + } > + return 1ULL << 11; > +} > + > +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, > + bool idaw_fmt_2) > +{ > + union {uint64_t fmt2; uint32_t fmt1; } idaw; > + int ret; > + hwaddr idaw_addr; > + > + if (idaw_fmt_2) { > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; > + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { So, what is supposed to happen if the idaw_addr is not aligned correctly _and_ is violating the limits? > + return -EINVAL; /* channel program check */ > + } > + ret = address_space_rw(&address_space_memory, idaw_addr, > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > + sizeof(idaw.fmt2), false); > + cds->cda = be64_to_cpu(idaw.fmt2); > + } else { > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > + return -EINVAL; /* channel program check */ > + } > + ret = address_space_rw(&address_space_memory, idaw_addr, > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > + sizeof(idaw.fmt1), false); > + cds->cda = be64_to_cpu(idaw.fmt1); > + } > + ++(cds->at_idaw); > + if (ret != MEMTX_OK) { > + /* assume inaccessible address */ > + return -EINVAL; /* channel program check */ > + > + } > + return 0; > +}
On Wed, 20 Sep 2017 15:42:38 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > > > Let's add indirect data addressing support for our virtual channel > > subsystem. This implementation does not bother with any kind of > > prefetching. We simply step through the IDAL on demand. > > > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 116 insertions(+), 1 deletion(-) > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index 2d37a9ddde..a3ce6d89b6 100644 > > --- a/hw/s390x/css.c > > +++ b/hw/s390x/css.c > > @@ -827,6 +827,121 @@ incr: > > return 0; > > } > > > > +/* returns values between 1 and bsz, where bsz is a power of 2 */ > > +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) > > +{ > > + return bsz - (cda & (bsz - 1)); > > +} > > + > > +static inline uint64_t ccw_ida_block_size(uint8_t flags) > > +{ > > + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { > > + return 1ULL << 12; > > + } > > + return 1ULL << 11; > > +} > > + > > +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, > > + bool idaw_fmt_2) > > +{ > > + union {uint64_t fmt2; uint32_t fmt1; } idaw; > > + int ret; > > + hwaddr idaw_addr; > > + > > + if (idaw_fmt_2) { > > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; > > + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > > + return -EINVAL; /* channel program check */ > > + } > > + ret = address_space_rw(&address_space_memory, idaw_addr, > Ahh, just got one question here: > Do we need to considerate endianess for idaw_addr? That is taken care of below. And the previous version worked on my laptop via tcg ;) > > > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > > + sizeof(idaw.fmt2), false); > > + cds->cda = be64_to_cpu(idaw.fmt2); > > + } else { > > + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > > + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > > + return -EINVAL; /* channel program check */ > > + } > > + ret = address_space_rw(&address_space_memory, idaw_addr, > > + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > > + sizeof(idaw.fmt1), false); > > + cds->cda = be64_to_cpu(idaw.fmt1); > Still need to check bit 0x80000000 here I think. Yes, I think this is 'must be zero' for format-1 idaws, and not covered by the ccw-format specific checks above. (Although the PoP can be a bit confusing with many similar terms...) > > > + } > > + ++(cds->at_idaw); > > + if (ret != MEMTX_OK) { > > + /* assume inaccessible address */ > > + return -EINVAL; /* channel program check */ > > + > Extra line. > > > + } > > + return 0; > > +} > > + > > +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, > > + CcwDataStreamOp op) > > +{ > > + uint64_t bsz = ccw_ida_block_size(cds->flags); > > + int ret = 0; > > + uint16_t cont_left, iter_len; > > + const bool idaw_fmt2 = cds->flags & CDS_F_C64; > > + bool ccw_fmt1 = cds->flags & CDS_F_FMT; > Use 'const bool' either? Although I doubt the value of using const here. > ;) Both being the same is still a good idea. > > > + > > + ret = cds_check_len(cds, len); > > + if (ret <= 0) { > > + return ret; > > + } > > [...] >
On 09/20/2017 10:11 AM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 20:27:45 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> Let's add indirect data addressing support for our virtual channel >> subsystem. This implementation does not bother with any kind of >> prefetching. We simply step through the IDAL on demand. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > My s-o-b is a bit preliminary ;) > >> --- >> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 116 insertions(+), 1 deletion(-) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 2d37a9ddde..a3ce6d89b6 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -827,6 +827,121 @@ incr: >> return 0; >> } >> >> +/* returns values between 1 and bsz, where bsz is a power of 2 */ >> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) >> +{ >> + return bsz - (cda & (bsz - 1)); >> +} >> + >> +static inline uint64_t ccw_ida_block_size(uint8_t flags) >> +{ >> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { >> + return 1ULL << 12; >> + } >> + return 1ULL << 11; >> +} >> + >> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, >> + bool idaw_fmt_2) >> +{ >> + union {uint64_t fmt2; uint32_t fmt1; } idaw; >> + int ret; >> + hwaddr idaw_addr; >> + >> + if (idaw_fmt_2) { >> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; >> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > > So, what is supposed to happen if the idaw_addr is not aligned > correctly _and_ is violating the limits? You are right this is obviously wrong. Should have been if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >> + return -EINVAL; /* channel program check */ >> + } >> + ret = address_space_rw(&address_space_memory, idaw_addr, >> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, >> + sizeof(idaw.fmt2), false); >> + cds->cda = be64_to_cpu(idaw.fmt2); >> + } else { >> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; >> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { Same here. >> + return -EINVAL; /* channel program check */ >> + } >> + ret = address_space_rw(&address_space_memory, idaw_addr, >> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, >> + sizeof(idaw.fmt1), false); >> + cds->cda = be64_to_cpu(idaw.fmt1); >> + } >> + ++(cds->at_idaw); >> + if (ret != MEMTX_OK) { >> + /* assume inaccessible address */ >> + return -EINVAL; /* channel program check */ >> + >> + } >> + return 0; >> +} >
On 09/20/2017 10:33 AM, Cornelia Huck wrote: > On Wed, 20 Sep 2017 15:42:38 +0800 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: >> >>> Let's add indirect data addressing support for our virtual channel >>> subsystem. This implementation does not bother with any kind of >>> prefetching. We simply step through the IDAL on demand. >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 116 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index 2d37a9ddde..a3ce6d89b6 100644 >>> --- a/hw/s390x/css.c >>> +++ b/hw/s390x/css.c >>> @@ -827,6 +827,121 @@ incr: >>> return 0; >>> } >>> >>> +/* returns values between 1 and bsz, where bsz is a power of 2 */ >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) >>> +{ >>> + return bsz - (cda & (bsz - 1)); >>> +} >>> + >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags) >>> +{ >>> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { >>> + return 1ULL << 12; >>> + } >>> + return 1ULL << 11; >>> +} >>> + >>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, >>> + bool idaw_fmt_2) >>> +{ >>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; >>> + int ret; >>> + hwaddr idaw_addr; >>> + >>> + if (idaw_fmt_2) { >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; >>> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { >>> + return -EINVAL; /* channel program check */ >>> + } >>> + ret = address_space_rw(&address_space_memory, idaw_addr, >> Ahh, just got one question here: >> Do we need to considerate endianess for idaw_addr? > > That is taken care of below. > > And the previous version worked on my laptop via tcg ;) Nod. > >> >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, >>> + sizeof(idaw.fmt2), false); >>> + cds->cda = be64_to_cpu(idaw.fmt2); >>> + } else { >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; >>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { >>> + return -EINVAL; /* channel program check */ >>> + } >>> + ret = address_space_rw(&address_space_memory, idaw_addr, >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, >>> + sizeof(idaw.fmt1), false); >>> + cds->cda = be64_to_cpu(idaw.fmt1); >> Still need to check bit 0x80000000 here I think. > > Yes, I think this is 'must be zero' for format-1 idaws, and not covered > by the ccw-format specific checks above. (Although the PoP can be a bit > confusing with many similar terms...) > It's taken care of in ccw_dstream_rw_ida before the actual access happens. Code looks like this: + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { + ret = -EINVAL; /* channel program check */ + goto err; + } The idea was to have it similar to the non-indirect case. >> >>> + } >>> + ++(cds->at_idaw); >>> + if (ret != MEMTX_OK) { >>> + /* assume inaccessible address */ >>> + return -EINVAL; /* channel program check */ >>> + >> Extra line. >> >>> + } >>> + return 0; >>> +} >>> + >>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, >>> + CcwDataStreamOp op) >>> +{ >>> + uint64_t bsz = ccw_ida_block_size(cds->flags); >>> + int ret = 0; >>> + uint16_t cont_left, iter_len; >>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64; >>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT; >> Use 'const bool' either? Although I doubt the value of using const here. >> ;) > > Both being the same is still a good idea. > Yeah. For which one should I go (with const or without)? >> >>> + >>> + ret = cds_check_len(cds, len); >>> + if (ret <= 0) { >>> + return ret; >>> + } >> >> [...] >> >
On Wed, 20 Sep 2017 13:13:01 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 09/20/2017 10:33 AM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 15:42:38 +0800 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > >>> + sizeof(idaw.fmt2), false); > >>> + cds->cda = be64_to_cpu(idaw.fmt2); > >>> + } else { > >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > >>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >>> + return -EINVAL; /* channel program check */ > >>> + } > >>> + ret = address_space_rw(&address_space_memory, idaw_addr, > >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > >>> + sizeof(idaw.fmt1), false); > >>> + cds->cda = be64_to_cpu(idaw.fmt1); > >> Still need to check bit 0x80000000 here I think. > > > > Yes, I think this is 'must be zero' for format-1 idaws, and not covered > > by the ccw-format specific checks above. (Although the PoP can be a bit > > confusing with many similar terms...) > > > > It's taken care of in ccw_dstream_rw_ida before the actual > access happens. Code looks like this: > + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { > + ret = -EINVAL; /* channel program check */ > + goto err; > + } > > The idea was to have it similar to the non-indirect case. <looks at patch again> Ah, I was simply looking for the wrong pattern. Looks correct. > >>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, > >>> + CcwDataStreamOp op) > >>> +{ > >>> + uint64_t bsz = ccw_ida_block_size(cds->flags); > >>> + int ret = 0; > >>> + uint16_t cont_left, iter_len; > >>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64; > >>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT; > >> Use 'const bool' either? Although I doubt the value of using const here. > >> ;) > > > > Both being the same is still a good idea. > > > > Yeah. For which one should I go (with const or without)? For the one you prefer :) (I'm not sure if the const adds value here.)
On 09/20/2017 01:18 PM, Cornelia Huck wrote: > On Wed, 20 Sep 2017 13:13:01 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 09/20/2017 10:33 AM, Cornelia Huck wrote: >>> On Wed, 20 Sep 2017 15:42:38 +0800 >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: >>> >>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, >>>>> + sizeof(idaw.fmt2), false); >>>>> + cds->cda = be64_to_cpu(idaw.fmt2); >>>>> + } else { >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; >>>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { >>>>> + return -EINVAL; /* channel program check */ >>>>> + } >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr, >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, >>>>> + sizeof(idaw.fmt1), false); >>>>> + cds->cda = be64_to_cpu(idaw.fmt1); >>>> Still need to check bit 0x80000000 here I think. >>> >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered >>> by the ccw-format specific checks above. (Although the PoP can be a bit >>> confusing with many similar terms...) >>> >> >> It's taken care of in ccw_dstream_rw_ida before the actual >> access happens. Code looks like this: >> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { >> + ret = -EINVAL; /* channel program check */ >> + goto err; >> + } >> >> The idea was to have it similar to the non-indirect case. > > <looks at patch again> > > Ah, I was simply looking for the wrong pattern. Looks correct. > > Thinking about this some more. Since in case of IDA we are guaranteed to never cross a block boundary with a single IDAW we won't ever cross block boundary. So we can do the check in ida_read_next_idaw by checking bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 local to ida_read_next_idaw and save one goto err. I think that would look a bit nicer than what I have here in v3. Agree? >>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, >>>>> + CcwDataStreamOp op) >>>>> +{ >>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags); >>>>> + int ret = 0; >>>>> + uint16_t cont_left, iter_len; >>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64; >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT; >>>> Use 'const bool' either? Although I doubt the value of using const here. >>>> ;) >>> >>> Both being the same is still a good idea. >>> >> >> Yeah. For which one should I go (with const or without)? > > For the one you prefer :) (I'm not sure if the const adds value here.) > I think we generally don't care about const-ness in such situations, so I think I won't use consts. I intend to fix the issues we have found and do a v4 tomorrow, unless somebody screams -- could do it today but I would like to give Dong Jia an opportunity to react. On the other hand waiting more that that will IMHO do us no favor either (I think of our storage/memory hierarchy). Regards, Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]: > > > On 09/20/2017 01:18 PM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 13:13:01 +0200 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> On 09/20/2017 10:33 AM, Cornelia Huck wrote: > >>> On Wed, 20 Sep 2017 15:42:38 +0800 > >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >>> > >>>> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > > > >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > >>>>> + sizeof(idaw.fmt2), false); > >>>>> + cds->cda = be64_to_cpu(idaw.fmt2); > >>>>> + } else { > >>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > >>>>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >>>>> + return -EINVAL; /* channel program check */ > >>>>> + } > >>>>> + ret = address_space_rw(&address_space_memory, idaw_addr, > >>>>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > >>>>> + sizeof(idaw.fmt1), false); > >>>>> + cds->cda = be64_to_cpu(idaw.fmt1); > >>>> Still need to check bit 0x80000000 here I think. > >>> > >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered > >>> by the ccw-format specific checks above. (Although the PoP can be a bit > >>> confusing with many similar terms...) > >>> > >> > >> It's taken care of in ccw_dstream_rw_ida before the actual > >> access happens. Code looks like this: > >> + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { > >> + ret = -EINVAL; /* channel program check */ > >> + goto err; > >> + } > >> > >> The idea was to have it similar to the non-indirect case. > > > > <looks at patch again> > > > > Ah, I was simply looking for the wrong pattern. Looks correct. > > > > > > Thinking about this some more. Since in case of IDA we are guaranteed > to never cross a block boundary with a single IDAW we won't ever cross > block boundary. So we can do the check in ida_read_next_idaw by checking > bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 > local to ida_read_next_idaw and save one goto err. I think that would > look a bit nicer than what I have here in v3. Agree? Agree. That would also do the check in the first place. Sounds better. > > >>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, > >>>>> + CcwDataStreamOp op) > >>>>> +{ > >>>>> + uint64_t bsz = ccw_ida_block_size(cds->flags); > >>>>> + int ret = 0; > >>>>> + uint16_t cont_left, iter_len; > >>>>> + const bool idaw_fmt2 = cds->flags & CDS_F_C64; > >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT; > >>>> Use 'const bool' either? Although I doubt the value of using const here. > >>>> ;) > >>> > >>> Both being the same is still a good idea. > >>> > >> > >> Yeah. For which one should I go (with const or without)? > > > > For the one you prefer :) (I'm not sure if the const adds value here.) > > > > I think we generally don't care about const-ness in such situations, > so I think I won't use consts. > > I intend to fix the issues we have found and do a v4 tomorrow, unless > somebody screams -- could do it today but I would like to give Dong > Jia an opportunity to react. Thanks. I'm coming. :) > On the other hand waiting more that that will IMHO do us no favor > either (I think of our storage/memory hierarchy). > > Regards, > Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 13:13:01 +0200]: > > > On 09/20/2017 10:33 AM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 15:42:38 +0800 > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > > > >> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:45 +0200]: > >> > >>> Let's add indirect data addressing support for our virtual channel > >>> subsystem. This implementation does not bother with any kind of > >>> prefetching. We simply step through the IDAL on demand. > >>> > >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>> --- > >>> hw/s390x/css.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 116 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >>> index 2d37a9ddde..a3ce6d89b6 100644 > >>> --- a/hw/s390x/css.c > >>> +++ b/hw/s390x/css.c > >>> @@ -827,6 +827,121 @@ incr: > >>> return 0; > >>> } > >>> > >>> +/* returns values between 1 and bsz, where bsz is a power of 2 */ > >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) > >>> +{ > >>> + return bsz - (cda & (bsz - 1)); > >>> +} > >>> + > >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags) > >>> +{ > >>> + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { > >>> + return 1ULL << 12; > >>> + } > >>> + return 1ULL << 11; > >>> +} > >>> + > >>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, > >>> + bool idaw_fmt_2) > >>> +{ > >>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; > >>> + int ret; > >>> + hwaddr idaw_addr; > >>> + > >>> + if (idaw_fmt_2) { > >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; > >>> + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >>> + return -EINVAL; /* channel program check */ > >>> + } > >>> + ret = address_space_rw(&address_space_memory, idaw_addr, > >> Ahh, just got one question here: > >> Do we need to considerate endianess for idaw_addr? > > > > That is taken care of below. > > > > And the previous version worked on my laptop via tcg ;) > > Nod. My fault! I was thinking of the idaw_addr itself, not the content of it. Now I realized that, since we already converted (cds->cda_orig) in copy_ccw_from_guest(), there is no need to convert (idaw_addr + idaw_size * idaw_index) anymore. Please ingnore my noise. ;P > > > > >> > >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, > >>> + sizeof(idaw.fmt2), false); > >>> + cds->cda = be64_to_cpu(idaw.fmt2); > >>> + } else { > >>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; > >>> + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { > >>> + return -EINVAL; /* channel program check */ > >>> + } > >>> + ret = address_space_rw(&address_space_memory, idaw_addr, > >>> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, > >>> + sizeof(idaw.fmt1), false); > >>> + cds->cda = be64_to_cpu(idaw.fmt1); [...]
On Thu, 21 Sep 2017 08:50:36 +0800 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-20 18:46:57 +0200]: > > Thinking about this some more. Since in case of IDA we are guaranteed > > to never cross a block boundary with a single IDAW we won't ever cross > > block boundary. So we can do the check in ida_read_next_idaw by checking > > bit 0x80000000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1 > > local to ida_read_next_idaw and save one goto err. I think that would > > look a bit nicer than what I have here in v3. Agree? > Agree. That would also do the check in the first place. Sounds better. Can't argue with nicer code, either :) Looking forward to the next version.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 2d37a9ddde..a3ce6d89b6 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -827,6 +827,121 @@ incr: return 0; } +/* returns values between 1 and bsz, where bsz is a power of 2 */ +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz) +{ + return bsz - (cda & (bsz - 1)); +} + +static inline uint64_t ccw_ida_block_size(uint8_t flags) +{ + if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) { + return 1ULL << 12; + } + return 1ULL << 11; +} + +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1, + bool idaw_fmt_2) +{ + union {uint64_t fmt2; uint32_t fmt1; } idaw; + int ret; + hwaddr idaw_addr; + + if (idaw_fmt_2) { + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; + if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { + return -EINVAL; /* channel program check */ + } + ret = address_space_rw(&address_space_memory, idaw_addr, + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2, + sizeof(idaw.fmt2), false); + cds->cda = be64_to_cpu(idaw.fmt2); + } else { + idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw; + if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) { + return -EINVAL; /* channel program check */ + } + ret = address_space_rw(&address_space_memory, idaw_addr, + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1, + sizeof(idaw.fmt1), false); + cds->cda = be64_to_cpu(idaw.fmt1); + } + ++(cds->at_idaw); + if (ret != MEMTX_OK) { + /* assume inaccessible address */ + return -EINVAL; /* channel program check */ + + } + return 0; +} + +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len, + CcwDataStreamOp op) +{ + uint64_t bsz = ccw_ida_block_size(cds->flags); + int ret = 0; + uint16_t cont_left, iter_len; + const bool idaw_fmt2 = cds->flags & CDS_F_C64; + bool ccw_fmt1 = cds->flags & CDS_F_FMT; + + ret = cds_check_len(cds, len); + if (ret <= 0) { + return ret; + } + if (!cds->at_idaw) { + /* read first idaw */ + ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2); + if (ret) { + goto err; + } + cont_left = ida_continuous_left(cds->cda, bsz); + } else { + cont_left = ida_continuous_left(cds->cda, bsz); + if (cont_left == bsz) { + ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2); + if (ret) { + goto err; + } + if (cds->cda & (bsz - 1)) { + ret = -EINVAL; /* channel program check */ + goto err; + } + } + } + do { + iter_len = MIN(len, cont_left); + if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) { + ret = -EINVAL; /* channel program check */ + goto err; + } + if (op != CDS_OP_A) { + ret = address_space_rw(&address_space_memory, cds->cda, + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op); + if (ret != MEMTX_OK) { + /* assume inaccessible address */ + ret = -EINVAL; /* channel program check */ + goto err; + } + } + cds->at_byte += iter_len; + cds->cda += iter_len; + len -= iter_len; + if (!len) { + break; + } + ret = ida_read_next_idaw(cds, ccw_fmt1, idaw_fmt2); + if (ret) { + goto err; + } + cont_left = bsz; + } while (true); + return ret; +err: + cds->flags |= CDS_F_STREAM_BROKEN; + return ret; +} + void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) { /* @@ -845,7 +960,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb) if (!(cds->flags & CDS_F_IDA)) { cds->op_handler = ccw_dstream_rw_noflags; } else { - assert(false); + cds->op_handler = ccw_dstream_rw_ida; } }