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);
+#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,
--
2.34.1On 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~
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> -- Thanks, David / dhildenb
© 2016 - 2026 Red Hat, Inc.