ppc: CPU reset must flush translation buffer

Without that, reset from SLOF crashes in full emulation.

Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 target-ppc/translate_init.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

On 21.05.2012, at 04:01, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Without that, reset from SLOF crashes in full emulation.
> 
> Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> target-ppc/translate_init.c |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index ae03065..fbf7705 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10285,6 +10285,7 @@ static void ppc_cpu_reset(CPUState *s)
>     env->error_code = 0;
>     /* Flush all TLBs */
>     tlb_flush(env, 1);
> +    tb_flush(env);

Shouldn't this be true for all CPUs? I remember talking about reset with Peter a while ago... but don't remember the conclusions :)


Alex

On Mon, 2012-05-21 at 08:16 +0200, Alexander Graf wrote:
> 
> On 21.05.2012, at 04:01, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > Without that, reset from SLOF crashes in full emulation.
> > 
> > Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > target-ppc/translate_init.c |    1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-ppc/translate_init.c
> b/target-ppc/translate_init.c
> > index ae03065..fbf7705 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -10285,6 +10285,7 @@ static void ppc_cpu_reset(CPUState *s)
> >     env->error_code = 0;
> >     /* Flush all TLBs */
> >     tlb_flush(env, 1);
> > +    tb_flush(env);
> 
> Shouldn't this be true for all CPUs? I remember talking about reset
> with Peter a while ago... but don't remember the conclusions :)

Possibly. I noticed other targets do that too (ARM iirc), in this case I
think it's the ROM being reloaded that doesn't flush the cached
translations for the vectors (I -think-, that's from memory). But there
could be all sort of other context changes, so it seems like the safest
thing to do.

Cheers,
Ben.

On 21 May 2012 07:16, Alexander Graf <agraf@suse.de> wrote:
> Shouldn't this be true for all CPUs? I remember talking about reset
> with Peter a while ago... but don't remember the conclusions :)

The conclusion we came to is that you only need to tb_flush
in your CPU's reset function if you have some CPU state which
you handle by baking it into translated code and doing a tb_flush
when the state changes. This is relatively rare, most CPU
frontends only use the other options:
 (a) CPU state is constant for life of simulation
 (b) CPU state not baked into code
 (c) CPU state encoded in tb_flags.

In particular, target-ppc doesn't have any uses of tb_flush
at the moment, so either this fix is insufficient (and you need
to also use tb_flush at the point where the relevant state is
changed by whatever helper function) or it's the wrong fix.

If the issue is ROM reloading then the loading code needs to
be fixed (compare the way that the memory region API correctly
handles bits of physical memory being mapped/unmapped/remapped
without the caller needing to do a tb_reset).

-- PMM

On Mon, 2012-05-21 at 08:15 +0100, Peter Maydell wrote:
> The conclusion we came to is that you only need to tb_flush
> in your CPU's reset function if you have some CPU state which
> you handle by baking it into translated code and doing a tb_flush
> when the state changes. This is relatively rare, most CPU
> frontends only use the other options:
>  (a) CPU state is constant for life of simulation
>  (b) CPU state not baked into code
>  (c) CPU state encoded in tb_flags.
> 
> In particular, target-ppc doesn't have any uses of tb_flush
> at the moment, so either this fix is insufficient (and you need
> to also use tb_flush at the point where the relevant state is
> changed by whatever helper function) or it's the wrong fix.
> 
> If the issue is ROM reloading then the loading code needs to
> be fixed (compare the way that the memory region API correctly
> handles bits of physical memory being mapped/unmapped/remapped
> without the caller needing to do a tb_reset).

Hrm, the state shouldn't change in a drastic way.... we can reproduce
from SLOF which is in real mode and the reset happens in real mode... it
looks like a flush of the exception vectors problem to me.

So that would mean that the ROM reload isn't flushing properly (well,
possibly, need to investigate more). From what I can tell the reload is
done implicitely by generic qemu code creating rom objects when I call
load_image_targphys.

So if something is missing here it's from the generic code, I will dig a
bit more later, gotta take care of sick kids...

Cheers,
Ben.