[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 12:57:17 +0200 |
On Tue, 19 Sep 2017 12:36:33 +0200
Halil Pasic <address@hidden> wrote:
> On 09/19/2017 11:48 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 13:50:05 +0800
> > Dong Jia Shi <address@hidden> wrote:
> >
> >> * Halil Pasic <address@hidden> [2017-09-13 13:50:29 +0200]:
> >>
> >>> Let's add indirect data addressing support for our virtual channel
> >>> subsystem. This implementation does no bother with any kind of
> >>> prefetching. We simply step trough the IDAL on demand.
> >>>
> >>> Signed-off-by: Halil Pasic <address@hidden>
> >>> ---
> >>> hw/s390x/css.c | 109
> >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 108 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index 6b0cd8861b..e34b2af4eb 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -819,6 +819,113 @@ incr:
> >>> return 0;
> >>> }
> >>>
> >>> +/* returns values between 1 and bsz, where bs 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)
> >>> +{
> >>> + return 1ULL << (((flags ^ CDS_F_C64) & (CDS_F_C64 | CDS_F_I2K)) ? 11
> >>> : 12);
> >> If CDS_F_C64 is set, (flags ^ CDS_F_C64) will be 0, so (1ULL << 11) will
> >> be the result regardless the I2K flag? The logic seems wrong.
>
> No. If CDS_F_C64 is set then the outcome depends on the fact if
> CDS_F_I2K is set or not.
> (flags & CDS_F_IK) => ((flags ^ CDS_F_C64) & CDS_F_IK)
> "(flags ^ CDS_F_C64) will be 0" is wrong. flags ^ CDS_F_C64
> just flips the CDS_F_C64.
>
> OTOH if the CDS_F_C64 was not set we have the corresponding
> bit set in flags ^ CDS_F_C64 so then the CDS_F_I2K bit does
> not matter: we have 1ULL << 11.
>
> In my reading the logic is good.
So I'll just leave it...
>
> >
> > I've stared at that condition now for a bit, but all it managed was to
> > get me more confused... probably just need a break.
> >
> >>
> >> I2K is meaningful only when C64 is 1, otherwise it is ignored. The logic
> >> here should be:
> >> if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> >> return 1ULL << 12;
> >> }
> >> return 1ULL << 11;
> >
> > But I do think your version is more readable...
> >
>
> I won't argue with this.
...and we could change that in a patch on top to avoid future confusion.
>
> >>
> >>> +}
> >>> +
> >>> +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.
>
> >>
> >>> + 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;
> >>> +
> >>> + ret = cds_check_len(cds, len);
> >>> + if (ret <= 0) {
> >>> + return ret;
> >>> + }
> >>> + if (!cds->at_idaw) {
> >>> + /* read first idaw */
> >>> + ret = ida_read_next_idaw(cds);
> >>> + 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);
> >>> + if (ret) {
> >>> + goto err;
> >>> + }
> >>> + if (cds->cda & (bsz - 1)) {
> >> Could move this check into ida_read_next_idaw?
> >
> > I'd like to avoid further code movement...
> >
>
> The first idaw is special. I don't think moving is possible.
So, the code is correct and I'll just leave it like that.
>
> >>
> >>> + ret = -EINVAL; /* channel program check */
> >>> + goto err;
> >>> + }
> >>> + }
> >>> + }
> >>> + do {
> >>> + iter_len = MIN(len, cont_left);
> >>> + if (op != CDS_OP_A) {
> >>> + ret = address_space_rw(&address_space_memory, cds->cda,
> >>> + MEMTXATTRS_UNSPECIFIED, buff,
> >>> iter_len, op);
> >> Ahh, now I recall that explictly defining CDS_OP_R to 0 and CDS_OP_W to
> >> 1 in 'struct CcwDataStreamOp' do have a meaning. Does it make sense to
> >> make it more obvious by adding some comment there?
> >
> > Would you have a good text for that?
> >
>
> I'm fine with clarifications.
Let's do it as a patch on top.
>
> >>
> >>> + 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);
> >>> + 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)
> >>> {
> >>> /*
> >>> @@ -835,7 +942,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;
> >>> }
> >>> }
> >>>
> >>> --
> >>> 2.13.5
> >>>
> >>
> >> Generally, the logic looks fine to me.
> >>
> >
> > It did pass Halil's test; but that can only test fmt-2 + 4k blocks, as
> > this is what the kernel infrastructure provides.
>
> Nod.
>
> >
> > Halil, do you have some more comments?
> >
>
> Just a question. Do I have to respin?
I don't think so. If you could confirm the check for format-1, I'll
just fixup that one and get the queued patches out of the door.
We can do more changes on top; it's not like I don't have more patches
waiting...
- [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 <=
- 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, 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