[Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Julio Guerra]

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

Re: [Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Julio Guerra]

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

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

[-- Attachment #2: Type: text/html, Size: 1464 bytes --]

Re: [Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Michael Tokarev]

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
> 

Re: [Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Julio Guerra]

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

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.

[-- Attachment #2: Type: text/html, Size: 2186 bytes --]

Re: [Qemu-devel] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[Michael Tokarev]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] taget-ppc: Fix read access to IBAT registers higher than IBAT3

[David Gibson]

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

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]