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.
In any case, s/retuns/returns/
>
> >
> >> +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?
