Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly

[M Bazz <bazz@bazz1.com>]

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