[02/31] target/s390x: Implement EXECUTE via new TranslationBlock

Commit Message

Aurelien Jarno May 24, 2017, 5:54 p.m. UTC

On 2017-05-23 16:21, Richard Henderson wrote:
> On 05/23/2017 10:28 AM, Aurelien Jarno wrote:
> > > Something like this, as a delta patch.

I confirm this patch is really needed, otherwise the executed
instruction seems to be executed at the next instruction.

> > Unfortunately it doesn't work. So far I have no real idea what could be
> > the root cause of the issue. I have just determined that up to the crash,
> > only a very limited set of instructions are being executed. They are the
> > 4 bytes long versions of MVC, CLC, XC, TR.
> 
> Yeah, it appears XC is the culprit, though I have not yet determined exactly
> what's going wrong.

It seems the problem arise if an interrupt happens when the TB
containing the EXECUTE instruction is being executed. In that case at
the end of the TB, the interruption code is translated with the ex_value
set, which means with the wrong PC, wrong permissions and wrong return
address.

This is the same kind of issue I identified on SH4 recently:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03880.html

The same king of solution also works, that is disabling the interrupts
when the ex_value is set:

Comments

Richard Henderson May 24, 2017, 9:45 p.m. UTC | #1

On 05/24/2017 10:54 AM, Aurelien Jarno wrote:
> It seems the problem arise if an interrupt happens when the TB
> containing the EXECUTE instruction is being executed. In that case at
> the end of the TB, the interruption code is translated with the ex_value
> set, which means with the wrong PC, wrong permissions and wrong return
> address.
> 
> This is the same kind of issue I identified on SH4 recently:
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03880.html
> 
> The same king of solution also works, that is disabling the interrupts
> when the ex_value is set:
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 6f81b1a16c..a33abdef16 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -655,6 +657,10 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>           S390CPU *cpu = S390_CPU(cs);
>           CPUS390XState *env = &cpu->env;
>   
> +        if (env->ex_value) {
> +            return false;
> +        }
> +
>           if (env->psw.mask & PSW_MASK_EXT) {
>               s390_cpu_do_interrupt(cs);
>               return true;
> 

Thanks for the research.  I've incorporated this into my patch set.


r~

Patch

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 6f81b1a16c..a33abdef16 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -655,6 +657,10 @@  bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         S390CPU *cpu = S390_CPU(cs);
         CPUS390XState *env = &cpu->env;
 
+        if (env->ex_value) {
+            return false;
+        }
+
         if (env->psw.mask & PSW_MASK_EXT) {
             s390_cpu_do_interrupt(cs);
             return true;