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

[David Hildenbrand <david@redhat.com>]

On 03.03.21 14:28, Thomas Huth wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> If the CCO bit is set, MVPG should not generate an exception but
> report page translation faults via a CC code.
> 
> Create a new helper, access_prepare_nf, which can use probe_access_flags
> in non-faulting mode, and then handle watchpoints.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> [thuth: Added logic to still inject protection exceptions]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v4: Add logic to inject protection exceptions if necessary
> 
>   target/s390x/cpu.h         |  3 ++
>   target/s390x/excp_helper.c |  3 ++
>   target/s390x/mem_helper.c  | 93 ++++++++++++++++++++++++++++----------
>   3 files changed, 76 insertions(+), 23 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 60d434d5ed..825503c6c0 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -114,6 +114,9 @@ struct CPUS390XState {
>   
>       uint64_t diag318_info;
>   

Should we start wrapping that stuff into #ifdef CONFIG_TCG ?

> +    uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
> +    int tlb_fill_exc;        /* exception number seen during tlb_fill */
> +
>       /* Fields up to this point are cleared by a CPU reset */
>       struct {} end_reset_fields;
>   
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index ce16af394b..c48cd6b46f 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -164,6 +164,9 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           tec = 0; /* unused */
>       }
>   
> +    env->tlb_fill_exc = excp;
> +    env->tlb_fill_tec = tec;
> +

Just what I had in mind.

>       if (!excp) {
>           qemu_log_mask(CPU_LOG_MMU,
>                         "%s: set tlb %" PRIx64 " -> %" PRIx64 " (%x)\n",
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 25cfede806..cf741541d3 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -130,28 +130,62 @@ typedef struct S390Access {
>       int mmu_idx;
>   } S390Access;
>   
> +static bool access_prepare_nf(S390Access *access, CPUS390XState *env,
> +                              bool nofault, vaddr vaddr1, int size,
> +                              MMUAccessType access_type,
> +                              int mmu_idx, uintptr_t ra)
> +{
> +    void *haddr1, *haddr2 = NULL;
> +    int size1, size2;
> +    vaddr vaddr2 = 0;
> +    int flags;
> +
> +    assert(size > 0 && size <= 4096);
> +
> +    size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
> +    size2 = size - size1;
> +
> +    flags = probe_access_flags(env, vaddr1, access_type, mmu_idx,
> +                               nofault, &haddr1, ra);
> +    if (unlikely(size2)) {
> +        /* The access crosses page boundaries. */
> +        vaddr2 = wrap_address(env, vaddr1 + size1);
> +        flags |= probe_access_flags(env, vaddr2, access_type, mmu_idx,
> +                                    nofault, &haddr2, ra);
> +    }
> +
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        return false;

^ I recall PAGE_WRITE_INV handling where we immediately set 
TLB_INVALID_MASK again on write access (to handle low-address protection 
cleanly). I suspect that TLB_INVALID_MASK will be set in that case (I 
could be wrong, though).

What certainly would work is checking for "haddr != NULL".

/* Don't rely on TLB_INVALID_MASK - see PAGE_WRITE_INV handling. */
if (unlikely(!haddr1)) {
	return false;
}

> +    }
> +    if (unlikely(flags & TLB_WATCHPOINT)) {
> +        /* S390 does not presently use transaction attributes. */
> +        cpu_check_watchpoint(env_cpu(env), vaddr1, size,
> +                             MEMTXATTRS_UNSPECIFIED,
> +                             (access_type == MMU_DATA_STORE
> +                              ? BP_MEM_WRITE : BP_MEM_READ), ra);
> +    }
> +

[...]

>   /* Helper to handle memset on a single page. */
> @@ -845,8 +879,10 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       const int mmu_idx = cpu_mmu_index(env, false);
>       const bool f = extract64(r0, 11, 1);
>       const bool s = extract64(r0, 10, 1);
> +    const bool cco = extract64(r0, 8, 1);
>       uintptr_t ra = GETPC();
>       S390Access srca, desta;
> +    bool ok;
>   
>       if ((f && s) || extract64(r0, 12, 4)) {
>           tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
> @@ -858,13 +894,24 @@ uint32_t HELPER(mvpg)(CPUS390XState *env, uint64_t r0, uint64_t r1, uint64_t r2)
>       /*
>        * 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,
> -                          ra);
> -    desta = access_prepare(env, r1, TARGET_PAGE_SIZE, MMU_DATA_STORE, mmu_idx,
> -                           ra);
> +    ok = access_prepare_nf(&srca, env, cco, r2, TARGET_PAGE_SIZE,
> +                           MMU_DATA_LOAD, mmu_idx, ra);
> +    if (!ok) {
> +        return 2;
> +    }
> +    ok = access_prepare_nf(&desta, env, cco, r1, TARGET_PAGE_SIZE,
> +                           MMU_DATA_STORE, mmu_idx, ra);
> +    if (!ok) {
> +        if (env->tlb_fill_exc == PGM_PROTECTION) {
> +            stq_phys(env_cpu(env)->as,
> +                     env->psa + offsetof(LowCore, trans_exc_code),
> +                     env->tlb_fill_tec);
> +            tcg_s390_program_interrupt(env, PGM_PROTECTION, ra);
> +        }
> +        return 1;
> +    }
>       access_memmove(env, &desta, &srca, ra);
>       return 0; /* data moved */
>   }
> 

Apart from that, looks good to me.

-- 
Thanks,

David / dhildenb