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 <th...@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-s...@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; > } >