On 5/2/22 12:17, Janis Schoetterl-Glausch wrote: > 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 <[email protected]> >> --- >> This fixes the new TPROT-related storage key checks in this new >> kvm-unit-tests patch: >> https://lore.kernel.org/kvm/[email protected]/ > > 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);
mmu_translate/mmu_handle_skey sets the change bit for stores, whereas TPROT specifies that it doesn't. Not sure what the best way to handle this is. Additional pretend fetch/store access modes? > > 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. > */ Disregard that, it's not true, TPROT does honor fetch-protection override, I just made a mistake while adding a test for it to the kvm-unit-test. [...]
