[PATCH] target/sparc: resolve ASI_USERTXT correctly
fixes a longstanding bug which causes a "Nonparity Synchronous Error"
kernel panic while using a debugger on Solaris / SunOS systems. The panic
would occur on the first attempt to single-step the process.
The problem stems from an lda instruction on ASI_USERTXT (8). This asi
was not being resolved correctly by resolve_asi().
Further details can be found in #2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
Signed-off-by: M Bazz
---
target/sparc/translate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 319934d9bd..1596005e22 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3,6 +3,7 @@
Copyright (C) 2003 Thomas M. Ogrisegg
Copyright (C) 2003-2005 Fabrice Bellard
+ Copyright (C) 2024 M Bazz
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int asi,
MemOp memop)
|| (asi == ASI_USERDATA
&& (dc->def->features & CPU_FEATURE_CASA))) {
switch (asi) {
+case ASI_USERTXT:/* User text access */
case ASI_USERDATA: /* User data access */
mem_idx = MMU_USER_IDX;
type = GET_ASI_DIRECT;
--
2.43.0
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
On Thu, Apr 11, 2024, 5:55 PM Richard Henderson
wrote:
>
> On 4/11/24 14:29, M Bazz wrote:
> > fixes a longstanding bug which causes a "Nonparity Synchronous Error"
> > kernel panic while using a debugger on Solaris / SunOS systems. The panic
> > would occur on the first attempt to single-step the process.
> >
> > The problem stems from an lda instruction on ASI_USERTXT (8). This asi
> > was not being resolved correctly by resolve_asi().
> >
> > Further details can be found in #2281
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
> >
> > Signed-off-by: M Bazz
> > ---
> > target/sparc/translate.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> > index 319934d9bd..1596005e22 100644
> > --- a/target/sparc/translate.c
> > +++ b/target/sparc/translate.c
> > @@ -3,6 +3,7 @@
> >
> > Copyright (C) 2003 Thomas M. Ogrisegg
> > Copyright (C) 2003-2005 Fabrice Bellard
> > + Copyright (C) 2024 M Bazz
> >
> > This library is free software; you can redistribute it and/or
> > modify it under the terms of the GNU Lesser General Public
> > @@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int
> > asi, MemOp memop)
> > || (asi == ASI_USERDATA
> > && (dc->def->features & CPU_FEATURE_CASA))) {
> > switch (asi) {
> > +case ASI_USERTXT:/* User text access */
> > case ASI_USERDATA: /* User data access */
> > mem_idx = MMU_USER_IDX;
> > type = GET_ASI_DIRECT;
>
> I don't believe this is correct, because it operates against the page's
> "read" permissions
> instead of "execute" permissions.
>
>
> r~
Hi Richard,
Thanks for your guidance. It set me in the right direction. Now I
think I've got the right spot.
function `helper_ld_asi` has a block to help load ASI_KERNELTXT, but the
ASI_USERTXT case defaults to sparc_raise_mmu_fault(); I believe this
is the true culprit
source reference:
https://gitlab.com/qemu-project/qemu/-/blob/master/target/sparc/ldst_helper.c?ref_type=heads#L687
The code for the ASI_KERNELTXT seems generic enough to also use for
ASI_USERTXT verbatim.
See v2 patch below. I've done a `make test` -- all passing (3 skips).
OS boots, and the
debuggers are working without issue. What do you think?
Once we arrive at the right solution, I'll finalize the patch.
-bazz
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index e581bb42ac..4f87e44a93 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -684,6 +684,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
break;
+case ASI_USERTXT: /* User code access */
case ASI_KERNELTXT: /* Supervisor code access */
oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
switch (size) {
@@ -779,7 +780,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case 0x4c: /* SuperSPARC MMU Breakpoint Action */
ret = env->mmubpaction;
break;
-case ASI_USERTXT: /* User code access, XXX */
default:
sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
ret = 0;
Re: [PATCH] target/sparc: Use GET_ASI_CODE for ASI_KERNELTXT and ASI_USERTXT
On Thu, Apr 11, 2024, 10:15 PM Richard Henderson <
richard.hender...@linaro.org> wrote:
> Reads are done with execute access. It is not clear whether writes
> are legal at all -- for now, leave helper_st_asi unchanged, so that
> we continue to raise an mmu fault.
>
> This generalizes the exiting code for ASI_KERNELTXT to be usable for
> ASI_USERTXT as well, by passing down the MemOpIdx to use.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
> Signed-off-by: Richard Henderson
> ---
> target/sparc/helper.h | 3 ++
> target/sparc/ldst_helper.c | 65 ++
> target/sparc/translate.c | 49 ++--
> 3 files changed, 95 insertions(+), 22 deletions(-)
>
> diff --git a/target/sparc/helper.h b/target/sparc/helper.h
> index e55fad5b8c..b8087d0d2b 100644
> --- a/target/sparc/helper.h
> +++ b/target/sparc/helper.h
> @@ -32,6 +32,9 @@ DEF_HELPER_FLAGS_3(udiv, TCG_CALL_NO_WG, i64, env, tl,
> tl)
> DEF_HELPER_FLAGS_3(sdiv, TCG_CALL_NO_WG, i64, env, tl, tl)
> DEF_HELPER_3(taddcctv, tl, env, tl, tl)
> DEF_HELPER_3(tsubcctv, tl, env, tl, tl)
> +#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
> +DEF_HELPER_FLAGS_3(ld_code, TCG_CALL_NO_WG, i64, env, tl, i32)
> +#endif
> #if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
> DEF_HELPER_FLAGS_4(ld_asi, TCG_CALL_NO_WG, i64, env, tl, int, i32)
> DEF_HELPER_FLAGS_5(st_asi, TCG_CALL_NO_WG, void, env, tl, i64, int, i32)
> diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
> index e581bb42ac..2846a86cc4 100644
> --- a/target/sparc/ldst_helper.c
> +++ b/target/sparc/ldst_helper.c
> @@ -585,7 +585,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> #if defined(DEBUG_MXCC) || defined(DEBUG_ASI)
> uint32_t last_addr = addr;
> #endif
> -MemOpIdx oi;
>
> do_check_align(env, addr, size - 1, GETPC());
> switch (asi) {
> @@ -684,24 +683,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
> case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
> break;
> -case ASI_KERNELTXT: /* Supervisor code access */
> -oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
> -switch (size) {
> -case 1:
> -ret = cpu_ldb_code_mmu(env, addr, oi, GETPC());
> -break;
> -case 2:
> -ret = cpu_ldw_code_mmu(env, addr, oi, GETPC());
> -break;
> -default:
> -case 4:
> -ret = cpu_ldl_code_mmu(env, addr, oi, GETPC());
> -break;
> -case 8:
> -ret = cpu_ldq_code_mmu(env, addr, oi, GETPC());
> -break;
> -}
> -break;
> case ASI_M_TXTC_TAG: /* SparcStation 5 I-cache tag */
> case ASI_M_TXTC_DATA: /* SparcStation 5 I-cache data */
> case ASI_M_DATAC_TAG: /* SparcStation 5 D-cache tag */
> @@ -779,7 +760,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> case 0x4c: /* SuperSPARC MMU Breakpoint Action */
> ret = env->mmubpaction;
> break;
> -case ASI_USERTXT: /* User code access, XXX */
> default:
> sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
> ret = 0;
> @@ -787,6 +767,8 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
>
> case ASI_USERDATA: /* User data access */
> case ASI_KERNELDATA: /* Supervisor data access */
> +case ASI_USERTXT: /* User code access */
> +case ASI_KERNELTXT: /* Supervisor code access */
> case ASI_P: /* Implicit primary context data access (v9 only?) */
> case ASI_M_BYPASS:/* MMU passthrough */
> case ASI_LEON_BYPASS: /* LEON MMU passthrough */
> @@ -1161,6 +1143,49 @@ void helper_st_asi(CPUSPARCState *env, target_ulong
> addr, uint64_t val,
> #endif
> }
>
> +uint64_t helper_ld_code(CPUSPARCState *env, target_ulong addr, uint32_t
> oi)
> +{
> +MemOp mop = get_memop(oi);
> +uintptr_t ra = GETPC();
> +uint64_t ret;
> +
> +switch (mop & MO_SIZE) {
> +case MO_8:
> +ret = cpu_ldb_code_mmu(env, addr, oi, ra);
> +if (mop & MO_SIGN) {
> +ret = (int8_t)ret;
> +}
> +break;
> +case MO_16:
> +ret = cpu_ldw_code_mmu(env, addr, oi, ra);
> +if ((mop & MO_BSWAP) != MO_TE) {
> +ret = bswap16(ret);
> +}
> +if (mop & MO_SIGN) {
> +ret = (int16_t)ret;
> +}
> +break;
> +case MO_32:
> +ret = cpu_ldl_code_mmu(env, addr, oi, ra);
> +if ((mop & MO_BSWAP) != MO_TE) {
> +ret = bswap32(ret);
> +}
> +if (mop & MO_SIGN) {
> +ret = (int32_t)ret;
> +}
>
Re: [PATCH] target/sparc: Use GET_ASI_CODE for ASI_KERNELTXT and ASI_USERTXT
Hi Philippe, On Fri, Apr 12, 2024 at 7:14 AM Philippe Mathieu-Daudé wrote: > > Hi Bazz, > > On 12/4/24 06:18, M Bazz wrote: > > On Thu, Apr 11, 2024, 10:15 PM Richard Henderson > > mailto:richard.hender...@linaro.org>> wrote: > > > > Reads are done with execute access. It is not clear whether writes > > are legal at all -- for now, leave helper_st_asi unchanged, so that > > we continue to raise an mmu fault. > > > > This generalizes the exiting code for ASI_KERNELTXT to be usable for > > ASI_USERTXT as well, by passing down the MemOpIdx to use. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281 > > <https://gitlab.com/qemu-project/qemu/-/issues/2281> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059 > > <https://gitlab.com/qemu-project/qemu/-/issues/2059> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609 > > <https://gitlab.com/qemu-project/qemu/-/issues/1609> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166 > > <https://gitlab.com/qemu-project/qemu/-/issues/1166> > > Signed-off-by: Richard Henderson > <mailto:richard.hender...@linaro.org>> > > --- > > target/sparc/helper.h | 3 ++ > > target/sparc/ldst_helper.c | 65 ++ > > target/sparc/translate.c | 49 ++-- > > 3 files changed, 95 insertions(+), 22 deletions(-) > > > Hi Richard, > > > > I see this is in your hands now. I agree with your take on leaving > > writes alone. I'm also grateful for the opportunity to collaborate with you. > > > > It brings a smile for the community members who will be touched by this > > amazing contribution. I see them happily realizing that this perplexing > > bug has been solved, and in our case finally able to use the debuggers > > we love! :D > > Does that mean you tested this patch and it resolves your issues? > > If so, could you add your 'Tested-by: M Bazz ' tag > when committing this patch? > > Regards, > > Phil. Yes, I can confirm the issue is resolved. Richard has already made the pull request. I am appreciative of the fast movement on this. I would like to have my "Tested-by" tag added to the PR, if it's no trouble. How might I get it there? Tested-by: M Bazz > > > Thanks for the proper fix, qemu sensei! > > > > -bazz > > >
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
Hi Richard, On Thu, Apr 11, 2024 at 10:16 PM Richard Henderson wrote: > > On 4/11/24 18:15, M Bazz wrote: > > diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c > > index e581bb42ac..4f87e44a93 100644 > > --- a/target/sparc/ldst_helper.c > > +++ b/target/sparc/ldst_helper.c > > @@ -684,6 +684,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, > > target_ulong addr, > > case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */ > > case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */ > > break; > > +case ASI_USERTXT: /* User code access */ > > case ASI_KERNELTXT: /* Supervisor code access */ > > oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true)); > > No, this also does not work, because it uses the wrong permissions (kernel > instead of > user). I have just sent a patch to fix both problems. This thought just came to me. `lda` is a privileged instruction. It has to run in supervisor mode. So, I'm struggling to understand how the kernel permission was wrong. Isn't that the right permission for this instruction? I just want to have the right understanding, so that I'm more prepared for the next time. Here is a related excerpt from the Sparcv8 manual: "For each instruction access and each normal data access, the IU appends to the 32-bit memory address an 8-bit address space identifier, or ASI. The ASI encodes whether the processor is in supervisor or user mode, and whether the access is an instruction or data access. There are also privileged load/store alternate instructions (see below) that can provide an arbitrary ASI with their data addresses." Thank you, -bazz > > > r~
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
Hi Henry,
I want to thank you for every chance I get to learn from you. Each
email excites me.
On Sun, Apr 14, 2024 at 1:20 PM Richard Henderson
wrote:
> The "current" permission, as computed by
>
> > -case ASI_KERNELTXT: /* Supervisor code access */
> > -oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
>
> was correct for ASI_KERNELTXT, because as you say "lda" is a supervisor-only
> instruction
> prior to sparcv9.
>
I noticed that cpu_mmu_index() would have returned MMU_USER_IDX
if the supervisor bit hadn't happened to be set (not sure if this
execution path can
occur for lda). Note that this check is gone in your patch.
I consider you my sensei, so while I'm confident in your work I also
want to show
the things I catch.
If I understand everything you've taught me, then the following patch would
have also satisfied the permissions issue. Could you confirm this please?
The essential change is the MMU_USER_IDX in the call to make_memop_idx()
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index e581bb42ac..be3c03a3b6 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -702,6 +702,24 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
break;
}
break;
+case ASI_USERTXT: /* User code access */
+oi = make_memop_idx(memop, MMU_USER_IDX);
+switch (size) {
+case 1:
+ret = cpu_ldb_code_mmu(env, addr, oi, GETPC());
+break;
+case 2:
+ret = cpu_ldw_code_mmu(env, addr, oi, GETPC());
+break;
+default:
+case 4:
+ret = cpu_ldl_code_mmu(env, addr, oi, GETPC());
+break;
+case 8:
+ret = cpu_ldq_code_mmu(env, addr, oi, GETPC());
+break;
+}
+break;
case ASI_M_TXTC_TAG: /* SparcStation 5 I-cache tag */
case ASI_M_TXTC_DATA: /* SparcStation 5 I-cache data */
case ASI_M_DATAC_TAG: /* SparcStation 5 D-cache tag */
@@ -779,7 +797,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case 0x4c: /* SuperSPARC MMU Breakpoint Action */
ret = env->mmubpaction;
break;
-case ASI_USERTXT: /* User code access, XXX */
default:
sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
ret = 0;
> Unfortunately, we do not have any good documentation for tcg softmmu or the
> intended role
> of the mmu_idx. Partly that's due to the final use of the mmu_idx is
> target-specific.
I love learning from code, but the lack of documentation definitely
increases the value of your
discourse with me.
Thank you,
Sincerely,
-bazz
Re: [PULL 0/1] target/sparc late fix
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0 for any user-visible changes. The 9.0 Changelog was never updated. Could someone with the permissions please add the following to the SPARC section: sparc32: Fixed a longstanding softmmu bug that caused kernel panics when the UserTxt ASI was accessed. Appreciated, -- Bazz -- PMM
