Re: [PATCH 1/2] target/s390x: Fix SIGILL psw.addr reporting

[David Hildenbrand <david@redhat.com>]

On 21.05.21 05:01, Ilya Leoshkevich wrote:
> When a s390x CPU attempts to execute an illegal instruction, an
> operation exception is recognized. This is a suppressing exception,
> which means that the PSW is advanced by the length of the illegal
> instruction.
> 
> On the real hardware or in qemu-system-s390x the kernel then raises
> SIGILL with si_addr pointing to the suppressed instruction and
> psw.addr containing the updated PSW.
> 
> Unfortunately qemu-s390x sets both to the address of the suppressed
> instruction at the moment. Fix by sharing the PSW advancement logic
> with qemu-system-s390x and setting si_addr to the address of the
> instruction that raised the exception.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1920913
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/s390x/cpu_loop.c |  6 +++-
>   target/s390x/excp_helper.c  | 69 ++++++++++++++++++++-----------------
>   target/s390x/internal.h     |  1 +
>   3 files changed, 43 insertions(+), 33 deletions(-)
> 
> diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
> index f2d1215fb1..6f5462d4f8 100644
> --- a/linux-user/s390x/cpu_loop.c
> +++ b/linux-user/s390x/cpu_loop.c
> @@ -21,6 +21,7 @@
>   #include "qemu-common.h"
>   #include "qemu.h"
>   #include "cpu_loop-common.h"
> +#include "internal.h"
>   
>   /* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
>   #define S390X_FAIL_ADDR_MASK -4096LL
> @@ -29,6 +30,7 @@ void cpu_loop(CPUS390XState *env)
>   {
>       CPUState *cs = env_cpu(env);
>       int trapnr, n, sig;
> +    target_ulong excp_psw_addr;
>       target_siginfo_t info;
>       target_ulong addr;
>       abi_long ret;
> @@ -38,6 +40,7 @@ void cpu_loop(CPUS390XState *env)
>           trapnr = cpu_exec(cs);
>           cpu_exec_end(cs);
>           process_queued_cpu_work(cs);
> +        excp_psw_addr = env->psw.addr;
>   
>           switch (trapnr) {
>           case EXCP_INTERRUPT:
> @@ -66,6 +69,7 @@ void cpu_loop(CPUS390XState *env)
>               n = TARGET_TRAP_BRKPT;
>               goto do_signal_pc;
>           case EXCP_PGM:
> +            s390_cpu_program_interrupt_advance_psw(env);
>               n = env->int_pgm_code;
>               switch (n) {
>               case PGM_OPERATION:
> @@ -131,7 +135,7 @@ void cpu_loop(CPUS390XState *env)
>               break;
>   
>           do_signal_pc:
> -            addr = env->psw.addr;
> +            addr = excp_psw_addr;
>           do_signal:
>               info.si_signo = sig;
>               info.si_errno = 0;
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index 20625c2c8f..0a323967ae 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -82,6 +82,42 @@ void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
>       tcg_s390_data_exception(env, dxc, GETPC());
>   }
>   
> +void s390_cpu_program_interrupt_advance_psw(CPUS390XState *env)
> +{
> +    switch (env->int_pgm_code) {
> +    case PGM_PER:
> +        if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
> +            break;
> +        }
> +        /* FALL THROUGH */
> +    case PGM_OPERATION:
> +    case PGM_PRIVILEGED:
> +    case PGM_EXECUTE:
> +    case PGM_PROTECTION:
> +    case PGM_ADDRESSING:
> +    case PGM_SPECIFICATION:
> +    case PGM_DATA:
> +    case PGM_FIXPT_OVERFLOW:
> +    case PGM_FIXPT_DIVIDE:
> +    case PGM_DEC_OVERFLOW:
> +    case PGM_DEC_DIVIDE:
> +    case PGM_HFP_EXP_OVERFLOW:
> +    case PGM_HFP_EXP_UNDERFLOW:
> +    case PGM_HFP_SIGNIFICANCE:
> +    case PGM_HFP_DIVIDE:
> +    case PGM_TRANS_SPEC:
> +    case PGM_SPECIAL_OP:
> +    case PGM_OPERAND:
> +    case PGM_HFP_SQRT:
> +    case PGM_PC_TRANS_SPEC:
> +    case PGM_ALET_SPEC:
> +    case PGM_MONITOR:
> +        /* advance the PSW if our exception is not nullifying */
> +        env->psw.addr += env->int_pgm_ilen;
> +        break;
> +    }
> +}
> +
>   #if defined(CONFIG_USER_ONLY)
>   
>   void s390_cpu_do_interrupt(CPUState *cs)
> @@ -202,38 +238,7 @@ static void do_program_interrupt(CPUS390XState *env)
>   
>       assert(ilen == 2 || ilen == 4 || ilen == 6);
>   
> -    switch (env->int_pgm_code) {
> -    case PGM_PER:
> -        if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
> -            break;
> -        }
> -        /* FALL THROUGH */
> -    case PGM_OPERATION:
> -    case PGM_PRIVILEGED:
> -    case PGM_EXECUTE:
> -    case PGM_PROTECTION:
> -    case PGM_ADDRESSING:
> -    case PGM_SPECIFICATION:
> -    case PGM_DATA:
> -    case PGM_FIXPT_OVERFLOW:
> -    case PGM_FIXPT_DIVIDE:
> -    case PGM_DEC_OVERFLOW:
> -    case PGM_DEC_DIVIDE:
> -    case PGM_HFP_EXP_OVERFLOW:
> -    case PGM_HFP_EXP_UNDERFLOW:
> -    case PGM_HFP_SIGNIFICANCE:
> -    case PGM_HFP_DIVIDE:
> -    case PGM_TRANS_SPEC:
> -    case PGM_SPECIAL_OP:
> -    case PGM_OPERAND:
> -    case PGM_HFP_SQRT:
> -    case PGM_PC_TRANS_SPEC:
> -    case PGM_ALET_SPEC:
> -    case PGM_MONITOR:
> -        /* advance the PSW if our exception is not nullifying */
> -        env->psw.addr += ilen;
> -        break;
> -    }
> +    s390_cpu_program_interrupt_advance_psw(env);
>   
>       qemu_log_mask(CPU_LOG_INT,
>                     "%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n",
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 11515bb617..9f1665ccbf 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -272,6 +272,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>   void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>                                      MMUAccessType access_type,
>                                      int mmu_idx, uintptr_t retaddr);
> +void s390_cpu_program_interrupt_advance_psw(CPUS390XState *cpu);
>   
>   
>   /* fpu_helper.c */
> 


LGTM, thanks

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb