Re: [PATCH 1/2] target/s390x: Implement Early Exception Recognition

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2023-03-14 at 12:00 +0100, Ilya Leoshkevich wrote:
> Generate specification exception if a reserved bit is set in the PSW

Generate a ...
> mask or if the PSW address is out of bounds dictated by the addresing

addresSing

> mode.

Does this approach also work with SET SYSTEM MASK and STORE THEN OR SYSTEM
MASK, i.e. do these call s390_cpu_set_psw via some code path I'm not seeing?

In any case, you don't seem to implement their special requirements with regards
to the ilen, so you should at least mention that.

> 
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  target/s390x/cpu.c             | 26 ++++++++++++++++++++++++++
>  target/s390x/cpu.h             |  1 +
>  target/s390x/tcg/excp_helper.c |  3 ++-
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index b10a8541ff8..8e6e46aa3d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -41,6 +41,26 @@
>  #define CR0_RESET       0xE0UL
>  #define CR14_RESET      0xC2000000UL;
>  
> +#ifndef CONFIG_USER_ONLY
> +static bool is_early_exception_recognized(uint64_t mask, uint64_t addr)
> +{
> +    if (mask & PSW_MASK_RESERVED) {
> +        return true;
> +    }
> +
> +    switch (mask & (PSW_MASK_32 | PSW_MASK_64)) {
> +    case 0:
> +        return addr & ~0xffffffULL;
> +    case PSW_MASK_32:
> +        return addr & ~0x7fffffffULL;
> +    case PSW_MASK_32 | PSW_MASK_64:
> +        return false;
> +    default: /* PSW_MASK_64 */
> +        return true;
> +    }
> +}
> +#endif
> +
>  void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>  {
>  #ifndef CONFIG_USER_ONLY
> @@ -57,6 +77,12 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>      env->cc_op = (mask >> 44) & 3;
>  
>  #ifndef CONFIG_USER_ONLY
> +    if (is_early_exception_recognized(mask, addr)) {
> +        env->int_pgm_ilen = 0;
> +        trigger_pgm_exception(env, PGM_SPECIFICATION);
> +        return;
> +    }
> +
>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>          s390_cpu_recompute_watchpoints(env_cpu(env));
>      }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b2..b4de6945936 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -292,6 +292,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_32             0x0000000080000000ULL
>  #define PSW_MASK_SHORT_ADDR     0x000000007fffffffULL
>  #define PSW_MASK_SHORT_CTRL     0xffffffff80000000ULL
> +#define PSW_MASK_RESERVED       0xb800007e7fffffffULL

What about bit 12?

With bit 12 added:
I haven't checked, but I don't think that would be sufficient for short PSWs loaded
with lpsw. I'm not seeing op_lpsw flipping the 12th bit.
op_lpsw looks pretty broken to me overall, putting the BA bit into the address.
I think it should look something like this (didn't actually try it):

static DisasJumpType op_lpsw(DisasContext *s, DisasOps *o)
{
    TCGv_i64 t1, t2;

    per_breaking_event(s);

    t1 = tcg_temp_new_i64();
    t2 = tcg_temp_new_i64();
    tcg_gen_qemu_ld_i64(t1, o->in2, get_mem_index(s),
                        MO_TEUQ | MO_ALIGN);		//load 8byte instead of 4
    tcg_gen_addi_i64(o->in2, o->in2, 4);
    tcg_gen_qemu_ld32u(t2, o->in2, get_mem_index(s));
    tcg_gen_andi_i64(t2, t2, PSW_MASK_SHORT_ADDR);
    /* Convert the 32-bit PSW_MASK into the 64-bit PSW_MASK.  */
    tcg_gen_andi_i64(t1, t1, PSW_MASK_SHORT_CTRL);
    tcg_gen_xori_i64(t1, t1, PSW_MASK_SHORTPSW);
    gen_helper_load_psw(cpu_env, t1, t2);
    return DISAS_NORETURN;
}
>  
>  #undef PSW_ASC_PRIMARY
>  #undef PSW_ASC_ACCREG
> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
> index bc767f04438..a7829b1e494 100644
> --- a/target/s390x/tcg/excp_helper.c
> +++ b/target/s390x/tcg/excp_helper.c
> @@ -212,7 +212,8 @@ static void do_program_interrupt(CPUS390XState *env)
>      LowCore *lowcore;
>      int ilen = env->int_pgm_ilen;
>  
> -    assert(ilen == 2 || ilen == 4 || ilen == 6);
> +    assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) ||
> +           ilen == 2 || ilen == 4 || ilen == 6);
>  
>      switch (env->int_pgm_code) {
>      case PGM_PER: