diff mbox series

[qemu,v6,1/6] ppc: Start CPU in the default mode which is big-endian 32bit

Message ID 20200203032943.121178-2-aik@ozlabs.ru
State New
Headers show
Series [qemu,v6,1/6] ppc: Start CPU in the default mode which is big-endian 32bit | expand

Commit Message

Alexey Kardashevskiy Feb. 3, 2020, 3:29 a.m. UTC
At the moment we enforce 64bit mode on a CPU when reset. This does not
make difference as SLOF or Linux set the desired mode straight away.
However if we ever boot something other than these two,
this might not work as, for example, GRUB expects the default MSR state
and does not work properly.

This removes setting MSR_SF from the PPC CPU reset.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target/ppc/translate_init.inc.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

David Gibson Feb. 12, 2020, 5:43 a.m. UTC | #1
On Mon, Feb 03, 2020 at 02:29:38PM +1100, Alexey Kardashevskiy wrote:
> At the moment we enforce 64bit mode on a CPU when reset. This does not
> make difference as SLOF or Linux set the desired mode straight away.
> However if we ever boot something other than these two,
> this might not work as, for example, GRUB expects the default MSR state
> and does not work properly.
> 
> This removes setting MSR_SF from the PPC CPU reset.

Hrm.  This is in the core cpu model so it doesn't just affect pseries,
but powernv (and theoretically others) as well.  Generally the cpu
model should have the bare metal behaviour, and we can override it in
the pseries machine if necessary.

So for a bare metal POWER system, what mode do we start in?  I'm
guessing it probably doesn't matter in practice, since the skiboot
firmware also probably does a mode set on entry, but it'd be nice to
get this right in theory.

> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  target/ppc/translate_init.inc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 53995f62eab2..f6a676cf55e8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10710,12 +10710,6 @@ static void ppc_cpu_reset(CPUState *s)
>  #endif
>  #endif
>  
> -#if defined(TARGET_PPC64)
> -    if (env->mmu_model & POWERPC_MMU_64) {
> -        msr |= (1ULL << MSR_SF);
> -    }
> -#endif
> -
>      hreg_store_msr(env, msr, 1);
>  
>  #if !defined(CONFIG_USER_ONLY)
Alexey Kardashevskiy Feb. 13, 2020, 3:09 a.m. UTC | #2
On 12/02/2020 16:43, David Gibson wrote:
> On Mon, Feb 03, 2020 at 02:29:38PM +1100, Alexey Kardashevskiy wrote:
>> At the moment we enforce 64bit mode on a CPU when reset. This does not
>> make difference as SLOF or Linux set the desired mode straight away.
>> However if we ever boot something other than these two,
>> this might not work as, for example, GRUB expects the default MSR state
>> and does not work properly.
>>
>> This removes setting MSR_SF from the PPC CPU reset.
> 
> Hrm.  This is in the core cpu model so it doesn't just affect pseries,
> but powernv (and theoretically others) as well.  Generally the cpu
> model should have the bare metal behaviour, and we can override it in
> the pseries machine if necessary.
> 
> So for a bare metal POWER system, what mode do we start in?  I'm
> guessing it probably doesn't matter in practice, since the skiboot
> firmware also probably does a mode set on entry, but it'd be nice to
> get this right in theory.


Huh. "Figure 65.  MSR setting due to interrupt" of PowerISA 3.0 says
"The SF bit is set to 1" so after all the existing behavior is correct
and my patch is just wrong. Cool.



> 
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  target/ppc/translate_init.inc.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>> index 53995f62eab2..f6a676cf55e8 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -10710,12 +10710,6 @@ static void ppc_cpu_reset(CPUState *s)
>>  #endif
>>  #endif
>>  
>> -#if defined(TARGET_PPC64)
>> -    if (env->mmu_model & POWERPC_MMU_64) {
>> -        msr |= (1ULL << MSR_SF);
>> -    }
>> -#endif
>> -
>>      hreg_store_msr(env, msr, 1);
>>  
>>  #if !defined(CONFIG_USER_ONLY)
>
David Gibson Feb. 13, 2020, 3:34 a.m. UTC | #3
On Thu, Feb 13, 2020 at 02:09:17PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 12/02/2020 16:43, David Gibson wrote:
> > On Mon, Feb 03, 2020 at 02:29:38PM +1100, Alexey Kardashevskiy wrote:
> >> At the moment we enforce 64bit mode on a CPU when reset. This does not
> >> make difference as SLOF or Linux set the desired mode straight away.
> >> However if we ever boot something other than these two,
> >> this might not work as, for example, GRUB expects the default MSR state
> >> and does not work properly.
> >>
> >> This removes setting MSR_SF from the PPC CPU reset.
> > 
> > Hrm.  This is in the core cpu model so it doesn't just affect pseries,
> > but powernv (and theoretically others) as well.  Generally the cpu
> > model should have the bare metal behaviour, and we can override it in
> > the pseries machine if necessary.
> > 
> > So for a bare metal POWER system, what mode do we start in?  I'm
> > guessing it probably doesn't matter in practice, since the skiboot
> > firmware also probably does a mode set on entry, but it'd be nice to
> > get this right in theory.
> 
> 
> Huh. "Figure 65.  MSR setting due to interrupt" of PowerISA 3.0 says
> "The SF bit is set to 1" so after all the existing behavior is correct
> and my patch is just wrong. Cool.

Well, I guess SF after interrupt isn't *necessarily* the same as SF at
reset, but it's a good place to start.
diff mbox series

Patch

diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 53995f62eab2..f6a676cf55e8 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10710,12 +10710,6 @@  static void ppc_cpu_reset(CPUState *s)
 #endif
 #endif
 
-#if defined(TARGET_PPC64)
-    if (env->mmu_model & POWERPC_MMU_64) {
-        msr |= (1ULL << MSR_SF);
-    }
-#endif
-
     hreg_store_msr(env, msr, 1);
 
 #if !defined(CONFIG_USER_ONLY)