On 09/20/2017 10:11 AM, Cornelia Huck wrote: > On Tue, 19 Sep 2017 20:27:45 +0200 > Halil Pasic <pa...@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 <pa...@linux.vnet.ibm.com> >> Signed-off-by: Cornelia Huck <coh...@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; >> +} >