Re: [RFC PATCH] target/s390x: Check storage keys in the TPROT instruction

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On 5/2/22 10:25, Thomas Huth wrote:
> TPROT allows to specify an access key that should be used for checking
> with the storage key of the destination page, to see whether an access
> is allowed or not. Honor this storage key checking now in the emulated
> TPROT instruction, too.
> 
> Since we need the absolute address of the page (for getting the storage
> key), we are now also calling mmu_translate() directly instead of
> going via s390_cpu_virt_mem_check_write/read() - since we're only
> interested in one page, and since mmu_translate() does not try to inject
> excetions directly (it reports them via the return code instead), this
> is likely the better function to use in TPROT anyway.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  This fixes the new TPROT-related storage key checks in this new
>  kvm-unit-tests patch:
>  https://lore.kernel.org/kvm/20220425161731.1575742-1-scgl@linux.ibm.com/

Thanks for having a go at this.
The key checking logic looks good to me; the expressions get a bit unwieldy,
but that is a style thing.
However, I'm wondering whether it would be better to mirror what the kernel
is doing and address the

     * TODO: key-controlled protection. Only CPU accesses make use of the
     *       PSW key. CSS accesses are different - we have to pass in the key.

in mmu_handle_skey, then tprot emulation would just relay the result of trying
the translation with store or fetch, passing in the key.
> 
>  target/s390x/cpu.h            |  1 +
>  target/s390x/tcg/mem_helper.c | 61 ++++++++++++++++++++++++++++-------
>  2 files changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..348950239f 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -328,6 +328,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  /* Control register 0 bits */
>  #define CR0_LOWPROT             0x0000000010000000ULL
>  #define CR0_SECONDARY           0x0000000004000000ULL
> +#define CR0_STOR_PROT_OVERRIDE  0x0000000001000000ULL
>  #define CR0_EDAT                0x0000000000800000ULL
>  #define CR0_AFP                 0x0000000000040000ULL
>  #define CR0_VECTOR              0x0000000000020000ULL
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index fc52aa128b..1e0309bbe8 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2141,43 +2141,80 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>      return 0;
>  }
>  

[...]

> +
>  uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
>  {
>      S390CPU *cpu = env_archcpu(env);
> -    CPUState *cs = env_cpu(env);
> +    const int tp_acc = (a2 & SK_ACC_MASK) >> 4;
> +    uint8_t skey;
> +    int acc, pgm_code, pflags;
> +    target_ulong abs_addr;
> +    uint64_t asc = cpu->env.psw.mask & PSW_MASK_ASC;
> +    uint64_t tec;
>  
>      /*
>       * TODO: we currently don't handle all access protection types
> -     * (including access-list and key-controlled) as well as AR mode.
> +     * (including access-list) as well as AR mode.
>       */
> -    if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
> -        /* Fetching permitted; storing permitted */
> +    pgm_code = mmu_translate(env, a1, true, asc, &abs_addr, &pflags, &tec);

I don't like the use of true to indicate a store here, when values other than 0 and 1 are possible.
Any reason not to use MMU_DATA_STORE?

A comment about fetch protection override might be nice here:
       /*
        * Since fetch protection override may apply to half of page 0 only,
        * it need not be considered in the following.
        */
> +    if (!pgm_code) {
> +        /* Fetching permitted; storing permitted - but still check skeys */
> +        skey = get_skey(abs_addr);
> +        acc = (skey & SK_ACC_MASK) >> 4;
> +        if (tp_acc != 0 && tp_acc != acc &&
> +            !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
> +            if (skey & SK_F) {
> +                return 2;
> +            } else {
> +                return 1;
> +            }
> +        }
>          return 0;
>      }
>  
> -    if (env->int_pgm_code == PGM_PROTECTION) {
> +    if (pgm_code == PGM_PROTECTION) {
>          /* retry if reading is possible */
> -        cs->exception_index = -1;
> -        if (!s390_cpu_virt_mem_check_read(cpu, a1, 0, 1)) {
> +        pgm_code = mmu_translate(env, a1, false, asc, &abs_addr, &pflags, &tec);
> +        if (!pgm_code) {
>              /* Fetching permitted; storing not permitted */
> +            skey = get_skey(abs_addr);
> +            acc = (skey & SK_ACC_MASK) >> 4;
> +            if ((skey & SK_F) && tp_acc != 0 && tp_acc != acc &&
> +                !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
> +                return 2;
> +            }
>              return 1;
>          }
>      }
>  
> -    switch (env->int_pgm_code) {
> +    switch (pgm_code) {
>      case PGM_PROTECTION:
>          /* Fetching not permitted; storing not permitted */
> -        cs->exception_index = -1;
>          return 2;
>      case PGM_ADDRESSING:
>      case PGM_TRANS_SPEC:
>          /* exceptions forwarded to the guest */
> -        s390_cpu_virt_mem_handle_exc(cpu, GETPC());
> -        return 0;
> +        tcg_s390_program_interrupt(env, pgm_code, GETPC());
>      }
>  
>      /* Translation not available */
> -    cs->exception_index = -1;
>      return 3;
>  }
>