From: Cornelia Huck <cohuck@redhat.com>
To: Halil Pasic <pasic@linux.vnet.ibm.com>
Cc: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>,
Pierre Morel <pmorel@linux.vnet.ibm.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA
Date: Wed, 13 Sep 2017 11:58:08 +0200 [thread overview]
Message-ID: <20170913115808.4a8d263b.cohuck@redhat.com> (raw)
In-Reply-To: <b4351b30-455a-7247-30f2-9a2b2c21ae17@linux.vnet.ibm.com>
On Mon, 11 Sep 2017 20:08:16 +0200
Halil Pasic <pasic@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 <pasic@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 <pasic@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?
next prev parent reply other threads:[~2017-09-13 9:58 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-05 11:16 [Qemu-devel] [PATCH 0/5] add CCW indirect data access support Halil Pasic
2017-09-05 11:16 ` [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream Halil Pasic
2017-09-06 12:18 ` Cornelia Huck
2017-09-06 12:40 ` Halil Pasic
2017-09-06 12:51 ` Cornelia Huck
2017-09-11 16:36 ` Halil Pasic
2017-09-13 9:53 ` Cornelia Huck
2017-09-13 11:35 ` Halil Pasic
2017-09-05 11:16 ` [Qemu-devel] [PATCH 2/5] s390x/css: use ccw " Halil Pasic
2017-09-06 12:32 ` Cornelia Huck
2017-09-06 12:42 ` Halil Pasic
2017-09-21 9:33 ` Pierre Morel
2017-09-21 9:36 ` Pierre Morel
2017-09-21 9:45 ` Cornelia Huck
2017-09-05 11:16 ` [Qemu-devel] [PATCH 3/5] virtio-ccw: " Halil Pasic
2017-09-06 12:42 ` Cornelia Huck
2017-09-06 12:49 ` Halil Pasic
2017-09-06 12:54 ` Cornelia Huck
2017-09-11 18:14 ` Halil Pasic
2017-09-13 9:58 ` Cornelia Huck
2017-09-13 11:36 ` Halil Pasic
2017-09-05 11:16 ` [Qemu-devel] [PATCH 4/5] s390x/css: support ccw IDA Halil Pasic
2017-09-06 13:10 ` Cornelia Huck
2017-09-11 18:08 ` Halil Pasic
2017-09-13 9:58 ` Cornelia Huck [this message]
2017-09-13 10:31 ` Halil Pasic
2017-09-13 10:50 ` Cornelia Huck
2017-09-05 11:16 ` [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device Halil Pasic
2017-09-06 13:18 ` Cornelia Huck
2017-09-06 14:24 ` Halil Pasic
2017-09-06 15:20 ` Cornelia Huck
2017-09-06 16:16 ` Halil Pasic
2017-09-07 8:06 ` Cornelia Huck
2017-09-07 9:10 ` Janosch Frank
2017-09-07 12:24 ` Cornelia Huck
2017-09-07 7:31 ` Dong Jia Shi
2017-09-07 8:08 ` Cornelia Huck
2017-09-07 10:21 ` Halil Pasic
2017-09-07 10:52 ` Cornelia Huck
2017-09-08 2:01 ` Dong Jia Shi
2017-09-08 10:28 ` Halil Pasic
2017-09-19 6:03 ` Dong Jia Shi
2017-09-21 8:45 ` Dong Jia Shi
2017-09-21 8:54 ` Cornelia Huck
2017-09-26 7:48 ` Dong Jia Shi
2017-09-27 7:11 ` Dong Jia Shi
2017-09-08 10:45 ` [Qemu-devel] [PATCH 0/5] add CCW indirect data access support Halil Pasic
2017-09-08 10:49 ` Cornelia Huck
2017-09-08 11:03 ` Halil Pasic
2017-09-08 11:19 ` Cornelia Huck
2017-09-08 11:43 ` Halil Pasic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170913115808.4a8d263b.cohuck@redhat.com \
--to=cohuck@redhat.com \
--cc=bjsdjshi@linux.vnet.ibm.com \
--cc=pasic@linux.vnet.ibm.com \
--cc=pmorel@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).