PS3 early lock-up

PS3 early lock-up

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2591 bytes --]

On PS3, recent kernels lock up in the very early stage (i.e. before mere
mortals get to see a working console). The kernel crashes with

| kernel BUG at linux/arch/powerpc/platforms/ps3/htab.c:141!

Bisecting shows this happens since:

| commit a1f242ff460e4b50a045fa237c3c56cce9eabf83
| Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
| Date:   Wed Jul 23 21:27:08 2008 -0700
| 
|     powerpc ioremap_prot
|     
|     This adds ioremap_prot and pte_pgprot() so that one can extract protection
|     bits from a PTE and use them to ioremap_prot() (in order to support ptrace
|     of VM_IO | VM_PFNMAP as per Rik's patch).
|     
|     This moves a couple of flag checks around in the ioremap implementations
|     of arch/powerpc.  There's a side effect of allowing non-cacheable and
|     non-guarded mappings on ppc32 which before would always have _PAGE_GUARDED
|     set whenever _PAGE_NO_CACHE is.
|     
|     (standard ioremap will still set _PAGE_GUARDED, but ioremap_prot will be
|     capable of setting such a non guarded mapping).
  
Inside ps3_hpte_insert(), lv1_write_htab_entry() fails with error code 5
(LV1_ACCESS_VIOLATION) when adding the PS3 hotplug memory.

debug_dump_hpte() prints for the offending hpte:

| pa     = 500001000000h
| lpar   = 500001000000h
| va     = adc7d4c2d0000000h
| group  = 6168h
| bitmap = 0h
| hpte.v = adc7d4c2d011h
| hpte.r = 500001000115h
                      ^
| psize  = 0h
| slot   = 6168h

After manually reverting the offending commit, the system boots again. The only
change is:

| pa     = 500001000000h
| lpar   = 500001000000h
| va     = adc7d4c2d0000000h
| group  = 6168h
| bitmap = 0h
| hpte.v = adc7d4c2d011h
| hpte.r = 500001000117h
                      ^
| psize  = 0h
| slot   = 6168h

Note that when adding the initial (non-hotplug) memory, hpte.r always ends in
`194', both before and after reverting the offending commit.

ps3_hpte_insert() seems to be called during system initialization with the
following values of rflags:
  - first call: 0x190
  - initial memory: 0x194 (455 times)
  - hotplug memory:
      o crash: 0x115
      o OK: 0x117

Do you have an idea of what's really going on?

Thanks!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: PS3 early lock-up

[Benjamin Herrenschmidt]

On Mon, 2008-08-04 at 17:48 +0200, Geert Uytterhoeven wrote:
> On PS3, recent kernels lock up in the very early stage (i.e. before mere
> mortals get to see a working console). The kernel crashes with
> 
> | kernel BUG at linux/arch/powerpc/platforms/ps3/htab.c:141!
> 
> Bisecting shows this happens since:
> 
> | commit a1f242ff460e4b50a045fa237c3c56cce9eabf83
> | Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> | Date:   Wed Jul 23 21:27:08 2008 -0700
> | 
> |     powerpc ioremap_prot
> |     
> |     This adds ioremap_prot and pte_pgprot() so that one can extract protection
> |     bits from a PTE and use them to ioremap_prot() (in order to support ptrace
> |     of VM_IO | VM_PFNMAP as per Rik's patch).
> |     
> |     This moves a couple of flag checks around in the ioremap implementations
> |     of arch/powerpc.  There's a side effect of allowing non-cacheable and
> |     non-guarded mappings on ppc32 which before would always have _PAGE_GUARDED
> |     set whenever _PAGE_NO_CACHE is.
> |     
> |     (standard ioremap will still set _PAGE_GUARDED, but ioremap_prot will be
> |     capable of setting such a non guarded mapping).
>   
> Inside ps3_hpte_insert(), lv1_write_htab_entry() fails with error code 5
> (LV1_ACCESS_VIOLATION) when adding the PS3 hotplug memory.
> 
> debug_dump_hpte() prints for the offending hpte:
> 
> | pa     = 500001000000h
> | lpar   = 500001000000h
> | va     = adc7d4c2d0000000h
> | group  = 6168h
> | bitmap = 0h
> | hpte.v = adc7d4c2d011h
> | hpte.r = 500001000115h
>                       ^
> | psize  = 0h
> | slot   = 6168h
> 
> After manually reverting the offending commit, the system boots again. The only
> change is:
> 
> | pa     = 500001000000h
> | lpar   = 500001000000h
> | va     = adc7d4c2d0000000h
> | group  = 6168h
> | bitmap = 0h
> | hpte.v = adc7d4c2d011h
> | hpte.r = 500001000117h
>                       ^
> | psize  = 0h
> | slot   = 6168h
> 
> Note that when adding the initial (non-hotplug) memory, hpte.r always ends in
> `194', both before and after reverting the offending commit.
> 
> ps3_hpte_insert() seems to be called during system initialization with the
> following values of rflags:
>   - first call: 0x190
>   - initial memory: 0x194 (455 times)
>   - hotplug memory:
>       o crash: 0x115
>       o OK: 0x117
> 
> Do you have an idea of what's really going on?

Weird... Both look incorrect. In fact, it's a bit scary...

The one with the 7 at the end means that user space as RO access to
the segment (oops !) and supervisor too. The one with the 5 means
RO for user and RW for supervisor.

That is unless your HV is munging them in strange ways... I don't
know why LV1 is refusing a combination though.

As for the flags, it depends what htab_bolt_mapping() is called
with.

Do you have a backtrace ? I'm a bit lots in the mem hotswap code
trying to figure out where the mapping comes from..

Ben.

Re: PS3 early lock-up

[Benjamin Herrenschmidt]

> > ps3_hpte_insert() seems to be called during system initialization with the
> > following values of rflags:
> >   - first call: 0x190
> >   - initial memory: 0x194 (455 times)
> >   - hotplug memory:
> >       o crash: 0x115
> >       o OK: 0x117
> > 
> > Do you have an idea of what's really going on?
> 
> Weird... Both look incorrect. In fact, it's a bit scary...
> 
> The one with the 7 at the end means that user space as RO access to
> the segment (oops !) and supervisor too. The one with the 5 means
> RO for user and RW for supervisor.
> 
> That is unless your HV is munging them in strange ways... I don't
> know why LV1 is refusing a combination though.
> 
> As for the flags, it depends what htab_bolt_mapping() is called
> with.
> 
> Do you have a backtrace ? I'm a bit lots in the mem hotswap code
> trying to figure out where the mapping comes from..

Ah, found it... It should be ok... both the mapping of the RAM itself
and vmemmap_populate() should be passing 

  _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX;

Which should be 0x194.

Can you find out where that stupid value comes from ?

Ben.

Re: PS3 early lock-up

[Geoff Levand]

Hi,

Benjamin Herrenschmidt wrote:
>> > ps3_hpte_insert() seems to be called during system initialization with the
>> > following values of rflags:
>> >   - first call: 0x190
>> >   - initial memory: 0x194 (455 times)
>> >   - hotplug memory:
>> >       o crash: 0x115
>> >       o OK: 0x117
>> > 
>> > Do you have an idea of what's really going on?
>> 
>> Weird... Both look incorrect. In fact, it's a bit scary...
>> 
>> The one with the 7 at the end means that user space as RO access to
>> the segment (oops !) and supervisor too. The one with the 5 means
>> RO for user and RW for supervisor.
>> 
>> That is unless your HV is munging them in strange ways... I don't
>> know why LV1 is refusing a combination though.
>> 
>> As for the flags, it depends what htab_bolt_mapping() is called
>> with.
>> 
>> Do you have a backtrace ? I'm a bit lots in the mem hotswap code
>> trying to figure out where the mapping comes from..
> 
> Ah, found it... It should be ok... both the mapping of the RAM itself
> and vmemmap_populate() should be passing 
> 
>   _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX;
> 
> Which should be 0x194.

That is 0x190.

0x194 = _PAGE_EXEC | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX

> 
> Can you find out where that stupid value comes from ?

I didn't have time to look at in detail, but it fails from the
ioremap call in ps3_map_htab (arch/powerpc/platfroms/ps3/htab.c):

 htab = (__force struct hash_pte *)ioremap_flags(htab_addr, htab_size,
     pgprot_val(PAGE_READONLY_X));

IIRC, lv1 doesn't allow a read/write mapping of the htab, and that is
why I used pgprot_val(PAGE_READONLY_X) here.

I guess the value returned from pgprot_val(PAGE_READONLY_X)
changed in recent kernels, and that is what is causing the failure.

Just FYI, I put these in:

  printk("%s:%d: flags = %x\n", __func__, __LINE__, (_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX));
  printk("%s:%d: flags = %x\n", __func__, __LINE__, pgprot_val(PAGE_READONLY_X));

and got this (and lv1_write_htab_entry failed):

  ps3_map_htab:288: flags = 190
  ps3_map_htab:289: flags = 117

-Geoff

Re: PS3 early lock-up

[Benjamin Herrenschmidt]

> > Which should be 0x194.
> 
> That is 0x190.
> 
> 0x194 = _PAGE_EXEC | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX

Right, _PAGE_EXEC should only be set for the part covering the kernel
text. In any case, it shouldn't be what you showed.
 
> > Can you find out where that stupid value comes from ?
> 
> I didn't have time to look at in detail, but it fails from the
> ioremap call in ps3_map_htab (arch/powerpc/platfroms/ps3/htab.c):
> 
>  htab = (__force struct hash_pte *)ioremap_flags(htab_addr, htab_size,
>      pgprot_val(PAGE_READONLY_X));
> 
> IIRC, lv1 doesn't allow a read/write mapping of the htab, and that is
> why I used pgprot_val(PAGE_READONLY_X) here.

Why are you mapping it in the first place btw ? Do you actually use that
mapping ?

> I guess the value returned from pgprot_val(PAGE_READONLY_X)
> changed in recent kernels, and that is what is causing the failure.

Ok, there's definitely something fishy about passing the PP bits down
to ioremap. The reason it fails is that I no longer let _PAGE_USER
go down to the mapping. However, ioremap passes those bits as-is
to the hash insert code, it should instead perform the same munging
as the asm hashing code does, to turn that into a supervisor only
mapping.

However, there is a deeper issue with what you are doing. With using
only 2 PP bits, as is the case with linux, you -cannot- have a supervisor
read-only mapping that isn't also readable by userspace. It's possible
the newer 3 PPP bits encoding, but I don't know if Cell supports it and
linux currently doesn't use it.

That means that currently, your hash table is user readable... oops :-)

