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

[Janis Schoetterl-Glausch]

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 <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);

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.

[...]