[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: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA |
Date: |
Tue, 19 Sep 2017 12:36:33 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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.
>
> 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.
>>
>>> +}
>>> +
>>> +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.
>>> + 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.
>>
>>> + 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.
>>
>>> + 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.
>>
>>> + 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?
Halil
- [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 <=
- 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, 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