Re: [PATCH] target/s390x: Implement the MVPG condition-code-option bit

[Richard Henderson <richard.henderson@linaro.org>]

On 3/2/21 11:25 AM, David Hildenbrand wrote:
> On 02.03.21 20:12, Thomas Huth wrote:
>> If the CCO bit is set, MVPG should not generate an exception
>> but report page translation faults via a CC code, so we have
>> to check the translation in this case before calling the
>> access_prepare() function.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   This patch is required to get Claudio's new kvm-unit-tests patches
>>   working with TCG: https://www.spinics.net/lists/kvm/msg236784.html
>>
>>   target/s390x/cpu.h         | 14 ++++++++++++++
>>   target/s390x/excp_helper.c | 14 --------------
>>   target/s390x/mem_helper.c  | 23 ++++++++++++++++++++++-
>>   3 files changed, 36 insertions(+), 15 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 60d434d5ed..731e2c6452 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -366,6 +366,20 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool 
>> ifetch)
>>   #endif
>>   }
>> +static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
>> +{
>> +    switch (mmu_idx) {
>> +    case MMU_PRIMARY_IDX:
>> +        return PSW_ASC_PRIMARY;
>> +    case MMU_SECONDARY_IDX:
>> +        return PSW_ASC_SECONDARY;
>> +    case MMU_HOME_IDX:
>> +        return PSW_ASC_HOME;
>> +    default:
>> +        abort();
>> +    }
>> +}
>> +
>>   static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
>>                                           target_ulong *cs_base, uint32_t 
>> *flags)
>>   {
>> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
>> index ce16af394b..44bff27f8f 100644
>> --- a/target/s390x/excp_helper.c
>> +++ b/target/s390x/excp_helper.c
>> @@ -105,20 +105,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int 
>> size,
>>   #else /* !CONFIG_USER_ONLY */
>> -static inline uint64_t cpu_mmu_idx_to_asc(int mmu_idx)
>> -{
>> -    switch (mmu_idx) {
>> -    case MMU_PRIMARY_IDX:
>> -        return PSW_ASC_PRIMARY;
>> -    case MMU_SECONDARY_IDX:
>> -        return PSW_ASC_SECONDARY;
>> -    case MMU_HOME_IDX:
>> -        return PSW_ASC_HOME;
>> -    default:
>> -        abort();
>> -    }
>> -}
>> -
>>   bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>                          MMUAccessType access_type, int mmu_idx,
>>                          bool probe, uintptr_t retaddr)
>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>> index 25cfede806..c7037adf2c 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -855,10 +855,31 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, 
>> uint64_t r1, uint64_t r2)
>>       r1 = wrap_address(env, r1 & TARGET_PAGE_MASK);
>>       r2 = wrap_address(env, r2 & TARGET_PAGE_MASK);
>> +    /*
>> +     * If the condition-code-option (CCO) bit is set and DAT is enabled,
>> +     * we have to check for page table translation faults first:
>> +     */
>> +#ifndef CONFIG_USER_ONLY
>> +    if (extract64(r0, 8, 1) && mmu_idx != MMU_REAL_IDX) {
>> +        uint64_t asc = cpu_mmu_idx_to_asc(mmu_idx);
>> +        uint64_t raddr, tec;
>> +        int flags, exc;
>> +
>> +        exc = mmu_translate(env, r2, MMU_DATA_LOAD, asc, &raddr, &flags, &tec);
>> +        if (exc) {
>> +            return 2;
>> +        }
>> +
>> +        exc = mmu_translate(env, r1, MMU_DATA_STORE, asc, &raddr, &flags, 
>> &tec);
>> +        if (exc && exc != PGM_PROTECTION) {
>> +            return 1;
>> +        }
>> +    }
>> +#endif
>> +
> 
> This way you always need two additional translations and don't even check if we 
> have something in the TLB. While this works, it's quite inefficient.
> 
> Using probe_access_flags() we can actually lookup the tlb/fill the tlb but get 
> an error instead of a fault. We could e.g., extent probe_access() to allow 
> specifying whether we want a fault or not.

I think probe_access_flags() will do all that you need; no further extension to 
probe_access() required.  I presume you meant access_prepare() is what you 
meant to extend?


r~