Skip to contentSkip to conversation

sparc32 boot mode flag fix

PATCH In review Robert Reif 3 participants 7 messages Nov 6, 2007 – Nov 7, 2007

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 sparc32 boot mode flag fix

This patch adds CPU dependent boot mode flag support.

Different CPUs use different bits for the boot mode flag. The constant MMU_BM is replaced with a variable which is set for the selected CPU.

This patch also removes the MMU flags from being saved in the translation block code as a result of an off line discussion with Paul Brook.

This patch also performs a CPU reset after the CPU is registered rather than before.

This patch has successfully booted the debian installer and the initrd kernel in sparc-test successfully for both an ss5 and ss10. It also makes running an ss10 openboot rom image behave a little better.

Index: cpu-exec.c
===================================================================
RCS file: /sources/qemu/qemu/cpu-exec.c,v
retrieving revision 1.120

cpu-exec.c +2 -4
diff -p -u -r1.120 cpu-exec.c
@@ -181,10 +181,8 @@ static inline TranslationBlock *tb_find_
     flags = (((env->pstate & PS_PEF) >> 1) | ((env->fprs & FPRS_FEF) << 2))
         | (env->pstate & PS_PRIV) | ((env->lsu & (DMMU_E | IMMU_E)) >> 2);
 #else
-    // FPU enable . MMU Boot . MMU enabled . MMU no-fault . Supervisor
-    flags = (env->psref << 4) | (((env->mmuregs[0] & MMU_BM) >> 14) << 3)
-        | ((env->mmuregs[0] & (MMU_E | MMU_NF)) << 1)
-        | env->psrs;
+    // FPU enable . Supervisor
+    flags = (env->psref << 4) | env->psrs;
 #endif
     cs_base = env->npc;
     pc = env->pc;

Index: target-sparc/cpu.h
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/cpu.h,v
retrieving revision 1.56

cpu.h +1 -1
diff -p -u -r1.56 cpu.h
@@ -147,7 +147,6 @@
 /* MMU */
 #define MMU_E     (1<<0)
 #define MMU_NF    (1<<1)
-#define MMU_BM    (1<<14)
 
 #define PTE_ENTRYTYPE_MASK 3
 #define PTE_ACCESS_MASK    0x1c
