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

[David Hildenbrand]

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. If we 
then store the last exception during tlb_fill in a fixed location, we 
could obtain that information.


>       /*
>        * TODO:
>        * - Access key handling
> -     * - CC-option with surpression of page-translation exceptions
>        * - Store r1/r2 register identifiers at real location 162
>        */
>       srca = access_prepare(env, r2, TARGET_PAGE_SIZE, MMU_DATA_LOAD, mmu_idx,


--
Thanks,

David / dhildenb