On 09/13/2017 11:58 AM, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 20:08:16 +0200
> Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
>
>> On 09/06/2017 03:10 PM, Cornelia Huck wrote:
>>> On Tue, 5 Sep 2017 13:16:44 +0200
>>> Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
>>>
>>>> Let's add indirect data addressing support for our virtual channel
>>>> subsystem. This implementation does no prefetching, but steps
>>>> trough the idal as we go.
>>>>
>>>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
>>>> ---
>>>> hw/s390x/css.c | 110
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 109 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index c1bc9944e6..9d8f9fd7ea 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -819,6 +819,114 @@ incr:
>>>> return 0;
>>>> }
>>>>
>>>> +/* retuns values between 1 and bs, where bs is a power of 2 */
>>>
>>> 'bs' is 'block size'?
>>
>> Yes.
>
> The first thing that popped up in my head was something else, that's
> why I asked :) Maybe bsz would be better.
>
Will change to bsz.
> In any case, s/retuns/returns/
>
I think I've already fixed that :).
>>
>>>
>>>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bs)
>>>> +{
>>>> + return bs - (cda & (bs - 1));
>>>> +}
>
>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>>> + CcwDataStreamOp op)
>>>> +{
>>>> + uint64_t bs = 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, bs);
>>>> + } else {
>>>> + cont_left = ida_continuous_left(cds->cda, bs);
>>>> + if (cont_left == bs) {
>>>> + ret = ida_read_next_idaw(cds);
>>>> + if (ret) {
>>>> + goto err;
>>>> + }
>>>> + if (cds->cda & (bs - 1)) {
>>>> + ret = -EINVAL; /* chanel program check */
>>>> + goto err;
>>>> + }
>>>> + }
>>>> + }
>>>> +do_iter_len:
>>>> + iter_len = MIN(len, cont_left);
>>>> + if (op == CDS_OP_A) {
>>>> + goto incr;
>>>> + }
>>>> + ret = address_space_rw(&address_space_memory, cds->cda,
>>>> + MEMTXATTRS_UNSPECIFIED, buff, iter_len, op);
>>>> + if (ret != MEMTX_OK) {
>>>> + /* assume inaccessible address */
>>>> + ret = -EINVAL; /* chanel program check */
>>>> + goto err;
>>>> + }
>>>> +incr:
>>>> + cds->at_byte += iter_len;
>>>> + cds->cda += iter_len;
>>>> + len -= iter_len;
>>>> + if (len) {
>>>> + ret = ida_read_next_idaw(cds);
>>>> + if (ret) {
>>>> + goto err;
>>>> + }
>>>> + cont_left = bs;
>>>> + goto do_iter_len;
>>>> + }
>>>> + return ret;
>>>> +err:
>>>> + cds->flags |= CDS_F_STREAM_BROKEN;
>>>> + return ret;
>>>> +}
>>>
>>> I'm not so happy about the many gotos (excepting the err ones). Any
>>> chance to get around these?
>>>
>> Certainly possible:
>>
>> static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
>>
>> CcwDataStreamOp op)
>>
>> {
>>
>> uint64_t bs = 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, bs);
>>
>> } else {
>>
>> cont_left = ida_continuous_left(cds->cda, bs);
>>
>> if (cont_left == bs) {
>>
>> ret = ida_read_next_idaw(cds);
>>
>> if (ret) {
>>
>> goto err;
>>
>> }
>>
>> if (cds->cda & (bs - 1)) {
>>
>> 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);
>> 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 = bs;
>>
>> } while (true);
>>
>> return ret;
>>
>> err:
>>
>> cds->flags |= CDS_F_STREAM_BROKEN;
>>
>> return ret;
>>
>> }
>>
>> My idea was decrease indentation level and at the same time make
>> labels tell us something about the purpose of code pieces. Which
>> one do you prefer?
>
> I do not really like either much :( Any chance to make this better via
> some kind of helper functions?
>
I already use helper functions (ida_read_next_idaw, ida_continuous_left,
ccw_ida_block_size, cds_check_len). The function is flow control and
error handling heavy. I have no further ideas for helper functions,
or other simplifications.
IMHO if there are no non-aesthetics issues we can defer the non-trivial
aesthetics issues until somebody/anybody has an idea and the time to
sort them out.
I think, in absence of further complaints or concrete ideas I will go
with the more structured variant (second one) for v2. I would like
to post a v2 soon to avoid juggling to much stuff in head.
Regards,
Halil
