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

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

On 3/15/23 08:30, Thomas Huth wrote:
> 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?

This is a pre-coffee guess, but the assert looks backward.

We should only arrive there if nonfault was true for the probe (otherwise the probe would 
have raised the exception directly).  I would think we could just remove the assert.


r~