Re: [PATCH 1/7] target/s390x: Fix s390_probe_access for user-only

[Thomas Huth]

On 09/01/2023 21.18, Richard Henderson wrote:
> In db9aab5783a2 we broke the contract of s390_probe_access, in that it
> no longer returned an exception code, nor set __excp_addr.  Fix both.
>
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/tcg/mem_helper.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index cb82cd1c1d..5c0a7b1961 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -138,23 +138,27 @@ typedef struct S390Access {
>    * For !CONFIG_USER_ONLY, the TEC is stored stored to env->tlb_fill_tec.
>    * For CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.
>    */
> -static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
> -                             MMUAccessType access_type, int mmu_idx,
> -                             bool nonfault, void **phost, uintptr_t ra)
> +static inline int s390_probe_access(CPUArchState *env, target_ulong addr,
> +                                    int size, MMUAccessType access_type,
> +                                    int mmu_idx, bool nonfault,
> +                                    void **phost, uintptr_t ra)
>   {
> -#if defined(CONFIG_USER_ONLY)
> -    return probe_access_flags(env, addr, access_type, mmu_idx,
> -                              nonfault, phost, ra);
> -#else
> -    int flags;
> +    int flags = probe_access_flags(env, addr, access_type, mmu_idx,
> +                                   nonfault, phost, ra);
>   
> -    env->tlb_fill_exc = 0;
> -    flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, 
> phost,
> -                               ra);
> -    if (env->tlb_fill_exc) {
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        assert(!nonfault);

 Hi Richard,

qemu-system-s390x now triggers on this assert() if running the
kvm-unit-tests in TCG mode:

$ qemu-system-s390x -nographic -kernel s390x/mvpg.elf
...
PASS: mvpg: exceptions: specification: Key Function Control value 27
PASS: mvpg: exceptions: specification: Key Function Control value 28
PASS: mvpg: exceptions: specification: Key Function Control value 29
PASS: mvpg: exceptions: specification: Key Function Control value 30
PASS: mvpg: exceptions: specification: Key Function Control value 31
qemu-system-s390x: ../../devel/qemu/target/s390x/tcg/mem_helper.c:152: 
s390_probe_access: Assertion `!nonfault' failed.
Aborted (core dumped)

If I've got the test right, it tries to do a "mvpg" with an illegal
address and expects to see an addressing exception.

It seems to work when I remove the assert() statement. Could we maybe
replace it with a qemu_log_mask(LOG_GUEST_ERROR, ...) instead?

> +#ifdef CONFIG_USER_ONLY
> +        /* Address is in TEC in system mode; see s390_cpu_record_sigsegv. */
> +        env->__excp_addr = addr & TARGET_PAGE_MASK;
> +        return (page_get_flags(addr) & PAGE_VALID
> +                ? PGM_PROTECTION : PGM_ADDRESSING);
> +#else
>           return env->tlb_fill_exc;
> +#endif
>       }

 Thomas