| Message ID | 20230109201856.3916639-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/s390x: mem_helper.c cleanups | expand |
On 09.01.23 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> Should we add a Fixes tag? Reviewed-by: David Hildenbrand <david@redhat.com>
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
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~
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); +#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 } +#ifndef CONFIG_USER_ONLY if (unlikely(flags & TLB_WATCHPOINT)) { /* S390 does not presently use transaction attributes. */ cpu_check_watchpoint(env_cpu(env), addr, size, @@ -162,8 +166,9 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, (access_type == MMU_DATA_STORE ? BP_MEM_WRITE : BP_MEM_READ), ra); } - return 0; #endif + + return 0; } static int access_prepare_nf(S390Access *access, CPUS390XState *env,
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(-)