(This is a bug with other early IO mappings too btw, I'll have
to fix that).

However, it appears to me that you don't use the mapping of the hash
table anyway. So just remove the ioremap :-) I'll look at fixing
the attribute parsing for ioremap_prot() in the long run though.

Ben.

> Just FYI, I put these in:
> 
>   printk("%s:%d: flags = %x\n", __func__, __LINE__, (_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_COHERENT | PP_RWXX));
>   printk("%s:%d: flags = %x\n", __func__, __LINE__, pgprot_val(PAGE_READONLY_X));
> 
> and got this (and lv1_write_htab_entry failed):
> 
>   ps3_map_htab:288: flags = 190
>   ps3_map_htab:289: flags = 117
> 
> -Geoff
> 
> 

Re: PS3 early lock-up

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1072 bytes --]

On Tue, 5 Aug 2008, Benjamin Herrenschmidt wrote:
> > > Can you find out where that stupid value comes from ?
> > 
> > I didn't have time to look at in detail, but it fails from the
> > ioremap call in ps3_map_htab (arch/powerpc/platfroms/ps3/htab.c):
> > 
> >  htab = (__force struct hash_pte *)ioremap_flags(htab_addr, htab_size,
> >      pgprot_val(PAGE_READONLY_X));
> > 
> > IIRC, lv1 doesn't allow a read/write mapping of the htab, and that is
> > why I used pgprot_val(PAGE_READONLY_X) here.
> 
> Why are you mapping it in the first place btw ? Do you actually use that
> mapping ?