@@ -200,6 +199,7 @@ typedef struct CPUSPARCState {
     int interrupt_index;
     int interrupt_request;
     int halted;
+    uint32_t mmu_bm;
     /* NOTE: we allow 8 more registers to handle wrapping */
     target_ulong regbase[NWINDOWS * 16 + 8];
 

Index: target-sparc/helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/helper.c,v retrieving revision 1.28

helper.c +1 -1
diff -p -u -r1.28 helper.c
@@ -114,7 +114,7 @@ int get_physical_address (CPUState *env,
 
     if ((env->mmuregs[0] & MMU_E) == 0) { /* MMU disabled */
         // Boot mode: instruction fetches are taken from PROM
-        if (rw == 2 && (env->mmuregs[0] & MMU_BM)) {
+        if (rw == 2 && (env->mmuregs[0] & env->mmu_bm)) {
             *physical = 0xff0000000ULL | (address & 0x3ffffULL);
             *prot = PAGE_READ | PAGE_EXEC;
             return 0;

Index: target-sparc/op_helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op_helper.c,v retrieving revision 1.50

op_helper.c +2 -2
diff -p -u -r1.50 op_helper.c
@@ -493,8 +493,8 @@ void helper_st_asi(int asi, int size)
             oldreg = env->mmuregs[reg];
             switch(reg) {
             case 0:
-                env->mmuregs[reg] &= ~(MMU_E | MMU_NF | MMU_BM);
-                env->mmuregs[reg] |= T1 & (MMU_E | MMU_NF | MMU_BM);
+                env->mmuregs[reg] &= ~(MMU_E | MMU_NF | env->mmu_bm);
+                env->mmuregs[reg] |= T1 & (MMU_E | MMU_NF | env->mmu_bm);
                 // Mappings generated during no-fault mode or MMU
                 // disabled mode are invalid in normal mode
                 if (oldreg != env->mmuregs[reg])

Index: target-sparc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/translate.c,v retrieving revision 1.78

translate.c +9 -2
diff -p -u -r1.78 translate.c
@@ -59,6 +59,7 @@ struct sparc_def_t {
     target_ulong iu_version;
     uint32_t fpu_version;
     uint32_t mmu_version;
+    uint32_t mmu_bm;
 };
 
 static uint16_t *gen_opc_ptr;
@@ -3482,7 +3483,7 @@ void cpu_reset(CPUSPARCState *env)
 #else
     env->pc = 0;
     env->mmuregs[0] &= ~(MMU_E | MMU_NF);
-    env->mmuregs[0] |= MMU_BM;
+    env->mmuregs[0] |= env->mmu_bm;
 #endif
     env->npc = env->pc + 4;
 #endif
@@ -3496,7 +3497,6 @@ CPUSPARCState *cpu_sparc_init(void)
     if (!env)
         return NULL;
     cpu_exec_init(env);
-    cpu_reset(env);
     return (env);
 }
 
@@ -3515,30 +3515,35 @@ static const sparc_def_t sparc_defs[] = 
         .iu_version = 0x04 << 24, /* Impl 0, ver 4 */
         .fpu_version = 4 << 17, /* FPU version 4 (Meiko) */
         .mmu_version = 0x04 << 24, /* Impl 0, ver 4 */
+        .mmu_bm = 0x00004000,
     },
     {
         .name = "Fujitsu MB86907",
         .iu_version = 0x05 << 24, /* Impl 0, ver 5 */
         .fpu_version = 4 << 17, /* FPU version 4 (Meiko) */
         .mmu_version = 0x05 << 24, /* Impl 0, ver 5 */
+        .mmu_bm = 0x00004000,
     },
     {
         .name = "TI MicroSparc I",
         .iu_version = 0x41000000,
         .fpu_version = 4 << 17,
         .mmu_version = 0x41000000,
+        .mmu_bm = 0x00004000,
     },
     {
         .name = "TI SuperSparc II",
         .iu_version = 0x40000000,
         .fpu_version = 0 << 17,
         .mmu_version = 0x04000000,
+        .mmu_bm = 0x00002000,
     },
     {
         .name = "Ross RT620",
         .iu_version = 0x1e000000,
         .fpu_version = 1 << 17,
         .mmu_version = 0x17000000,
+        .mmu_bm = 0x00004000,
     },
 #endif
 };
@@ -3579,9 +3584,11 @@ int cpu_sparc_register (CPUSPARCState *e
     env->version = def->iu_version;
     env->fsr = def->fpu_version;
 #if !defined(TARGET_SPARC64)
+    env->mmu_bm = def->mmu_bm;
     env->mmuregs[0] |= def->mmu_version;
     env->mxccregs[7] = ((cpu + 8) & 0xf) << 24;
 #endif
+    cpu_reset(env);
     return 0;
 }
 
#1 lore raw
Quoted 4 lines · Robert Reif

This patch adds CPU dependent boot mode flag support.

Different CPUs use different bits for the boot mode flag. The constant MMU_BM is replaced with a variable which is set for the selected CPU.

Nice. I have a patch for OpenBIOS to fix setting of the boot flag in the exception handler. It's not used except for crashes.

Quoted 3 lines

This patch also removes the MMU flags from being saved in the translation block code as a result of an off line discussion with Paul Brook.

I'd like to hear the reasoning behind that. The TBs generated while in boot mode and MMU disabled may contain translations generated from virtual to physical mappings that do not exist when the mode is changed. Boot mode and MMU disable are not used after boot and these bits don't affect translation, so those bits may be less important and not worth the few bits in TB flags.

No-fault mode is used by Linux to store user register windows forcibly to user stack. This happens often.

Looks like this part works, but I'm not sure this is a correct solution. I'd like to get rid of the TLB flushing when MMU flags change to support the hardware contexts better, this change would rely on that heavily. There is also a long standing problem: system_reset in a SMP system crashes and I suspect this area.

Quoted 2 lines

This patch also performs a CPU reset after the CPU is registered rather than before.

Why is this change needed?

Quoted 4 lines

This patch has successfully booted the debian installer and the initrd kernel
in sparc-test successfully for both an ss5 and ss10. It also makes running an ss10 openboot rom image behave a little better.

It also passes my tests and the patch looks OK for me, except for the questions above.

#2 lore raw
Quoted 10 lines
Quoted 3 lines

This patch also removes the MMU flags from being saved in the translation block code as a result of an off line discussion with Paul Brook.

I'd like to hear the reasoning behind that. The TBs generated while in boot mode and MMU disabled may contain translations generated from virtual to physical mappings that do not exist when the mode is changed. Boot mode and MMU disable are not used after boot and these bits don't affect translation, so those bits may be less important and not worth the few bits in TB flags.

It think you're confusing the TB cache with the TLB. Each TB is already indexed by both physical and virtual address (explicitly in tb_find_slow, and implicitly in tb_find_fast because a tlb flush clears env->tb_jmp_cache).

IIUC enabling/disabling boot mode is no different to and other VM change. If the virtual->physical mapping happens to be the same then it's perfectly ok to reuse the TB.

The TLB is already flushed whenever the MMU mode is changes. There is no need to invalidate the TB.

Paul

#3 lore raw
Quoted 11 lines · Paul Brook
Quoted 10 lines
Quoted 3 lines

This patch also removes the MMU flags from being saved in the translation block code as a result of an off line discussion with Paul Brook.

I'd like to hear the reasoning behind that. The TBs generated while in boot mode and MMU disabled may contain translations generated from virtual to physical mappings that do not exist when the mode is changed. Boot mode and MMU disable are not used after boot and these bits don't affect translation, so those bits may be less important and not worth the few bits in TB flags.

Quoted 7 lines

It think you're confusing the TB cache with the TLB. Each TB is already indexed by both physical and virtual address (explicitly in tb_find_slow, and implicitly in tb_find_fast because a tlb flush clears env->tb_jmp_cache).

IIUC enabling/disabling boot mode is no different to and other VM change. If the virtual->physical mapping happens to be the same then it's perfectly ok to reuse the TB.

Not in this case: in boot mode, physical and virtual address 0 generates TBs from PROM code. When control transfers to OS in normal MMU mode, virtual and physical address 0 generates TBs from code in RAM.

Quoted 2 lines

The TLB is already flushed whenever the MMU mode is changes. There is no need to invalidate the TB.

The TB is valid, but only when the boot mode bit is the same as recorded in the TB flags.

#4 lore raw
Quoted 6 lines
Quoted 3 lines

IIUC enabling/disabling boot mode is no different to and other VM change. If the virtual->physical mapping happens to be the same then it's perfectly ok to reuse the TB.

Not in this case: in boot mode, physical and virtual address 0 generates TBs from PROM code.

How is this different to using the MMU to map the PROM at virtual address zero?

I see exactly one use of MMU_BM. It simply hard-wires a particular virtual->physical address mapping.

Paul

#5 lore raw
Quoted 9 lines · Paul Brook
Quoted 6 lines
Quoted 3 lines

IIUC enabling/disabling boot mode is no different to and other VM change. If the virtual->physical mapping happens to be the same then it's perfectly ok to reuse the TB.

Not in this case: in boot mode, physical and virtual address 0 generates TBs from PROM code.

How is this different to using the MMU to map the PROM at virtual address zero?

In that case, the virtual address is zero, but physical address is 0xff0000000. Oh, now I see my error, it is the same in the boot mode case too! Sorry about the noise.

Quoted 2 lines

I see exactly one use of MMU_BM. It simply hard-wires a particular virtual->physical address mapping.

It derives from the Sparc specification, where the PC at reset is zero. I wonder why they didn't put the start of memory somewhere higher instead of this hack.

On Sparc64 the reset vector is implementation dependent so it can be directly set to the PROM address.

#6 lore raw
In reply to Blue Swirl
Quoted 10 lines
Quoted 5 lines

This patch also performs a CPU reset after the CPU is registered rather than before.

Why is this change needed?

Reset should be doing CPU dependent stuff and the CPU dependent setup is performed when the CPU is registered.