Message ID | 4DE345B0.8020505@in.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Suzuki Poulose wrote: > Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S > =================================================================== > --- /dev/null > +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S .... > + * > + */ > + bl nxtins /* Find our address */ > +nxtins: mflr r5 /* Make it accessible */ Please don't mix labels and instructions. > + tlbsx r23,0,r5 /* Find entry we are in */ using tabs instead of spaces would make it look nice. Please also separate the arguments of the instruction i.e. tlbsx r23, 0, r5 > Index: powerpc/arch/powerpc/kernel/misc_32.S > =================================================================== > --- powerpc.orig/arch/powerpc/kernel/misc_32.S > +++ powerpc/arch/powerpc/kernel/misc_32.S > @@ -736,6 +736,28 @@ relocate_new_kernel: > mr r5, r31 > > li r0, 0 > +#elif defined(CONFIG_44x) && !defined(CONFIG_47x) > + > + mr r29, r3 > + mr r30, r4 > + mr r31, r5 > + > +#include "44x_kexec_mapping.S" The way you setup the 1:1 mapping should be close to what you are doing on kernel entry. Isn't it possible to include the file here and in the entry code? Sebastian
On 05/31/11 20:45, Sebastian Andrzej Siewior wrote: > > Suzuki Poulose wrote: >> Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S >> =================================================================== >> --- /dev/null >> +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S > .... > >> + * >> + */ >> + bl nxtins /* Find our address */ >> +nxtins: mflr r5 /* Make it accessible */ > > Please don't mix labels and instructions. OK. > >> + tlbsx r23,0,r5 /* Find entry we are in */ > > using tabs instead of spaces would make it look nice. Please also separate the arguments of the instruction i.e. > tlbsx r23, 0, r5 Sure. > > >> Index: powerpc/arch/powerpc/kernel/misc_32.S >> =================================================================== >> --- powerpc.orig/arch/powerpc/kernel/misc_32.S >> +++ powerpc/arch/powerpc/kernel/misc_32.S >> @@ -736,6 +736,28 @@ relocate_new_kernel: >> mr r5, r31 >> >> li r0, 0 >> +#elif defined(CONFIG_44x) && !defined(CONFIG_47x) >> + >> + mr r29, r3 >> + mr r30, r4 >> + mr r31, r5 >> + >> +#include "44x_kexec_mapping.S" > > The way you setup the 1:1 mapping should be close to what you are doing on > kernel entry.Isn't it possible to include the file here and in the entry > code? I will make this change and resend the patch. Thanks Suzuki
On Tue, 2011-05-31 at 17:15 +0200, Sebastian Andrzej Siewior wrote: > Suzuki Poulose wrote: > > Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S > > =================================================================== > > --- /dev/null > > +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S > .... > > > + * > > + */ > > + bl nxtins /* Find our address */ > > +nxtins: mflr r5 /* Make it accessible */ > > Please don't mix labels and instructions. With proper indent it's fine as long as he uses numerical relative labels which should be the case here. For example, the above, it should look like: bl 1f 1: mflr r5 > > + tlbsx r23,0,r5 /* Find entry we are in */ > > using tabs instead of spaces would make it look nice. Please also separate > the arguments of the instruction i.e. > tlbsx r23, 0, r5 That's arguable. If you look at arch/powerpc, we tend not to separate the arguments ;-) Actually I used to, others didn't and I changed my own style. > > Index: powerpc/arch/powerpc/kernel/misc_32.S > > =================================================================== > > --- powerpc.orig/arch/powerpc/kernel/misc_32.S > > +++ powerpc/arch/powerpc/kernel/misc_32.S > > @@ -736,6 +736,28 @@ relocate_new_kernel: > > mr r5, r31 > > > > li r0, 0 > > +#elif defined(CONFIG_44x) && !defined(CONFIG_47x) > > + > > + mr r29, r3 > > + mr r30, r4 > > + mr r31, r5 > > + > > +#include "44x_kexec_mapping.S" > > The way you setup the 1:1 mapping should be close to what you are doing on > kernel entry. Isn't it possible to include the file here and in the entry > code? It should just not be #included, what about branching out instead ? Cheers, Ben.
On 06/02/11 12:55, Benjamin Herrenschmidt wrote: > On Tue, 2011-05-31 at 17:15 +0200, Sebastian Andrzej Siewior wrote: >> Suzuki Poulose wrote: >>> Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S >>> =================================================================== >>> --- /dev/null >>> +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S >> .... >> >>> + * >>> + */ >>> + bl nxtins /* Find our address */ >>> +nxtins: mflr r5 /* Make it accessible */ >> >> Please don't mix labels and instructions. > > With proper indent it's fine as long as he uses numerical relative > labels which should be the case here. For example, the above, it should > look like: > > bl 1f > 1: mflr r5 > >>> + tlbsx r23,0,r5 /* Find entry we are in */ >> >> using tabs instead of spaces would make it look nice. Please also separate >> the arguments of the instruction i.e. >> tlbsx r23, 0, r5 > > That's arguable. If you look at arch/powerpc, we tend not to separate > the arguments ;-) > > Actually I used to, others didn't and I changed my own style. > >>> Index: powerpc/arch/powerpc/kernel/misc_32.S >>> =================================================================== >>> --- powerpc.orig/arch/powerpc/kernel/misc_32.S >>> +++ powerpc/arch/powerpc/kernel/misc_32.S >>> @@ -736,6 +736,28 @@ relocate_new_kernel: >>> mr r5, r31 >>> >>> li r0, 0 >>> +#elif defined(CONFIG_44x)&& !defined(CONFIG_47x) >>> + >>> + mr r29, r3 >>> + mr r30, r4 >>> + mr r31, r5 >>> + >>> +#include "44x_kexec_mapping.S" >> >> The way you setup the 1:1 mapping should be close to what you are doing on >> kernel entry. Isn't it possible to include the file here and in the entry >> code? > > It should just not be #included, what about branching out instead ? This code, i.e, relocate_new_kernel is copied into the control buffer, which will get the first chance to execute before the purgatory. So we may not be able to branch to the code, since we are executing this from a different address and we would invalidate the mapping for the code except the control buffer. Thanks Suzuki
On 06/02/11 12:04, Suzuki Poulose wrote: > On 05/31/11 20:45, Sebastian Andrzej Siewior wrote: >> >> Suzuki Poulose wrote: >>> Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S >>> =================================================================== >>> --- /dev/null >>> +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S >> .... >> >>> + * >>> + */ >>> + bl nxtins /* Find our address */ >>> +nxtins: mflr r5 /* Make it accessible */ >> >> Please don't mix labels and instructions. > OK. >> >>> + tlbsx r23,0,r5 /* Find entry we are in */ >> >> using tabs instead of spaces would make it look nice. Please also separate the arguments of the instruction i.e. >> tlbsx r23, 0, r5 > > Sure. >> >> >>> Index: powerpc/arch/powerpc/kernel/misc_32.S >>> =================================================================== >>> --- powerpc.orig/arch/powerpc/kernel/misc_32.S >>> +++ powerpc/arch/powerpc/kernel/misc_32.S >>> @@ -736,6 +736,28 @@ relocate_new_kernel: >>> mr r5, r31 >>> >>> li r0, 0 >>> +#elif defined(CONFIG_44x) && !defined(CONFIG_47x) >>> + >>> + mr r29, r3 >>> + mr r30, r4 >>> + mr r31, r5 >>> + >>> +#include "44x_kexec_mapping.S" >> >> The way you setup the 1:1 mapping should be close to what you are doing on >> kernel entry.Isn't it possible to include the file here and in the entry >> code? > I will make this change and resend the patch. I took a look at the way we do it at kernel entry. It looks more cleaner to leave it untouched. Especially, when we add the support for 47x in the future, the code will become more unreadable. What do you think ? Thanks Suzuki > > Thanks > > Suzuki > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
Suzuki Poulose wrote: >>> The way you setup the 1:1 mapping should be close to what you are >>> doing on >>> kernel entry.Isn't it possible to include the file here and in the entry >>> code? > >> I will make this change and resend the patch. > > I took a look at the way we do it at kernel entry. It looks more cleaner > to leave > it untouched. Especially, when we add the support for 47x in the future, > the code > will become more unreadable. > > What do you think ? So the entry code has one 256MiB mapping, you need 8 of those. Entry goes for TLB 63 and you need to be flexible and avoid TLB 63 :). So after all you don't have that much in common with the entry code. If you look at the FSL-book code then you will notice that I tried to share some code. I don't understand why you don't flip the address space bit. On fsl we setup the tmp mapping in the "other address" space so we don't have two mappings for the same address. The entry code could be doing this with STS bit, I'm not sure. If you want to keep your tiny mmu flip code instead of merging with the entry code then please don't put it into a separate file. The only reason why I did it, is to have the code once since I can't branch to it and I wanted to share code. > > Thanks > > Suzuki Sebastian
On 06/03/11 19:23, Sebastian Andrzej Siewior wrote: > Suzuki Poulose wrote: >>>> The way you setup the 1:1 mapping should be close to what you are doing on >>>> kernel entry.Isn't it possible to include the file here and in the entry >>>> code? >> >>> I will make this change and resend the patch. >> >> I took a look at the way we do it at kernel entry. It looks more cleaner to leave >> it untouched. Especially, when we add the support for 47x in the future, the code >> will become more unreadable. >> >> What do you think ? > > So the entry code has one 256MiB mapping, you need 8 of those. Entry goes for TLB 63 and you need to be flexible and avoid TLB 63 :). > So after all you don't have that much in common with the entry code. If > you look at the FSL-book code then you will notice that I tried to share > some code. > > I don't understand why you don't flip the address space bit. On fsl we > setup the tmp mapping in the "other address" space so we don't have two > mappings for the same address. The entry code could be doing this with STS > bit, I'm not sure. I am not sure if I understood this correctly. Could you explain how could there be two mappings for the same address ? We are setting up 1:1 mapping for 0-2GiB and the only mapping that could exist (in other words, not invalidated) is PAGE_OFFSET mapping. Since PAGE_OFFSET < 2GiB we won't have multiple mappings. Or in other words we could limit KEXEC_*_MEMORY_LIMIT to PAGE_OFFSET to make sure the crossing doesn't occur. The kernel entry code sets up the mapping without a tmp mapping in 44x. i.e, it uses the mapping setup by the firmware/boot loader. Thanks Suzuki
Index: powerpc/arch/powerpc/Kconfig =================================================================== --- powerpc.orig/arch/powerpc/Kconfig +++ powerpc/arch/powerpc/Kconfig @@ -349,7 +349,7 @@ config ARCH_ENABLE_MEMORY_HOTREMOVE config KEXEC bool "kexec system call (EXPERIMENTAL)" - depends on (PPC_BOOK3S || FSL_BOOKE) && EXPERIMENTAL + depends on (PPC_BOOK3S || FSL_BOOKE || (44x && !SMP && !47x)) && EXPERIMENTAL help kexec is a system call that implements the ability to shutdown your current kernel, and to start another kernel. It is like a reboot Index: powerpc/arch/powerpc/include/asm/kexec.h =================================================================== --- powerpc.orig/arch/powerpc/include/asm/kexec.h +++ powerpc/arch/powerpc/include/asm/kexec.h @@ -2,7 +2,7 @@ #define _ASM_POWERPC_KEXEC_H #ifdef __KERNEL__ -#ifdef CONFIG_FSL_BOOKE +#if defined(CONFIG_FSL_BOOKE) || defined(CONFIG_44x) /* * On FSL-BookE we setup a 1:1 mapping which covers the first 2GiB of memory Index: powerpc/arch/powerpc/kernel/44x_kexec_mapping.S =================================================================== --- /dev/null +++ powerpc/arch/powerpc/kernel/44x_kexec_mapping.S @@ -0,0 +1,66 @@ +/* + * Copyright (C) 2011 IBM + * + * Author : Suzuki K. Poulose <suzuki@in.ibm.com> + * + * This source code is licensed under the GNU General Public License, + * Version 2. See the file COPYING for more details. + * + * Code for setting up 1:1 mapping for PPC440x for KEXEC + * + * We cannot switch off the MMU on PPC44x. + * So we: + * 1) Invalidate all the mappings except the one we are + * running from. + * 2) Create a 1:1 mapping for 0-2GiB + * + * - Based on the kexec support code for FSL BookE + * - Based on the boot up code for ppc44x from head_44x.S + * - Doesn't support 47x yet. + * - Doesn't support SMP. + * + */ + bl nxtins /* Find our address */ +nxtins: mflr r5 /* Make it accessible */ + tlbsx r23,0,r5 /* Find entry we are in */ + li r4,0 /* Start at TLB entry 0 */ + li r3,0 /* Set PAGEID inval value */ +1: cmpw r23,r4 /* Is this our entry? */ + beq skip /* If so, skip the inval */ + tlbwe r3,r4,PPC44x_TLB_PAGEID /* If not, inval the entry */ +skip: addi r4,r4,1 /* Increment */ + cmpwi r4,64 /* Are we done? */ + bne 1b /* If not, repeat */ + isync + +/* + * Setup 1:1 mapping for 0-2GiB in blocks of 256M. + * r23 is the index to the mapping of this code. + * Skip it. + */ + /* attribute fields. rwx for SUPERVISOR mode */ + li r5, 0 + ori r5, r5, (PPC44x_TLB_SW | PPC44x_TLB_SR | PPC44x_TLB_SX | PPC44x_TLB_G) + + li r11, 0 /* PageNumber */ + li r6, 0 /* TLB Index */ + +next_tlb: + rotlwi r3, r11, 28 /* Create EPN (bits 0-3) */ + mr r4, r3 /* RPN = EPN */ + ori r3, r3, PPC44x_TLB_VALID | PPC44x_TLB_256M /* SIZE = 256M, Valid */ + cmpw r6, r23 /* Is this our TLB entry ? */ + bne write_tlb + addi r6, r6, 1 /* Skip our entry */ + +write_tlb: + tlbwe r3, r6, PPC44x_TLB_PAGEID /* PageID field : EPN, V, SIZE */ + tlbwe r4, r6, PPC44x_TLB_XLAT /* Address translation : RPN */ + tlbwe r5, r6, PPC44x_TLB_ATTRIB /* Attributes */ + + addi r11, r11, 1 /* Increment PN */ + addi r6, r6, 1 /* Increment TLB Index */ + cmpwi r11, 8 /* Are we done ? */ + bne next_tlb + isync + Index: powerpc/arch/powerpc/kernel/misc_32.S =================================================================== --- powerpc.orig/arch/powerpc/kernel/misc_32.S +++ powerpc/arch/powerpc/kernel/misc_32.S @@ -736,6 +736,28 @@ relocate_new_kernel: mr r5, r31 li r0, 0 +#elif defined(CONFIG_44x) && !defined(CONFIG_47x) + + mr r29, r3 + mr r30, r4 + mr r31, r5 + +#include "44x_kexec_mapping.S" + + mr r3, r29 + mr r4, r30 + mr r5, r31 + + li r0, 0 + + /* Jump back to the 1:1 address */ + mr r8, r0 + mtspr SPRN_SRR1, r8 + addi r8, r4, 1f - relocate_new_kernel + mtspr SPRN_SRR0, r8 + sync + rfi +1: #else li r0, 0
KEXEC Support for ppc440X The patch adds kexec support for PPC440x based boards. This work is based on the KEXEC patches for FSL BookE. The FSL BookE patch and the code flow could be found at the link below: http://patchwork.ozlabs.org/patch/49359/ We invalidate all the TLB entries except the one this code is run from, and create a 1:1 mapping for 0-2GiB in blocks of 256M. Later we jump to the 1:1 mapping. I have tested this patches on Ebony and Sequoia boards. It would be great if somebody could test this on the other boards. Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com> --- arch/powerpc/Kconfig | 2 arch/powerpc/include/asm/kexec.h | 2 arch/powerpc/kernel/44x_kexec_mapping.S | 66 ++++++++++++++++++++++++++++++++ arch/powerpc/kernel/misc_32.S | 22 ++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-)