Re: PS3 early lock-up
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: [EMAIL PROTECTED] 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___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
Benjamin Herrenschmidt wrote: arch/powerpc/platfroms/ps3/htab.c:ps3_hpte_updatepp() uses `htab[slot].v'. 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). 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=linux/kernel/git/geoff/ps3-linux-patches.git;a=blob;f=ps3-wip/ps3-htab-rework.diff;hb=HEAD Unfortunately, this week Mokuno-san is on holiday, and I am busy preparing for SIGGRAPH (next week). - 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. If we can't remove the htab mapping with lv1_read_htab_entries(), I'll look into this way. -Geoff ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
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 [EMAIL PROTECTED] | 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 = 5100h | lpar = 5100h | va = adc7d4c2d000h | group = 6168h | bitmap = 0h | hpte.v = adc7d4c2d011h | hpte.r = 51000115h ^ | psize = 0h | slot = 6168h After manually reverting the offending commit, the system boots again. The only change is: | pa = 5100h | lpar = 5100h | va = adc7d4c2d000h | group = 6168h | bitmap = 0h | hpte.v = adc7d4c2d011h | hpte.r = 51000117h ^ | 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: PS3 early lock-up
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
