* Halil Pasic <pa...@linux.vnet.ibm.com> [2017-09-19 14:04:03 +0200]:
I have no problem with the rest parts of the discussion in this thread.
>
>
> 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.
> >
My fault... This is the address of an IDAW, not the content (data
address) in an IDAW. So what Halil pointed out is the right direction to
go I think.
I will review in the thread of the new version (v3).
>
> 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.
Right. This is what I actually wanted to say.
>
> 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".
>
> How shall we proceed?
>
> Halil
>
> >>>>
> >>>>> + 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;
> >>>>> +}
--
Dong Jia Shi
