| Message ID | 1451960299-14123-1-git-send-email-dennis.chen@arm.com |
|---|---|
| State | New |
| Headers | show |
On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > efi_mm.pgd while not the static efi_pgd[], since this function will be > called with early_initcall, which results in the pgd_cache used by > pgd_alloc() has not been initialized yet, kernel will hang in this > case. This patch is trying to make the pgd_cache_init() called before > arm_enable_runtime_services() by changing its core_initcall to > early_initcall. > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/arm64/mm/pgd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > index cb3ba1b..859a788 100644 > --- a/arch/arm64/mm/pgd.c > +++ b/arch/arm64/mm/pgd.c > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > SLAB_PANIC, NULL); > return 0; > } > -core_initcall(pgd_cache_init); > +early_initcall(pgd_cache_init); Hello all, Apologies for not spotting this before sending it out. The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, since the series this is part of primarily concerns ARM not arm64. Anyway, shuffling init ordering is my least favorite way of fixing bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote: > On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > > efi_mm.pgd while not the static efi_pgd[], since this function will be > > called with early_initcall, which results in the pgd_cache used by > > pgd_alloc() has not been initialized yet, kernel will hang in this > > case. This patch is trying to make the pgd_cache_init() called before > > arm_enable_runtime_services() by changing its core_initcall to > > early_initcall. > > > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > > > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > --- > > arch/arm64/mm/pgd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > > index cb3ba1b..859a788 100644 > > --- a/arch/arm64/mm/pgd.c > > +++ b/arch/arm64/mm/pgd.c > > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > > SLAB_PANIC, NULL); > > return 0; > > } > > -core_initcall(pgd_cache_init); > > +early_initcall(pgd_cache_init); > > > Hello all, > > Apologies for not spotting this before sending it out. > > The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did > not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, > since the series this is part of primarily concerns ARM not arm64. > > Anyway, shuffling init ordering is my least favorite way of fixing > bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), > we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and > define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. > > Ard, Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create another efi_pgd_alloc() used by efi only, which looks more like a temp workaround. Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make sense to get an entire page as the space for the efi.pgd. Changing the init order sequence is a reasonable way to fix it unless you can see some potential issues there. > -- > Ard. > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 5 January 2016 at 09:35, Dennis Chen <dennis.chen@arm.com> wrote: > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote: >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for >> > efi_mm.pgd while not the static efi_pgd[], since this function will be >> > called with early_initcall, which results in the pgd_cache used by >> > pgd_alloc() has not been initialized yet, kernel will hang in this >> > case. This patch is trying to make the pgd_cache_init() called before >> > arm_enable_runtime_services() by changing its core_initcall to >> > early_initcall. >> > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> >> > >> > Cc: Will Deacon <will.deacon@arm.com> >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > Cc: Sudeep Holla <sudeep.holla@arm.com> >> > --- >> > arch/arm64/mm/pgd.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c >> > index cb3ba1b..859a788 100644 >> > --- a/arch/arm64/mm/pgd.c >> > +++ b/arch/arm64/mm/pgd.c >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) >> > SLAB_PANIC, NULL); >> > return 0; >> > } >> > -core_initcall(pgd_cache_init); >> > +early_initcall(pgd_cache_init); >> >> >> Hello all, >> >> Apologies for not spotting this before sending it out. >> >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, >> since the series this is part of primarily concerns ARM not arm64. >> >> Anyway, shuffling init ordering is my least favorite way of fixing >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. >> >> > Ard, > > Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create > another efi_pgd_alloc() used by efi only, which looks more like a temp workaround. > Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make > sense to get an entire page as the space for the efi.pgd. Changing the init order > sequence is a reasonable way to fix it unless you can see some potential issues there. > Well, since arm_enable_runtime_services() is an early_initcall() itself, how are you guaranteeing the ordering between the two? Link order? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > On 5 January 2016 at 09:35, Dennis Chen <dennis.chen@arm.com> wrote: > > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote: > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > >> > called with early_initcall, which results in the pgd_cache used by > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > >> > case. This patch is trying to make the pgd_cache_init() called before > >> > arm_enable_runtime_services() by changing its core_initcall to > >> > early_initcall. > >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > >> > > >> > Cc: Will Deacon <will.deacon@arm.com> > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > >> > --- > >> > arch/arm64/mm/pgd.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > >> > index cb3ba1b..859a788 100644 > >> > --- a/arch/arm64/mm/pgd.c > >> > +++ b/arch/arm64/mm/pgd.c > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > >> > SLAB_PANIC, NULL); > >> > return 0; > >> > } > >> > -core_initcall(pgd_cache_init); > >> > +early_initcall(pgd_cache_init); > >> > >> > >> Hello all, > >> > >> Apologies for not spotting this before sending it out. > >> > >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did > >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, > >> since the series this is part of primarily concerns ARM not arm64. > >> > >> Anyway, shuffling init ordering is my least favorite way of fixing > >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), > >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and > >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. > >> > >> > > Ard, > > > > Both arm and arm64 provide pgd_alloc(), I don't think it's a good idea to create > > another efi_pgd_alloc() used by efi only, which looks more like a temp workaround. > > Plus, for 64KB page granule when VA = 48bit, the PGD_SIZE = 0x200, it doesn't make > > sense to get an entire page as the space for the efi.pgd. Changing the init order > > sequence is a reasonable way to fix it unless you can see some potential issues there. > > > > Well, since arm_enable_runtime_services() is an early_initcall() > itself, how are you guaranteeing the ordering between the two? Link > order? > Link order. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > > >> > called with early_initcall, which results in the pgd_cache used by > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > > >> > case. This patch is trying to make the pgd_cache_init() called before > > >> > arm_enable_runtime_services() by changing its core_initcall to > > >> > early_initcall. > > >> > > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > > >> > > > >> > Cc: Will Deacon <will.deacon@arm.com> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > > >> > --- > > >> > arch/arm64/mm/pgd.c | 2 +- > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > > >> > index cb3ba1b..859a788 100644 > > >> > --- a/arch/arm64/mm/pgd.c > > >> > +++ b/arch/arm64/mm/pgd.c > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > > >> > SLAB_PANIC, NULL); > > >> > return 0; > > >> > } > > >> > -core_initcall(pgd_cache_init); > > >> > +early_initcall(pgd_cache_init); [...] > > Well, since arm_enable_runtime_services() is an early_initcall() > > itself, how are you guaranteeing the ordering between the two? Link > > order? > > Link order. And can you explain how this works, what guarantees it gives? > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose > the contents to any other person, use it for any purpose, or store or > copy the information in any medium. Thank you. BTW, please sort out the legal disclaimer (raise an IT ticket). -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote: > On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > > efi_mm.pgd while not the static efi_pgd[], since this function will be > > called with early_initcall, which results in the pgd_cache used by > > pgd_alloc() has not been initialized yet, kernel will hang in this > > case. This patch is trying to make the pgd_cache_init() called before > > arm_enable_runtime_services() by changing its core_initcall to > > early_initcall. > > > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > > > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > --- > > arch/arm64/mm/pgd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > > index cb3ba1b..859a788 100644 > > --- a/arch/arm64/mm/pgd.c > > +++ b/arch/arm64/mm/pgd.c > > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > > SLAB_PANIC, NULL); > > return 0; > > } > > -core_initcall(pgd_cache_init); > > +early_initcall(pgd_cache_init); > > > Hello all, > > Apologies for not spotting this before sending it out. > > The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did > not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, > since the series this is part of primarily concerns ARM not arm64. > > Anyway, shuffling init ordering is my least favorite way of fixing > bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), > we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and > define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. Or we could put the pgd_cache initialisation in pgtable_cache_init, which sounds like the right place for it anyway. Curious, but why are our EFI runtime services initialised off the back of initcalls, whereas on x86 the initialisation is driven directly from init/main.c? I'm not saying it should be changed, it just looks odd having the two paths. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 5 January 2016 at 13:31, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Jan 05, 2016 at 08:36:39AM +0100, Ard Biesheuvel wrote: >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for >> > efi_mm.pgd while not the static efi_pgd[], since this function will be >> > called with early_initcall, which results in the pgd_cache used by >> > pgd_alloc() has not been initialized yet, kernel will hang in this >> > case. This patch is trying to make the pgd_cache_init() called before >> > arm_enable_runtime_services() by changing its core_initcall to >> > early_initcall. >> > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> >> > >> > Cc: Will Deacon <will.deacon@arm.com> >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > Cc: Sudeep Holla <sudeep.holla@arm.com> >> > --- >> > arch/arm64/mm/pgd.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c >> > index cb3ba1b..859a788 100644 >> > --- a/arch/arm64/mm/pgd.c >> > +++ b/arch/arm64/mm/pgd.c >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) >> > SLAB_PANIC, NULL); >> > return 0; >> > } >> > -core_initcall(pgd_cache_init); >> > +early_initcall(pgd_cache_init); >> >> >> Hello all, >> >> Apologies for not spotting this before sending it out. >> >> The issue only occurs when PGD_SIZE != PAGE_SIZE, which is why I did >> not see it myself. I only tested with 4k/39-bits and 64k/42-bits IIRC, >> since the series this is part of primarily concerns ARM not arm64. >> >> Anyway, shuffling init ordering is my least favorite way of fixing >> bugs. Since only ARM requires pgd_alloc() (for the kernel mappings), >> we could also simply factor out pgd_alloc() into efi_pgd_alloc(), and >> define it to mean '__get_free_page(PGALLOC_GFP)' on arm64. > > Or we could put the pgd_cache initialisation in pgtable_cache_init, which > sounds like the right place for it anyway. > Indeed. > Curious, but why are our EFI runtime services initialised off the back of > initcalls, whereas on x86 the initialisation is driven directly from > init/main.c? I'm not saying it should be changed, it just looks odd having > the two paths. > Simply because we tried not to pollute all core files with explicit EFI calls. The EFI init bits are called in line by setup_arch(), because they are needed so early, so there we had no choice. The runtime services bits are only needed much later, so there we used an initcall because we could. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: > On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: > > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > > > >> > called with early_initcall, which results in the pgd_cache used by > > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > > > >> > case. This patch is trying to make the pgd_cache_init() called before > > > >> > arm_enable_runtime_services() by changing its core_initcall to > > > >> > early_initcall. > > > >> > > > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > > > >> > > > > >> > Cc: Will Deacon <will.deacon@arm.com> > > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > > > >> > --- > > > >> > arch/arm64/mm/pgd.c | 2 +- > > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > > > >> > index cb3ba1b..859a788 100644 > > > >> > --- a/arch/arm64/mm/pgd.c > > > >> > +++ b/arch/arm64/mm/pgd.c > > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > > > >> > SLAB_PANIC, NULL); > > > >> > return 0; > > > >> > } > > > >> > -core_initcall(pgd_cache_init); > > > >> > +early_initcall(pgd_cache_init); > [...] > > > Well, since arm_enable_runtime_services() is an early_initcall() > > > itself, how are you guaranteeing the ordering between the two? Link > > > order? > > > > Link order. > > And can you explain how this works, what guarantees it gives? > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, for the init call section, early_initcall is the first chuck in the section, followed by LEVEL[0-7]. For the same level, the layout order is determined by the link order, ARCH is always precedence over the drivers. Catalin, are you giving me a kernel examination? :) > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > > recipient, please notify the sender immediately and do not disclose > > the contents to any other person, use it for any purpose, or store or > > copy the information in any medium. Thank you. > > BTW, please sort out the legal disclaimer (raise an IT ticket). > Hmm, the ticket has been raised. > -- > Catalin > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be >> > > >> > called with early_initcall, which results in the pgd_cache used by >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this >> > > >> > case. This patch is trying to make the pgd_cache_init() called before >> > > >> > arm_enable_runtime_services() by changing its core_initcall to >> > > >> > early_initcall. >> > > >> > >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> >> > > >> > >> > > >> > Cc: Will Deacon <will.deacon@arm.com> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> >> > > >> > --- >> > > >> > arch/arm64/mm/pgd.c | 2 +- >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > > >> > >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c >> > > >> > index cb3ba1b..859a788 100644 >> > > >> > --- a/arch/arm64/mm/pgd.c >> > > >> > +++ b/arch/arm64/mm/pgd.c >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) >> > > >> > SLAB_PANIC, NULL); >> > > >> > return 0; >> > > >> > } >> > > >> > -core_initcall(pgd_cache_init); >> > > >> > +early_initcall(pgd_cache_init); >> [...] >> > > Well, since arm_enable_runtime_services() is an early_initcall() >> > > itself, how are you guaranteeing the ordering between the two? Link >> > > order? >> > >> > Link order. >> >> And can you explain how this works, what guarantees it gives? >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, > for the init call section, early_initcall is the first chuck in the section, > followed by LEVEL[0-7]. For the same level, the layout order is determined > by the link order, ARCH is always precedence over the drivers. Catalin, are > you giving me a kernel examination? :) > We all know how initcalls are implemented. The question is how you are going to ensure that the early_initcall() that initializes the PGD cache is always invoked before the early_initcall() that creates the UEFI runtime page tables. And saying that the currently observed link order happens to be correct is not good enough. We need to be sure that, even if we change the link order, or switch to LTO at some point, things don't suddenly break again. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote: > On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: > > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: > >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: > >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > >> > > >> > called with early_initcall, which results in the pgd_cache used by > >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > >> > > >> > case. This patch is trying to make the pgd_cache_init() called before > >> > > >> > arm_enable_runtime_services() by changing its core_initcall to > >> > > >> > early_initcall. > >> > > >> > > >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > >> > > >> > > >> > > >> > Cc: Will Deacon <will.deacon@arm.com> > >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > >> > > >> > --- > >> > > >> > arch/arm64/mm/pgd.c | 2 +- > >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > > >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > >> > > >> > index cb3ba1b..859a788 100644 > >> > > >> > --- a/arch/arm64/mm/pgd.c > >> > > >> > +++ b/arch/arm64/mm/pgd.c > >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > >> > > >> > SLAB_PANIC, NULL); > >> > > >> > return 0; > >> > > >> > } > >> > > >> > -core_initcall(pgd_cache_init); > >> > > >> > +early_initcall(pgd_cache_init); > >> [...] > >> > > Well, since arm_enable_runtime_services() is an early_initcall() > >> > > itself, how are you guaranteeing the ordering between the two? Link > >> > > order? > >> > > >> > Link order. > >> > >> And can you explain how this works, what guarantees it gives? > >> > > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, > > for the init call section, early_initcall is the first chuck in the section, > > followed by LEVEL[0-7]. For the same level, the layout order is determined > > by the link order, ARCH is always precedence over the drivers. Catalin, are > > you giving me a kernel examination? :) > > > > We all know how initcalls are implemented. The question is how you are > going to ensure that the early_initcall() that initializes the PGD > cache is always invoked before the early_initcall() that creates the > UEFI runtime page tables. > > And saying that the currently observed link order happens to be > correct is not good enough. We need to be sure that, even if we change > the link order, or switch to LTO at some point, things don't suddenly > break again. > Well, if the build system changes the link order, it can't make sure it will break something unexpectedly, needless to say the pgd_cache_init here at all. Do you think the kernel will change its currently link order policy? If the answer is yes, what's the benefit? > -- > Ard. > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote: > On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote: >> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: >> > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: >> >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: >> >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: >> >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: >> >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime >> >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for >> >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be >> >> > > >> > called with early_initcall, which results in the pgd_cache used by >> >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this >> >> > > >> > case. This patch is trying to make the pgd_cache_init() called before >> >> > > >> > arm_enable_runtime_services() by changing its core_initcall to >> >> > > >> > early_initcall. >> >> > > >> > >> >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> >> >> > > >> > >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> >> >> > > >> > --- >> >> > > >> > arch/arm64/mm/pgd.c | 2 +- >> >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > > >> > >> >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c >> >> > > >> > index cb3ba1b..859a788 100644 >> >> > > >> > --- a/arch/arm64/mm/pgd.c >> >> > > >> > +++ b/arch/arm64/mm/pgd.c >> >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) >> >> > > >> > SLAB_PANIC, NULL); >> >> > > >> > return 0; >> >> > > >> > } >> >> > > >> > -core_initcall(pgd_cache_init); >> >> > > >> > +early_initcall(pgd_cache_init); >> >> [...] >> >> > > Well, since arm_enable_runtime_services() is an early_initcall() >> >> > > itself, how are you guaranteeing the ordering between the two? Link >> >> > > order? >> >> > >> >> > Link order. >> >> >> >> And can you explain how this works, what guarantees it gives? >> >> >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, >> > for the init call section, early_initcall is the first chuck in the section, >> > followed by LEVEL[0-7]. For the same level, the layout order is determined >> > by the link order, ARCH is always precedence over the drivers. Catalin, are >> > you giving me a kernel examination? :) >> > >> >> We all know how initcalls are implemented. The question is how you are >> going to ensure that the early_initcall() that initializes the PGD >> cache is always invoked before the early_initcall() that creates the >> UEFI runtime page tables. >> >> And saying that the currently observed link order happens to be >> correct is not good enough. We need to be sure that, even if we change >> the link order, or switch to LTO at some point, things don't suddenly >> break again. >> > Well, if the build system changes the link order, it can't make sure it will break something > unexpectedly, needless to say the pgd_cache_init here at all. That is no excuse to introduce yet another failure mode. > Do you think the kernel > will change its currently link order policy? If the answer is yes, what's the benefit? > It does not matter what I think. You seem to think that the link order is set in stone, so it is you who should argue why that is a reasonable assumption. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote: > On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote: > > On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote: > >> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: > >> > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: > >> >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: > >> >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > >> >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > >> >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > >> >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > >> >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > >> >> > > >> > called with early_initcall, which results in the pgd_cache used by > >> >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > >> >> > > >> > case. This patch is trying to make the pgd_cache_init() called before > >> >> > > >> > arm_enable_runtime_services() by changing its core_initcall to > >> >> > > >> > early_initcall. > >> >> > > >> > > >> >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > >> >> > > >> > > >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com> > >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > >> >> > > >> > --- > >> >> > > >> > arch/arm64/mm/pgd.c | 2 +- > >> >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > > >> > > >> >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > >> >> > > >> > index cb3ba1b..859a788 100644 > >> >> > > >> > --- a/arch/arm64/mm/pgd.c > >> >> > > >> > +++ b/arch/arm64/mm/pgd.c > >> >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > >> >> > > >> > SLAB_PANIC, NULL); > >> >> > > >> > return 0; > >> >> > > >> > } > >> >> > > >> > -core_initcall(pgd_cache_init); > >> >> > > >> > +early_initcall(pgd_cache_init); > >> >> [...] > >> >> > > Well, since arm_enable_runtime_services() is an early_initcall() > >> >> > > itself, how are you guaranteeing the ordering between the two? Link > >> >> > > order? > >> >> > > >> >> > Link order. > >> >> > >> >> And can you explain how this works, what guarantees it gives? > >> >> > >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, > >> > for the init call section, early_initcall is the first chuck in the section, > >> > followed by LEVEL[0-7]. For the same level, the layout order is determined > >> > by the link order, ARCH is always precedence over the drivers. Catalin, are > >> > you giving me a kernel examination? :) > >> > > >> > >> We all know how initcalls are implemented. The question is how you are > >> going to ensure that the early_initcall() that initializes the PGD > >> cache is always invoked before the early_initcall() that creates the > >> UEFI runtime page tables. > >> > >> And saying that the currently observed link order happens to be > >> correct is not good enough. We need to be sure that, even if we change > >> the link order, or switch to LTO at some point, things don't suddenly > >> break again. > >> > > Well, if the build system changes the link order, it can't make sure it will break something > > unexpectedly, needless to say the pgd_cache_init here at all. > > That is no excuse to introduce yet another failure mode. > > > Do you think the kernel > > will change its currently link order policy? If the answer is yes, what's the benefit? > > > > It does not matter what I think. You seem to think that the link order > is set in stone, so it is you who should argue why that is a > reasonable assumption. > OK, If you think the link order is volatile, how can you guarantee the arm_enable_runtime_services with early_initcall always work in a volatile link order environment? > -- > Ard. > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote: > On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote: >> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote: >> > On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote: >> >> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: >> >> > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: >> >> >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: >> >> >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: >> >> >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: >> >> >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime >> >> >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for >> >> >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be >> >> >> > > >> > called with early_initcall, which results in the pgd_cache used by >> >> >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this >> >> >> > > >> > case. This patch is trying to make the pgd_cache_init() called before >> >> >> > > >> > arm_enable_runtime_services() by changing its core_initcall to >> >> >> > > >> > early_initcall. >> >> >> > > >> > >> >> >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> >> >> >> > > >> > >> >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com> >> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> >> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> >> >> >> > > >> > --- >> >> >> > > >> > arch/arm64/mm/pgd.c | 2 +- >> >> >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> > > >> > >> >> >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c >> >> >> > > >> > index cb3ba1b..859a788 100644 >> >> >> > > >> > --- a/arch/arm64/mm/pgd.c >> >> >> > > >> > +++ b/arch/arm64/mm/pgd.c >> >> >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) >> >> >> > > >> > SLAB_PANIC, NULL); >> >> >> > > >> > return 0; >> >> >> > > >> > } >> >> >> > > >> > -core_initcall(pgd_cache_init); >> >> >> > > >> > +early_initcall(pgd_cache_init); >> >> >> [...] >> >> >> > > Well, since arm_enable_runtime_services() is an early_initcall() >> >> >> > > itself, how are you guaranteeing the ordering between the two? Link >> >> >> > > order? >> >> >> > >> >> >> > Link order. >> >> >> >> >> >> And can you explain how this works, what guarantees it gives? >> >> >> >> >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, >> >> > for the init call section, early_initcall is the first chuck in the section, >> >> > followed by LEVEL[0-7]. For the same level, the layout order is determined >> >> > by the link order, ARCH is always precedence over the drivers. Catalin, are >> >> > you giving me a kernel examination? :) >> >> > >> >> >> >> We all know how initcalls are implemented. The question is how you are >> >> going to ensure that the early_initcall() that initializes the PGD >> >> cache is always invoked before the early_initcall() that creates the >> >> UEFI runtime page tables. >> >> >> >> And saying that the currently observed link order happens to be >> >> correct is not good enough. We need to be sure that, even if we change >> >> the link order, or switch to LTO at some point, things don't suddenly >> >> break again. >> >> >> > Well, if the build system changes the link order, it can't make sure it will break something >> > unexpectedly, needless to say the pgd_cache_init here at all. >> >> That is no excuse to introduce yet another failure mode. >> >> > Do you think the kernel >> > will change its currently link order policy? If the answer is yes, what's the benefit? >> > >> >> It does not matter what I think. You seem to think that the link order >> is set in stone, so it is you who should argue why that is a >> reasonable assumption. >> > OK, If you think the link order is volatile, how can you guarantee the arm_enable_runtime_services with early_initcall > always work in a volatile link order environment? > By not relying on other early_initcalls _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 06, 2016 at 09:54:42AM +0100, Ard Biesheuvel wrote: > On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote: > > On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote: > >> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote: > >> > On Wed, Jan 06, 2016 at 08:13:24AM +0100, Ard Biesheuvel wrote: > >> >> On 6 January 2016 at 07:14, Dennis Chen <dennis.chen@arm.com> wrote: > >> >> > On Tue, Jan 05, 2016 at 09:56:03AM +0000, Catalin Marinas wrote: > >> >> >> On Tue, Jan 05, 2016 at 04:40:44PM +0800, Dennis Chen wrote: > >> >> >> > On Tue, Jan 05, 2016 at 09:38:11AM +0100, Ard Biesheuvel wrote: > >> >> >> > > >> On 5 January 2016 at 03:18, Dennis Chen <dennis.chen@arm.com> wrote: > >> >> >> > > >> > The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime > >> >> >> > > >> > code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for > >> >> >> > > >> > efi_mm.pgd while not the static efi_pgd[], since this function will be > >> >> >> > > >> > called with early_initcall, which results in the pgd_cache used by > >> >> >> > > >> > pgd_alloc() has not been initialized yet, kernel will hang in this > >> >> >> > > >> > case. This patch is trying to make the pgd_cache_init() called before > >> >> >> > > >> > arm_enable_runtime_services() by changing its core_initcall to > >> >> >> > > >> > early_initcall. > >> >> >> > > >> > > >> >> >> > > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> >> >> > > >> > Tested-by: Sudeep Holla <sudeep.holla@arm.com> > >> >> >> > > >> > > >> >> >> > > >> > Cc: Will Deacon <will.deacon@arm.com> > >> >> >> > > >> > Cc: Catalin Marinas <catalin.marinas@arm.com> > >> >> >> > > >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> >> >> > > >> > Cc: Sudeep Holla <sudeep.holla@arm.com> > >> >> >> > > >> > --- > >> >> >> > > >> > arch/arm64/mm/pgd.c | 2 +- > >> >> >> > > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> >> > > >> > > >> >> >> > > >> > diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c > >> >> >> > > >> > index cb3ba1b..859a788 100644 > >> >> >> > > >> > --- a/arch/arm64/mm/pgd.c > >> >> >> > > >> > +++ b/arch/arm64/mm/pgd.c > >> >> >> > > >> > @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) > >> >> >> > > >> > SLAB_PANIC, NULL); > >> >> >> > > >> > return 0; > >> >> >> > > >> > } > >> >> >> > > >> > -core_initcall(pgd_cache_init); > >> >> >> > > >> > +early_initcall(pgd_cache_init); > >> >> >> [...] > >> >> >> > > Well, since arm_enable_runtime_services() is an early_initcall() > >> >> >> > > itself, how are you guaranteeing the ordering between the two? Link > >> >> >> > > order? > >> >> >> > > >> >> >> > Link order. > >> >> >> > >> >> >> And can you explain how this works, what guarantees it gives? > >> >> >> > >> >> > You can take a look at include/asm-generic/vmlinux.ldx.h: INIT_CALLS macro, > >> >> > for the init call section, early_initcall is the first chuck in the section, > >> >> > followed by LEVEL[0-7]. For the same level, the layout order is determined > >> >> > by the link order, ARCH is always precedence over the drivers. Catalin, are > >> >> > you giving me a kernel examination? :) > >> >> > > >> >> > >> >> We all know how initcalls are implemented. The question is how you are > >> >> going to ensure that the early_initcall() that initializes the PGD > >> >> cache is always invoked before the early_initcall() that creates the > >> >> UEFI runtime page tables. > >> >> > >> >> And saying that the currently observed link order happens to be > >> >> correct is not good enough. We need to be sure that, even if we change > >> >> the link order, or switch to LTO at some point, things don't suddenly > >> >> break again. > >> >> > >> > Well, if the build system changes the link order, it can't make sure it will break something > >> > unexpectedly, needless to say the pgd_cache_init here at all. > >> > >> That is no excuse to introduce yet another failure mode. > >> > >> > Do you think the kernel > >> > will change its currently link order policy? If the answer is yes, what's the benefit? > >> > > >> > >> It does not matter what I think. You seem to think that the link order > >> is set in stone, so it is you who should argue why that is a > >> reasonable assumption. > >> > > OK, If you think the link order is volatile, how can you guarantee the arm_enable_runtime_services with early_initcall > > always work in a volatile link order environment? > > > > By not relying on other early_initcalls > You're limiting the scope of the link order IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Dennis, On Wed, Jan 06, 2016 at 04:59:30PM +0800, Dennis Chen wrote: > On Wed, Jan 06, 2016 at 09:54:42AM +0100, Ard Biesheuvel wrote: > > On 6 January 2016 at 09:52, Dennis Chen <dennis.chen@arm.com> wrote: > > > On Wed, Jan 06, 2016 at 08:42:20AM +0100, Ard Biesheuvel wrote: > > >> On 6 January 2016 at 08:38, Dennis Chen <dennis.chen@arm.com> wrote: > > >> > Well, if the build system changes the link order, it can't make sure it will break something > > >> > unexpectedly, needless to say the pgd_cache_init here at all. > > >> > > >> That is no excuse to introduce yet another failure mode. > > >> > > >> > Do you think the kernel > > >> > will change its currently link order policy? If the answer is yes, what's the benefit? > > >> > > > >> > > >> It does not matter what I think. You seem to think that the link order > > >> is set in stone, so it is you who should argue why that is a > > >> reasonable assumption. > > >> > > > OK, If you think the link order is volatile, how can you guarantee the arm_enable_runtime_services with early_initcall > > > always work in a volatile link order environment? > > > > > > > By not relying on other early_initcalls > > > You're limiting the scope of the link order Please stop this mindless bickering. It's a waste of everybody's time and we have a kernel to test. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c index cb3ba1b..859a788 100644 --- a/arch/arm64/mm/pgd.c +++ b/arch/arm64/mm/pgd.c @@ -56,4 +56,4 @@ static int __init pgd_cache_init(void) SLAB_PANIC, NULL); return 0; } -core_initcall(pgd_cache_init); +early_initcall(pgd_cache_init);
The commit 3400749b5a22 ("arm64/efi: refactor EFI init and runtime code for reuse by 32-bit ARM") uses pgd_alloc() to allocate space for efi_mm.pgd while not the static efi_pgd[], since this function will be called with early_initcall, which results in the pgd_cache used by pgd_alloc() has not been initialized yet, kernel will hang in this case. This patch is trying to make the pgd_cache_init() called before arm_enable_runtime_services() by changing its core_initcall to early_initcall. Signed-off-by: Dennis Chen <dennis.chen@arm.com> Tested-by: Sudeep Holla <sudeep.holla@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Sudeep Holla <sudeep.holla@arm.com> --- arch/arm64/mm/pgd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.9.1 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel