| Message ID | 20130205102700.1f745c36@gondolin |
|---|---|
| State | New |
| Headers | show |
On Tue, 5 Feb 2013 10:27:00 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Mon, 04 Feb 2013 22:57:43 +0100 > Stefan Weil <sw@weilnetz.de> wrote: > > > Am 04.02.2013 22:23, schrieb Anthony Liguori: > > > As best I can tell, this is a false positive. > > > > > > [aliguori@ccnode4 qemu-s390]$ make > > > CC s390x-softmmu/target-s390x/helper.o > > > /home/aliguori/git/qemu/target-s390x/helper.c: In function ‘do_interrupt’: > > > /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > /home/aliguori/git/qemu/target-s390x/helper.c:620:20: note: ‘addr’ was declared here > > > /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘mask’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > /home/aliguori/git/qemu/target-s390x/helper.c:620:14: note: ‘mask’ was declared here > > > cc1: all warnings being treated as errors > > > make[1]: *** [target-s390x/helper.o] Error 1 > > > make: *** [subdir-s390x-softmmu] Error 2 > > > > > > > Yes, this is a false positive. A better compiler will complain when your > > patch was applied because addr, mask are assigned values which are > > never used... > > > > Would it be possible to completely eliminate variable "found" and > > move the DPRINTF, load_psw statements into the for loop (just before > > the break statement)? > > We could move the lpsw. However, this made me notice another problem: > We stop scanning subsequent iscs if we found an interrupt to inject... > > This is not a problem for current virtio-ccw based Linux guests since > they never use anything else than isc 3, but we'll probably want the > following patch. > > From 8b2f40e8eaac16b55a72ab1e36a4c5de0b016495 Mon Sep 17 00:00:00 2001 > From: Cornelia Huck <cornelia.huck@de.ibm.com> > Date: Tue, 5 Feb 2013 10:14:49 +0100 > Subject: [PATCH] s390: Keep I/O interrupts enabled for all iscs. > > do_io_interrupt() would stop scanning further iscs if it found > an I/O interrupt it could inject. This might cause the pending > interrupt indication for I/O interrupts to be reset although there > might be queued I/O interrupts for subsequent iscs. > > Fix this by reordering the logic: Inject the I/O interrupt immediately > and continue searching all iscs for queued interrupts. > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> Any opinions on this? I have another fix touching this code and I'd like to base that patch on top of this one. > --- > target-s390x/helper.c | 40 +++++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 19 deletions(-) > > diff --git a/target-s390x/helper.c b/target-s390x/helper.c > index 043feb2..9f9088b 100644 > --- a/target-s390x/helper.c > +++ b/target-s390x/helper.c > @@ -617,7 +617,6 @@ static void do_ext_interrupt(CPUS390XState *env) > > static void do_io_interrupt(CPUS390XState *env) > { > - uint64_t mask = 0, addr = 0; > LowCore *lowcore; > IOIntQueue *q; > uint8_t isc; > @@ -642,36 +641,39 @@ static void do_io_interrupt(CPUS390XState *env) > disable = 0; > continue; > } > - found = 1; > - lowcore = cpu_map_lowcore(env); > + if (!found) { > + uint64_t mask, addr; > > - lowcore->subchannel_id = cpu_to_be16(q->id); > - lowcore->subchannel_nr = cpu_to_be16(q->nr); > - lowcore->io_int_parm = cpu_to_be32(q->parm); > - lowcore->io_int_word = cpu_to_be32(q->word); > - lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env)); > - lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr); > - mask = be64_to_cpu(lowcore->io_new_psw.mask); > - addr = be64_to_cpu(lowcore->io_new_psw.addr); > + found = 1; > + lowcore = cpu_map_lowcore(env); > > - cpu_unmap_lowcore(lowcore); > + lowcore->subchannel_id = cpu_to_be16(q->id); > + lowcore->subchannel_nr = cpu_to_be16(q->nr); > + lowcore->io_int_parm = cpu_to_be32(q->parm); > + lowcore->io_int_word = cpu_to_be32(q->word); > + lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env)); > + lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr); > + mask = be64_to_cpu(lowcore->io_new_psw.mask); > + addr = be64_to_cpu(lowcore->io_new_psw.addr); > > - env->io_index[isc]--; > + cpu_unmap_lowcore(lowcore); > + > + env->io_index[isc]--; > + > + DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, > + env->psw.mask, env->psw.addr); > + load_psw(env, mask, addr); > + } > if (env->io_index[isc] >= 0) { > disable = 0; > } > - break; > + continue; > } > > if (disable) { > env->pending_int &= ~INTERRUPT_IO; > } > > - if (found) { > - DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, > - env->psw.mask, env->psw.addr); > - load_psw(env, mask, addr); > - } > } > > static void do_mchk_interrupt(CPUS390XState *env)
diff --git a/target-s390x/helper.c b/target-s390x/helper.c index 043feb2..9f9088b 100644 --- a/target-s390x/helper.c +++ b/target-s390x/helper.c @@ -617,7 +617,6 @@ static void do_ext_interrupt(CPUS390XState *env) static void do_io_interrupt(CPUS390XState *env) { - uint64_t mask = 0, addr = 0; LowCore *lowcore; IOIntQueue *q; uint8_t isc; @@ -642,36 +641,39 @@ static void do_io_interrupt(CPUS390XState *env) disable = 0; continue; } - found = 1; - lowcore = cpu_map_lowcore(env); + if (!found) { + uint64_t mask, addr; - lowcore->subchannel_id = cpu_to_be16(q->id); - lowcore->subchannel_nr = cpu_to_be16(q->nr); - lowcore->io_int_parm = cpu_to_be32(q->parm); - lowcore->io_int_word = cpu_to_be32(q->word); - lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env)); - lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr); - mask = be64_to_cpu(lowcore->io_new_psw.mask); - addr = be64_to_cpu(lowcore->io_new_psw.addr); + found = 1; + lowcore = cpu_map_lowcore(env); - cpu_unmap_lowcore(lowcore); + lowcore->subchannel_id = cpu_to_be16(q->id); + lowcore->subchannel_nr = cpu_to_be16(q->nr); + lowcore->io_int_parm = cpu_to_be32(q->parm); + lowcore->io_int_word = cpu_to_be32(q->word); + lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env)); + lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr); + mask = be64_to_cpu(lowcore->io_new_psw.mask); + addr = be64_to_cpu(lowcore->io_new_psw.addr); - env->io_index[isc]--; + cpu_unmap_lowcore(lowcore); + + env->io_index[isc]--; + + DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, + env->psw.mask, env->psw.addr); + load_psw(env, mask, addr); + } if (env->io_index[isc] >= 0) { disable = 0; } - break; + continue; } if (disable) { env->pending_int &= ~INTERRUPT_IO; } - if (found) { - DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, - env->psw.mask, env->psw.addr); - load_psw(env, mask, addr); - } } static void do_mchk_interrupt(CPUS390XState *env)