target/sparc/translate.c | 2 ++ 1 file changed, 2 insertions(+)
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 <bazz@bazz1.com>
---
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 <tom@fnord.at>
Copyright (C) 2003-2005 Fabrice Bellard
+ Copyright (C) 2024 M Bazz <bazz@bazz1.com>
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.0On 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 <bazz@bazz1.com>
> ---
> 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 <tom@fnord.at>
> Copyright (C) 2003-2005 Fabrice Bellard
> + Copyright (C) 2024 M Bazz <bazz@bazz1.com>
>
> 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~
On Thu, Apr 11, 2024, 5:55 PM Richard Henderson
<richard.henderson@linaro.org> 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 <bazz@bazz1.com>
> > ---
> > 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 <tom@fnord.at>
> > Copyright (C) 2003-2005 Fabrice Bellard
> > + Copyright (C) 2024 M Bazz <bazz@bazz1.com>
> >
> > 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;
On 4/11/24 18:15, M Bazz wrote:
> On Thu, Apr 11, 2024, 5:55 PM Richard Henderson
> <richard.henderson@linaro.org> 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 <bazz@bazz1.com>
>>> ---
>>> 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 <tom@fnord.at>
>>> Copyright (C) 2003-2005 Fabrice Bellard
>>> + Copyright (C) 2024 M Bazz <bazz@bazz1.com>
>>>
>>> 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));
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.
r~
Hi Richard, On Thu, Apr 11, 2024 at 10:16 PM Richard Henderson <richard.henderson@linaro.org> 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~
On 4/13/24 18:54, M Bazz wrote:
> 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?
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.
However, using the same value for ASI_USERTXT would be incorrect. For ASI_USERTXT and
ASI_USERDATA (and the new names for sparcv9, ASI_AIU*, "as-if user"), we need permissions
for the user context.
That's what
> + case ASI_USERTXT: /* User text access */
> + mem_idx = MMU_USER_IDX;
this is for in my patch. This gets passed into the helper,
> + case GET_ASI_CODE:
> +#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
> + {
> + MemOpIdx oi = make_memop_idx(da->memop, da->mem_idx);
> + TCGv_i64 t64 = tcg_temp_new_i64();
> +
> + gen_helper_ld_code(t64, tcg_env, addr, tcg_constant_i32(oi));
and then into the core memory access functions,
> + ret = cpu_ldl_code_mmu(env, addr, oi, ra);
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.
For sparc32, there are 3 mmu_idx:
> #define MMU_USER_IDX 0
> #define MMU_KERNEL_IDX 1
> #define MMU_PHYS_IDX 2
The interpretation of mmu_idx happens in target/sparc/mmu_helper.c, get_physical_address
(note there are two versions, for sparc32 and sparc64).
Ignoring MMU_PHYS_IDX, which is handled early, the difference between kernel and user is
is_user = mmu_idx == MMU_USER_IDX;
...
full->prot = perm_table[is_user][access_perms];
This controls the read/write/execute permissions for the page. Note that perm_table
matches the Access Allowed table on page 248 of the Sparc V8 architecture manual.
r~
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
<richard.henderson@linaro.org> 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
On 4/14/24 15:48, M Bazz wrote:
> 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).
No, it cannot.
> Note that this check is gone in your patch.
Correct. Since 'lda' has already checked that supervisor mode has been enabled, the
translator may jump directly to the desired result of MMU_KERNEL_IDX.
> 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;
Correct, that would also work.
r~
© 2016 - 2024 Red Hat, Inc.