arch/powerpc/platfroms/ps3/htab.c:ps3_hpte_updatepp() uses `htab[slot].v'.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB

Re: PS3 early lock-up

[Benjamin Herrenschmidt]

> arch/powerpc/platfroms/ps3/htab.c:ps3_hpte_updatepp() uses `htab[slot].v'.

Ah, I missed that one. Indeed it -is- used.

Ok, that leaves us with 2 options:

 - Change ps3_hpte_updatepp() to not read from the hash table via that
mapping (ie, do you have an LV1 call to read an HPTE ? Do you measure
any significant performance loss using that instead ? updatepp shouldn't
be something called -that- often).

 - Add a way to setup HPTEs using 3 PPP bits. I'm not going to implement
that for the main hash code just yet though (the assembly) but it might
be possible to implement it specifically for mappings bolted. That
means it would only work when the mapping is done early but on PS3, we
know that the hash table is always mapped early.

The later would be a matter of taking my htab_convert_pte_flags() function
and making it capable, when _PAGE_USER is _not_ set and _PAGE_RW is not
set neither, to set PPP to 110.

You could do that by adding:

	if (!(pteflags & (_PAGE_USER | _PAGE_RW)))
		rflags |= (1 << 1) | (1 << 63);

Dbl check that the resulting mapping isn't accessible to user space though.

