| Message ID | 20181003193931.18096-9-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg: Reorg 128-bit atomic operations | expand |
On 03/10/2018 21:39, Richard Henderson wrote: > When op raises an exception, it may not have initialized the output > temps that would be written back by wout or cout. > > Cc: qemu-s390x@nongnu.org > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/s390x/translate.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/target/s390x/translate.c b/target/s390x/translate.c > index 7363aabf3a..7fad3ad8e9 100644 > --- a/target/s390x/translate.c > +++ b/target/s390x/translate.c > @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) > if (insn->help_op) { > ret = insn->help_op(s, &o); > } > - if (insn->help_wout) { > - insn->help_wout(s, &f, &o); > - } > - if (insn->help_cout) { > - insn->help_cout(s, &o); > + if (ret != DISAS_NORETURN) { > + if (insn->help_wout) { > + insn->help_wout(s, &f, &o); > + } > + if (insn->help_cout) { > + insn->help_cout(s, &o); > + } > } > > /* Free any temporaries created by the helpers. */ > What about things like LPSW/LPWSE ? They certainly don't imply that we had an exception. (these two don't use wout/cout, so it is still fine, but I would prefer a comment somewhere because otherwise it is really easy to miss that DISAS_NORETURN makes us skip these handlers) Apart from that Reviewed-by: David Hildenbrand <david@redhat.com> -- Thanks, David / dhildenb
On 10/11/18 1:06 AM, David Hildenbrand wrote: > On 03/10/2018 21:39, Richard Henderson wrote: >> When op raises an exception, it may not have initialized the output >> temps that would be written back by wout or cout. >> >> Cc: qemu-s390x@nongnu.org >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/s390x/translate.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/target/s390x/translate.c b/target/s390x/translate.c >> index 7363aabf3a..7fad3ad8e9 100644 >> --- a/target/s390x/translate.c >> +++ b/target/s390x/translate.c >> @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) >> if (insn->help_op) { >> ret = insn->help_op(s, &o); >> } >> - if (insn->help_wout) { >> - insn->help_wout(s, &f, &o); >> - } >> - if (insn->help_cout) { >> - insn->help_cout(s, &o); >> + if (ret != DISAS_NORETURN) { >> + if (insn->help_wout) { >> + insn->help_wout(s, &f, &o); >> + } >> + if (insn->help_cout) { >> + insn->help_cout(s, &o); >> + } >> } >> >> /* Free any temporaries created by the helpers. */ >> > > What about things like LPSW/LPWSE ? They certainly don't imply that we > had an exception. Exception in the tcg sense, not the guest architectural sense, in that we call cpu_loop_exit from the helper, which performs a longjmp. (Incidentally, there's no reason to do that for load_psw -- we could just exit the tb normally.) > (these two don't use wout/cout, so it is still fine, but I would prefer > a comment somewhere because otherwise it is really easy to miss that > DISAS_NORETURN makes us skip these handlers) Where would you like me to place that comment? In the DisasInsn definition? r~
On 11/10/2018 18:47, Richard Henderson wrote: > On 10/11/18 1:06 AM, David Hildenbrand wrote: >> On 03/10/2018 21:39, Richard Henderson wrote: >>> When op raises an exception, it may not have initialized the output >>> temps that would be written back by wout or cout. >>> >>> Cc: qemu-s390x@nongnu.org >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/s390x/translate.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c >>> index 7363aabf3a..7fad3ad8e9 100644 >>> --- a/target/s390x/translate.c >>> +++ b/target/s390x/translate.c >>> @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) >>> if (insn->help_op) { >>> ret = insn->help_op(s, &o); >>> } >>> - if (insn->help_wout) { >>> - insn->help_wout(s, &f, &o); >>> - } >>> - if (insn->help_cout) { >>> - insn->help_cout(s, &o); >>> + if (ret != DISAS_NORETURN) { >>> + if (insn->help_wout) { >>> + insn->help_wout(s, &f, &o); >>> + } >>> + if (insn->help_cout) { >>> + insn->help_cout(s, &o); >>> + } >>> } >>> >>> /* Free any temporaries created by the helpers. */ >>> >> >> What about things like LPSW/LPWSE ? They certainly don't imply that we >> had an exception. > > Exception in the tcg sense, not the guest architectural sense, in that we call > cpu_loop_exit from the helper, which performs a longjmp. (Incidentally, > there's no reason to do that for load_psw -- we could just exit the tb normally.) > >> (these two don't use wout/cout, so it is still fine, but I would prefer >> a comment somewhere because otherwise it is really easy to miss that >> DISAS_NORETURN makes us skip these handlers) > > Where would you like me to place that comment? In the DisasInsn definition? That probably makes sense! > > > r~ > -- Thanks, David / dhildenb
diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 7363aabf3a..7fad3ad8e9 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) if (insn->help_op) { ret = insn->help_op(s, &o); } - if (insn->help_wout) { - insn->help_wout(s, &f, &o); - } - if (insn->help_cout) { - insn->help_cout(s, &o); + if (ret != DISAS_NORETURN) { + if (insn->help_wout) { + insn->help_wout(s, &f, &o); + } + if (insn->help_cout) { + insn->help_cout(s, &o); + } } /* Free any temporaries created by the helpers. */
When op raises an exception, it may not have initialized the output temps that would be written back by wout or cout. Cc: qemu-s390x@nongnu.org Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/translate.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) -- 2.17.1