| Message ID | 20210520032919.358935-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RFC,kernel] powerpc: Fix early setup to make early_ioremap work | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (3a81c0495fdb91fd9a9b4f617098c283131eeae1) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 1 errors, 0 warnings, 0 checks, 22 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hm, my thunderbird says it is not cc:'ed but git sendmail says it did cc: Server: localhost MAIL FROM:<aik@ozlabs.ru> RCPT TO:<linuxppc-dev@lists.ozlabs.org> RCPT TO:<aik@ozlabs.ru> RCPT TO:<mpe@ellerman.id.au> RCPT TO:<christophe.leroy@csgroup.eu> From: Alexey Kardashevskiy <aik@ozlabs.ru> To: linuxppc-dev@lists.ozlabs.org Cc: Alexey Kardashevskiy <aik@ozlabs.ru>, Michael Ellerman <mpe@ellerman.id.au>, Christophe Leroy <christophe.leroy@csgroup.eu> Subject: [RFC PATCH kernel] powerpc: Fix early setup to make early_ioremap work Not sure what to believe. On 20/05/2021 13:29, Alexey Kardashevskiy wrote: > The immediate problem is that after > 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") > the kernel silently reboots. The reason is that early_ioremap() returns > broken addresses as it uses slot_virt[] array which initialized with > offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == > KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 > when early_ioremap_setup() is called. __kernel_io_end is initialized > little bit later in early_init_mmu(). > > This fixes the initialization by swapping early_ioremap_setup and > early_init_mmu. > > This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, > seems to make sense, unless there is some weird logic with redefining > FIXADDR_SIZE as the compiling goes. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- > arch/powerpc/kernel/setup_64.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h > index a666d561b44d..54a06129794b 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; > #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) > #define IOREMAP_BASE (PHB_IO_END) > #define IOREMAP_START (ioremap_bot) > -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) > #define FIXADDR_SIZE SZ_32M > +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) > > /* Advertise special mapping type for AGP */ > #define HAVE_PAGE_AGP > diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c > index b779d25761cf..ce09fe5debf4 100644 > --- a/arch/powerpc/kernel/setup_64.c > +++ b/arch/powerpc/kernel/setup_64.c > @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) > apply_feature_fixups(); > setup_feature_keys(); > > - early_ioremap_setup(); > > /* Initialize the hash table or TLB handling */ > early_init_mmu(); > > + early_ioremap_setup(); > + > /* > * After firmware and early platform setup code has set things up, > * we note the SPR values for configurable control/performance >
Le 20/05/2021 à 05:33, Alexey Kardashevskiy a écrit : > Hm, my thunderbird says it is not cc:'ed but git sendmail says it did cc: > > > Server: localhost > MAIL FROM:<aik@ozlabs.ru> > RCPT TO:<linuxppc-dev@lists.ozlabs.org> > RCPT TO:<aik@ozlabs.ru> > RCPT TO:<mpe@ellerman.id.au> > RCPT TO:<christophe.leroy@csgroup.eu> > From: Alexey Kardashevskiy <aik@ozlabs.ru> > To: linuxppc-dev@lists.ozlabs.org > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>, > Michael Ellerman <mpe@ellerman.id.au>, > Christophe Leroy <christophe.leroy@csgroup.eu> > Subject: [RFC PATCH kernel] powerpc: Fix early setup to make early_ioremap work > > > Not sure what to believe. I got the patch. If you are looking at the mail you received from the ppc list, I think the list removes the members of the list from the Cc: in order to avoid the mail being received multiple times. Christophe > > > On 20/05/2021 13:29, Alexey Kardashevskiy wrote: >> The immediate problem is that after >> 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") >> the kernel silently reboots. The reason is that early_ioremap() returns >> broken addresses as it uses slot_virt[] array which initialized with >> offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == >> KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 >> when early_ioremap_setup() is called. __kernel_io_end is initialized >> little bit later in early_init_mmu(). >> >> This fixes the initialization by swapping early_ioremap_setup and >> early_init_mmu. >> >> This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, >> seems to make sense, unless there is some weird logic with redefining >> FIXADDR_SIZE as the compiling goes. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- >> arch/powerpc/kernel/setup_64.c | 3 ++- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h >> b/arch/powerpc/include/asm/book3s/64/pgtable.h >> index a666d561b44d..54a06129794b 100644 >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >> @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; >> #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) >> #define IOREMAP_BASE (PHB_IO_END) >> #define IOREMAP_START (ioremap_bot) >> -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >> #define FIXADDR_SIZE SZ_32M >> +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >> /* Advertise special mapping type for AGP */ >> #define HAVE_PAGE_AGP >> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c >> index b779d25761cf..ce09fe5debf4 100644 >> --- a/arch/powerpc/kernel/setup_64.c >> +++ b/arch/powerpc/kernel/setup_64.c >> @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) >> apply_feature_fixups(); >> setup_feature_keys(); >> - early_ioremap_setup(); >> /* Initialize the hash table or TLB handling */ >> early_init_mmu(); >> + early_ioremap_setup(); >> + >> /* >> * After firmware and early platform setup code has set things up, >> * we note the SPR values for configurable control/performance >> >
Le 20/05/2021 à 05:29, Alexey Kardashevskiy a écrit : > The immediate problem is that after > 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") > the kernel silently reboots. The reason is that early_ioremap() returns > broken addresses as it uses slot_virt[] array which initialized with > offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == > KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 > when early_ioremap_setup() is called. __kernel_io_end is initialized > little bit later in early_init_mmu(). > > This fixes the initialization by swapping early_ioremap_setup and > early_init_mmu. Hum ... Chris tested it on a T2080RDB, that must be a book3e. So we missed it. I guess your fix is right. > > This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, > seems to make sense, unless there is some weird logic with redefining > FIXADDR_SIZE as the compiling goes. Well, I don't think the order of defines matters, the change should be kept out of the fix. But if you want it anyway, then I'd suggest to move it before IOREMAP_BASE in order to keep the 3 IOREMAP_xxx together. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- > arch/powerpc/kernel/setup_64.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h > index a666d561b44d..54a06129794b 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; > #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) > #define IOREMAP_BASE (PHB_IO_END) > #define IOREMAP_START (ioremap_bot) > -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) > #define FIXADDR_SIZE SZ_32M > +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) > > /* Advertise special mapping type for AGP */ > #define HAVE_PAGE_AGP > diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c > index b779d25761cf..ce09fe5debf4 100644 > --- a/arch/powerpc/kernel/setup_64.c > +++ b/arch/powerpc/kernel/setup_64.c > @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) > apply_feature_fixups(); > setup_feature_keys(); > > - early_ioremap_setup(); > > /* Initialize the hash table or TLB handling */ > early_init_mmu(); > > + early_ioremap_setup(); > + > /* > * After firmware and early platform setup code has set things up, > * we note the SPR values for configurable control/performance >
On 20/05/2021 15:46, Christophe Leroy wrote: > > > Le 20/05/2021 à 05:29, Alexey Kardashevskiy a écrit : >> The immediate problem is that after >> 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") >> the kernel silently reboots. The reason is that early_ioremap() returns >> broken addresses as it uses slot_virt[] array which initialized with >> offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == >> KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 >> when early_ioremap_setup() is called. __kernel_io_end is initialized >> little bit later in early_init_mmu(). >> >> This fixes the initialization by swapping early_ioremap_setup and >> early_init_mmu. > > Hum ... Chris tested it on a T2080RDB, that must be a book3e. > > So we missed it. I guess your fix is right. Oh cool. >> >> This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, >> seems to make sense, unless there is some weird logic with redefining >> FIXADDR_SIZE as the compiling goes. > > Well, I don't think the order of defines matters, the change should be > kept out of the fix. When I see this: #define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) #define FIXADDR_SIZE SZ_32M ... I have to think harder what FIXADDR_SIZE was in the first macro and in what order the preprocessor expands them. > But if you want it anyway, then I'd suggest to move it before > IOREMAP_BASE in order to keep the 3 IOREMAP_xxx together. Up to Michael, I guess. > >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> --- >> arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- >> arch/powerpc/kernel/setup_64.c | 3 ++- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h >> b/arch/powerpc/include/asm/book3s/64/pgtable.h >> index a666d561b44d..54a06129794b 100644 >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >> @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; >> #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) >> #define IOREMAP_BASE (PHB_IO_END) >> #define IOREMAP_START (ioremap_bot) >> -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >> #define FIXADDR_SIZE SZ_32M >> +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >> /* Advertise special mapping type for AGP */ >> #define HAVE_PAGE_AGP >> diff --git a/arch/powerpc/kernel/setup_64.c >> b/arch/powerpc/kernel/setup_64.c >> index b779d25761cf..ce09fe5debf4 100644 >> --- a/arch/powerpc/kernel/setup_64.c >> +++ b/arch/powerpc/kernel/setup_64.c >> @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) >> apply_feature_fixups(); >> setup_feature_keys(); >> - early_ioremap_setup(); >> /* Initialize the hash table or TLB handling */ >> early_init_mmu(); >> + early_ioremap_setup(); >> + >> /* >> * After firmware and early platform setup code has set things up, >> * we note the SPR values for configurable control/performance >>
Le 20/05/2021 à 07:52, Alexey Kardashevskiy a écrit : > > > On 20/05/2021 15:46, Christophe Leroy wrote: >> >> >> Le 20/05/2021 à 05:29, Alexey Kardashevskiy a écrit : >>> The immediate problem is that after >>> 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") >>> the kernel silently reboots. The reason is that early_ioremap() returns >>> broken addresses as it uses slot_virt[] array which initialized with >>> offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == >>> KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 >>> when early_ioremap_setup() is called. __kernel_io_end is initialized >>> little bit later in early_init_mmu(). >>> >>> This fixes the initialization by swapping early_ioremap_setup and >>> early_init_mmu. >> >> Hum ... Chris tested it on a T2080RDB, that must be a book3e. >> >> So we missed it. I guess your fix is right. > > > Oh cool. I forgot, your patch should include Fixes: tags. Fixes: 265c3491c4bc ("powerpc: Add support for GENERIC_EARLY_IOREMAP") If you still change the FIXADDR_SIZE, then it must also have: Fixes: 9ccba66d4d2a ("powerpc/64: Fix the definition of the fixmap area") > >>> >>> This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, >>> seems to make sense, unless there is some weird logic with redefining >>> FIXADDR_SIZE as the compiling goes. >> >> Well, I don't think the order of defines matters, the change should be kept out of the fix. > > When I see this: > > #define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) > #define FIXADDR_SIZE SZ_32M > > > ... I have to think harder what FIXADDR_SIZE was in the first macro and in what order the > preprocessor expands them. > > >> But if you want it anyway, then I'd suggest to move it before IOREMAP_BASE in order to keep the 3 >> IOREMAP_xxx together. > > Up to Michael, I guess. > > >> >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> >>> --- >>> arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- >>> arch/powerpc/kernel/setup_64.c | 3 ++- >>> 2 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h >>> b/arch/powerpc/include/asm/book3s/64/pgtable.h >>> index a666d561b44d..54a06129794b 100644 >>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >>> @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; >>> #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) >>> #define IOREMAP_BASE (PHB_IO_END) >>> #define IOREMAP_START (ioremap_bot) >>> -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >>> #define FIXADDR_SIZE SZ_32M >>> +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) >>> /* Advertise special mapping type for AGP */ >>> #define HAVE_PAGE_AGP >>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c >>> index b779d25761cf..ce09fe5debf4 100644 >>> --- a/arch/powerpc/kernel/setup_64.c >>> +++ b/arch/powerpc/kernel/setup_64.c >>> @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) >>> apply_feature_fixups(); >>> setup_feature_keys(); >>> - early_ioremap_setup(); >>> /* Initialize the hash table or TLB handling */ >>> early_init_mmu(); >>> + early_ioremap_setup(); >>> + >>> /* >>> * After firmware and early platform setup code has set things up, >>> * we note the SPR values for configurable control/performance >>> >
On Thu, 20 May 2021 13:29:19 +1000, Alexey Kardashevskiy wrote: > The immediate problem is that after > 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") > the kernel silently reboots. The reason is that early_ioremap() returns > broken addresses as it uses slot_virt[] array which initialized with > offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == > KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 > when early_ioremap_setup() is called. __kernel_io_end is initialized > little bit later in early_init_mmu(). > > [...] Applied to powerpc/fixes. [1/1] powerpc: Fix early setup to make early_ioremap work https://git.kernel.org/powerpc/c/e2f5efd0f0e229bd110eab513e7c0331d61a4649 cheers
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h index a666d561b44d..54a06129794b 100644 --- a/arch/powerpc/include/asm/book3s/64/pgtable.h +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h @@ -325,8 +325,8 @@ extern unsigned long pci_io_base; #define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE) #define IOREMAP_BASE (PHB_IO_END) #define IOREMAP_START (ioremap_bot) -#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) #define FIXADDR_SIZE SZ_32M +#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE) /* Advertise special mapping type for AGP */ #define HAVE_PAGE_AGP diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c index b779d25761cf..ce09fe5debf4 100644 --- a/arch/powerpc/kernel/setup_64.c +++ b/arch/powerpc/kernel/setup_64.c @@ -369,11 +369,12 @@ void __init early_setup(unsigned long dt_ptr) apply_feature_fixups(); setup_feature_keys(); - early_ioremap_setup(); /* Initialize the hash table or TLB handling */ early_init_mmu(); + early_ioremap_setup(); + /* * After firmware and early platform setup code has set things up, * we note the SPR values for configurable control/performance
The immediate problem is that after 0bd3f9e953bd ("powerpc/legacy_serial: Use early_ioremap()") the kernel silently reboots. The reason is that early_ioremap() returns broken addresses as it uses slot_virt[] array which initialized with offsets from FIXADDR_TOP == IOREMAP_END+FIXADDR_SIZE == KERN_IO_END- FIXADDR_SIZ + FIXADDR_SIZE == __kernel_io_end which is 0 when early_ioremap_setup() is called. __kernel_io_end is initialized little bit later in early_init_mmu(). This fixes the initialization by swapping early_ioremap_setup and early_init_mmu. This also fixes IOREMAP_END to use FIXADDR_SIZE defined just next to it, seems to make sense, unless there is some weird logic with redefining FIXADDR_SIZE as the compiling goes. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- arch/powerpc/kernel/setup_64.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)