Ben.

Re: PS3 early lock-up

[Benjamin Herrenschmidt]

> You could do that by adding:
> 
> 	if (!(pteflags & (_PAGE_USER | _PAGE_RW)))
> 		rflags |= (1 << 1) | (1 << 63);
> 
> Dbl check that the resulting mapping isn't accessible to user space though.

Make these 1UL << x, and a proper patch would have to also test
that the CPU supports the 3rd PP bit. We probably need to add
a CPU feature bit for that.

Ben.

Re: PS3 early lock-up

[Geoff Levand]

Benjamin Herrenschmidt wrote:
>> arch/powerpc/platfroms/ps3/htab.c:ps3_hpte_updatepp() uses `htab[slot]=
.v'.
>=20
> Ok, that leaves us with 2 options:
>=20
>  - Change ps3_hpte_updatepp() to not read from the hash table via that
> mapping (ie, do you have an LV1 call to read an HPTE ? Do you measure
> any significant performance loss using that instead ? updatepp shouldn'=
t
> be something called -that- often).

Yes, we have lv1_read_htab_entries().  Mokuno-san started some work to
convert to using it and removing the htab mapping:

  http://git.kernel.org/?p=3Dlinux/kernel/git/geoff/ps3-linux-patches.git=
;a=3Dblob;f=3Dps3-wip/ps3-htab-rework.diff;hb=3DHEAD

Unfortunately, this week Mokuno-san is on holiday, and I am busy preparin=
g
for SIGGRAPH (next week).

>  - Add a way to setup HPTEs using 3 PPP bits. I'm not going to implemen=
t
> that for the main hash code just yet though (the assembly) but it might
> be possible to implement it specifically for mappings bolted. That
> means it would only work when the mapping is done early but on PS3, we
> know that the hash table is always mapped early.
>=20
> The later would be a matter of taking my =EF=BB=BFhtab_convert_pte_flag=
s() function
> and making it capable, when _PAGE_USER is _not_ set and _PAGE_RW is not
> set neither, to set PPP to 110.
>=20
> You could do that by adding:
>=20
> 	if (!(pteflags & (_PAGE_USER | _PAGE_RW)))
> 		rflags |=3D (1 << 1) | (1 << 63);
>=20
> Dbl check that the resulting mapping isn't accessible to user space tho=
ugh.

If we can't remove the htab mapping with lv1_read_htab_entries(), I'll
look into this way.

-Geoff