[Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Robert Reif]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

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.

[-- Attachment #2: mmu_bm.diff.txt --]
[-- Type: text/plain, Size: 5599 bytes --]

Index: cpu-exec.c
===================================================================
RCS file: /sources/qemu/qemu/cpu-exec.c,v
retrieving revision 1.120
diff -p -u -r1.120 cpu-exec.c
--- cpu-exec.c	14 Oct 2007 07:07:04 -0000	1.120
+++ cpu-exec.c	6 Nov 2007 00:12:56 -0000
@@ -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
diff -p -u -r1.56 cpu.h
--- target-sparc/cpu.h	14 Oct 2007 17:07:21 -0000	1.56
+++ target-sparc/cpu.h	6 Nov 2007 00:13:00 -0000
@@ -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
diff -p -u -r1.28 helper.c
--- target-sparc/helper.c	14 Oct 2007 07:07:08 -0000	1.28
+++ target-sparc/helper.c	6 Nov 2007 00:13:00 -0000
@@ -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
diff -p -u -r1.50 op_helper.c
--- target-sparc/op_helper.c	29 Oct 2007 14:39:49 -0000	1.50
+++ target-sparc/op_helper.c	6 Nov 2007 00:13:01 -0000
@@ -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
diff -p -u -r1.78 translate.c
--- target-sparc/translate.c	17 Oct 2007 17:34:57 -0000	1.78
+++ target-sparc/translate.c	6 Nov 2007 00:13:03 -0000
@@ -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;
 }
 

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Blue Swirl]

On 11/6/07, Robert Reif <reif@earthlink.net> wrote:
> 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.

> 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.

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

Why is this change needed?

> 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.

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Paul Brook]

> > 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

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Blue Swirl]

On 11/6/07, Paul Brook <paul@codesourcery.com> wrote:
> > > 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.

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.

> 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.

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Paul Brook]

> > 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

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Blue Swirl]

On 11/6/07, Paul Brook <paul@codesourcery.com> wrote:
> > > 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.

> 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.

Re: [Qemu-devel] [PATCH] sparc32 boot mode flag fix

[Robert Reif]

>>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.