| Message ID | 1444844599-33161-1-git-send-email-julio@farjump.io |
|---|---|
| State | New |
| Headers | show |
Ping :) Le mer. 14 oct. 2015 19:43, Julio Guerra <julio@farjump.io> a écrit : > Fix the index used to read the IBAT's vector which results in IBAT0..3 > instead > of IBAT4..N. > > The bug appeared by saving/restoring contexts including IBATs values. > > Signed-off-by: Julio Guerra <julio@farjump.io> > --- > target-ppc/translate_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index b541473..76d9a02 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -305,7 +305,7 @@ static void spr_read_ibat (DisasContext *ctx, int > gprn, int sprn) > > static void spr_read_ibat_h (DisasContext *ctx, int gprn, int sprn) > { > - tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn > & 1][(sprn - SPR_IBAT4U) / 2])); > + tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn > & 1][((sprn - SPR_IBAT4U) / 2) + 4])); > } > > static void spr_write_ibatu (DisasContext *ctx, int sprn, int gprn) > -- > 2.5.2 > >
03.11.2015 11:00, Julio Guerra wrote: > Ping :) Well, I'm not sure what can I do with this. I've no idea what is IBAT to start with, so while technically the patch is a one-liner, I've no idea what it does and how trivial it is :) Maybe you can include some context which teaches me what it is all about, and in that case it becomes really trivial, or.. I dunno :) Thanks, /mjt > Le mer. 14 oct. 2015 19:43, Julio Guerra <julio@farjump.io <mailto:julio@farjump.io>> a écrit : > > Fix the index used to read the IBAT's vector which results in IBAT0..3 instead > of IBAT4..N. > > The bug appeared by saving/restoring contexts including IBATs values. > > Signed-off-by: Julio Guerra <julio@farjump.io <mailto:julio@farjump.io>> > --- > target-ppc/translate_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index b541473..76d9a02 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -305,7 +305,7 @@ static void spr_read_ibat (DisasContext *ctx, int gprn, int sprn) > > static void spr_read_ibat_h (DisasContext *ctx, int gprn, int sprn) > { > - tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn & 1][(sprn - SPR_IBAT4U) / 2])); > + tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn & 1][((sprn - SPR_IBAT4U) / 2) + 4])); > } > > static void spr_write_ibatu (DisasContext *ctx, int sprn, int gprn) > -- > 2.5.2 >
On 03/11/15 12:16, Michael Tokarev wrote: > 03.11.2015 11:00, Julio Guerra wrote: >> Ping :) > > Well, I'm not sure what can I do with this. I've no idea what is IBAT to start > with, so while technically the patch is a one-liner, I've no idea what it does > and how trivial it is :) > > Maybe you can include some context which teaches me what it is all about, and in > that case it becomes really trivial, or.. I dunno :) FWIW PPC has a set of IBAT and DBAT registers on chip, each of which indicates a large continuous physical/virtual memory mapping for Instruction and Data memory respectively. The idea is that the OS can use these to provide "fast" virtual to physical lookups instead of invoking a time-consuming hash lookup to provide the translation. From casual observation comparing with spr_write_ibatu_h() in the same file (which already includes the +4 offset that the patch is adding to spr_read_ibat_h()), it does look like a genuine bug. However it really needs someone who understands PPC architecture a bit more to give a RB to ensure this is doing the right thing. ATB, Mark.
Le mar. 3 nov. 2015 à 14:33, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> a écrit : > On 03/11/15 12:16, Michael Tokarev wrote: > > > 03.11.2015 11:00, Julio Guerra wrote: > >> Ping :) > > > > Well, I'm not sure what can I do with this. I've no idea what is IBAT > to start > > with, so while technically the patch is a one-liner, I've no idea what > it does > > and how trivial it is :) > > > > Maybe you can include some context which teaches me what it is all > about, and in > > that case it becomes really trivial, or.. I dunno :) > > FWIW PPC has a set of IBAT and DBAT registers on chip, each of which > indicates a large continuous physical/virtual memory mapping for > Instruction and Data memory respectively. The idea is that the OS can > use these to provide "fast" virtual to physical lookups instead of > invoking a time-consuming hash lookup to provide the translation. > > From casual observation comparing with spr_write_ibatu_h() in the same > file (which already includes the +4 offset that the patch is adding to > spr_read_ibat_h()), it does look like a genuine bug. However it really > needs someone who understands PPC architecture a bit more to give a RB > to ensure this is doing the right thing. > > I would add the reason the bug never appeared is probably due to the fact BATs are not likely to be read by kernels, they simply write to them to program a large memory mapping. In our case, we saw the bug when fully saving/restoring the CPU context since we were in fact reading at BAT0-3 instead of BAT4-7 and then restoring BAT these values in BAT4-7... And the result can be very perverse... Linux PPC, which I think is how Alexander Graf tests qemu-ppc, probably does not use these higher BAT registers, they are CPU-specific.
14.10.2015 20:43, Julio Guerra wrote: > Fix the index used to read the IBAT's vector which results in IBAT0..3 instead > of IBAT4..N. > > The bug appeared by saving/restoring contexts including IBATs values. Applied to -trivial. It'd be *much* better if such changes were accepted by the maintainers who at least knows what's the talk about... :( Thanks, /mjt
On Fri, Nov 06, 2015 at 11:04:39AM +0300, Michael Tokarev wrote: > 14.10.2015 20:43, Julio Guerra wrote: > > Fix the index used to read the IBAT's vector which results in IBAT0..3 instead > > of IBAT4..N. > > > > The bug appeared by saving/restoring contexts including IBATs values. > > Applied to -trivial. It'd be *much* better if such changes were > accepted by the maintainers who at least knows what's the talk > about... :( Thanks and sorry about this. Alex Graf who usually handles most ppc stuff is off on paternity leave. I've been filling in for him, but I missed this one because I'm a bit swamped with my day job.
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index b541473..76d9a02 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -305,7 +305,7 @@ static void spr_read_ibat (DisasContext *ctx, int gprn, int sprn) static void spr_read_ibat_h (DisasContext *ctx, int gprn, int sprn) { - tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn & 1][(sprn - SPR_IBAT4U) / 2])); + tcg_gen_ld_tl(cpu_gpr[gprn], cpu_env, offsetof(CPUPPCState, IBAT[sprn & 1][((sprn - SPR_IBAT4U) / 2) + 4])); } static void spr_write_ibatu (DisasContext *ctx, int sprn, int gprn)
Fix the index used to read the IBAT's vector which results in IBAT0..3 instead of IBAT4..N. The bug appeared by saving/restoring contexts including IBATs values. Signed-off-by: Julio Guerra <julio@farjump.io> --- target-ppc/translate_init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)