On Tue, 19 Sep 2017 14:32:33 +0200 Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
> On 09/19/2017 02:23 PM, Cornelia Huck wrote: > > On Tue, 19 Sep 2017 14:04:03 +0200 > > Halil Pasic <pa...@linux.vnet.ibm.com> wrote: > > > >> On 09/19/2017 12:57 PM, Cornelia Huck wrote: > >>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds) > >>>>>>> +{ > >>>>>>> + union {uint64_t fmt2; uint32_t fmt1; } idaw; > >>>>>> ^ > >>>>>> Nit. > >>>>>> > >>>> Maybe checkpatch wanted it this way. My memories are blurry. > >>> > >>> I'd just leave it like that, tbh. > >>> > >>>>>>> + bool is_fmt2 = cds->flags & CDS_F_C64; > >>>>>>> + int ret; > >>>>>>> + hwaddr idaw_addr; > >>>>>>> + > >>>>>>> + if (is_fmt2) { > >>>>>>> + idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw; > >>>>>>> + if (idaw_addr & 0x07) { > >>>>>>> + 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) { > >>>>>> ?: > >>>>>> (idaw_addr & 0x80000003) > >>>>> Yes. > >>>>> > >>>> I will double check this. Does not seem unreasonable but > >>>> double-checking is better. > >>> Please let me know. I think the architecture says that the bit must be > >>> zero, and that we may (...) generate a channel program check. > >>> > >> > >> Not exactly. The more significant bits part of the check > >> depend on the ccw format. This needs to be done for both > >> idaw formats. I would need to introduce a new flag, or > >> access the SubchDev to do this properly. > >> > >> Architecturally we also need to check the data addresses > >> from which we read so we have nothing bigger than > >> (1 << 31) - 1 if we are working with format-1 idaws. > >> > >> I also think we did not take proper care of proper > >> maximum data address checks prior CwwDataStream which also > >> depend on the ccw format (in absence of IDAW or MIDAW). > >> > >> The ccw format dependent maximum address checks are (1 << 24) - 1 > >> and (1 << 31) - 1 respectively for format-0 and format-1 (on > >> the first indirection level that is for non-IDA for the data, > >> and for (M)IDA for the (M)IDAWs). > >> > >> Reference: > >> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and > >> "Invalid Data Address". > > > > Sigh, when are things ever easy :( > > > > I'll need some time to review this. But more urgently, I need a break. > > > >> > >> How shall we proceed? > > > > Given the discussion about this patch in particular, I'll probably > > dequeue it and send it with the next batch of patches. We can also > > include the other improvements, then. Not sure whether I should just > > dequeue this one or the whole series (I really want to get the rest of > > the patches out...) > > > > More after the break :) > > > > I think I can fix this pretty fast, and fixing this later is > an option too in my opinion as the patch is consistent with our > prior art (we ignored this maximum address checking requirement, > and we keep ignoring it). > > I would prefer not dequeue-ing the whole series because a > 3270 fix of mine (which just made the internal review) depends > on this. IMHO, it's not like the series is fundamentally broken, > not ain't an improvement at all. It is not perfect, but it's > IMHO certainly an improvement over what we have. The issue is not that the series is problematic, but that I need to put a stop on changes at some point in time and just flush my queue - otherwise this is not workable. Therefore, I dequeued the series for now. I'll probably do another pull request in the next weeks anyway - so there's unlikely to be a long delay, and we can hash out everything without pressure. It would be best to just think this through; then you can send a v3 with the feedback addressed and your 3270 patch on top.