Message ID | 1337565668.2458.14.camel@pasglop |
---|---|
State | New |
Headers | show |
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.
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); } static void ppc_cpu_initfn(Object *obj)
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(-)