[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA |
Date: |
Tue, 19 Sep 2017 16:34:11 +0200 |
On Tue, 19 Sep 2017 14:32:33 +0200
Halil Pasic <address@hidden> wrote:
> On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 14:04:03 +0200
> > Halil Pasic <address@hidden> 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.
- [Qemu-devel] [PATCH v2 0/4] add CCW indirect data access support, Halil Pasic, 2017/09/13
- [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/13
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Cornelia Huck, 2017/09/14
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Dong Jia Shi, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Cornelia Huck, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Cornelia Huck, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Cornelia Huck, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA,
Cornelia Huck <=
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Dong Jia Shi, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Dong Jia Shi, 2017/09/20
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Pierre Morel, 2017/09/19
- Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA, Halil Pasic, 2017/09/19
[Qemu-devel] [PATCH v2 2/4] s390x/css: use ccw data stream, Halil Pasic, 2017/09/13
[Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream, Halil Pasic, 2017/09/13