[PATCH 0/3] arm efi minor fixes

[PATCH 0/3] arm efi minor fixes

[Ard Biesheuvel]

This fixes two minor issues with the handling of .init data in the stub (#1, #2)

Patch #3 replaces some instances of early_memremap() with early_memremap_ro()
where the region in question is only accessed in r/o mode.

Ard Biesheuvel (3):
  arm64: efistub: drop __init annotation from handle_kernel_image()
  arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections
  efi: arm-init: use read-only early mappings

 arch/arm64/kernel/vmlinux.lds.S           |  1 +
 drivers/firmware/efi/arm-init.c           | 14 +++++++-------
 drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
 3 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.5.0

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

After moving arm64-stub.c to libstub/, all of its sections are emitted
as .init.xxx sections automatically, and the __init annotation of
handle_kernel_image() causes it to end up in .init.init.text, which is
not recognized as an __init section by the linker scripts. So drop the
annotation.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 78dfbd34b6bf..9e0342745e4f 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -13,13 +13,13 @@
 #include <asm/efi.h>
 #include <asm/sections.h>
 
-efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
-					unsigned long *image_addr,
-					unsigned long *image_size,
-					unsigned long *reserve_addr,
-					unsigned long *reserve_size,
-					unsigned long dram_base,
-					efi_loaded_image_t *image)
+efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
+				 unsigned long *image_addr,
+				 unsigned long *image_size,
+				 unsigned long *reserve_addr,
+				 unsigned long *reserve_size,
+				 unsigned long dram_base,
+				 efi_loaded_image_t *image)
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;
-- 
2.5.0

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Will Deacon]

On Thu, Jan 28, 2016 at 12:07:32PM +0100, Ard Biesheuvel wrote:
> After moving arm64-stub.c to libstub/, all of its sections are emitted
> as .init.xxx sections automatically, and the __init annotation of
> handle_kernel_image() causes it to end up in .init.init.text, which is
> not recognized as an __init section by the linker scripts. So drop the
> annotation.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 78dfbd34b6bf..9e0342745e4f 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -13,13 +13,13 @@
>  #include <asm/efi.h>
>  #include <asm/sections.h>
>  
> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
> -					unsigned long *image_addr,
> -					unsigned long *image_size,
> -					unsigned long *reserve_addr,
> -					unsigned long *reserve_size,
> -					unsigned long dram_base,
> -					efi_loaded_image_t *image)
> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
> +				 unsigned long *image_addr,
> +				 unsigned long *image_size,
> +				 unsigned long *reserve_addr,
> +				 unsigned long *reserve_size,
> +				 unsigned long dram_base,
> +				 efi_loaded_image_t *image)
>  {
>  	efi_status_t status;
>  	unsigned long kernel_size, kernel_memsize = 0;
> -- 
> 2.5.0
> 

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Thu, 28 Jan, at 12:07:32PM, Ard Biesheuvel wrote:
> After moving arm64-stub.c to libstub/, all of its sections are emitted
> as .init.xxx sections automatically, and the __init annotation of
> handle_kernel_image() causes it to end up in .init.init.text, which is
> not recognized as an __init section by the linker scripts. So drop the
> annotation.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 78dfbd34b6bf..9e0342745e4f 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -13,13 +13,13 @@
>  #include <asm/efi.h>
>  #include <asm/sections.h>
>  
> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
> -					unsigned long *image_addr,
> -					unsigned long *image_size,
> -					unsigned long *reserve_addr,
> -					unsigned long *reserve_size,
> -					unsigned long dram_base,
> -					efi_loaded_image_t *image)
> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
> +				 unsigned long *image_addr,
> +				 unsigned long *image_size,
> +				 unsigned long *reserve_addr,
> +				 unsigned long *reserve_size,
> +				 unsigned long dram_base,
> +				 efi_loaded_image_t *image)
>  {
>  	efi_status_t status;
>  	unsigned long kernel_size, kernel_memsize = 0;
> -- 
> 2.5.0
> 

Would it make more sense to #undef __init in one of the arm64 efistub
header files? I'm thinking of the case where some poor unsuspecting
developer writes a new function and marks it as __init, and we miss it
during review.

At least if we do the #undef we can document why __init makes no sense
in the EFI stub, and at the same time automatically fix things up.

An alternative would be to write some Kbuild magic that complains if
it sees __init, but that seems like more work than is reasonable.

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Thu, 28 Jan, at 12:07:32PM, Ard Biesheuvel wrote:
> After moving arm64-stub.c to libstub/, all of its sections are emitted
> as .init.xxx sections automatically, and the __init annotation of
> handle_kernel_image() causes it to end up in .init.init.text, which is
> not recognized as an __init section by the linker scripts. So drop the
> annotation.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index 78dfbd34b6bf..9e0342745e4f 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -13,13 +13,13 @@
>  #include <asm/efi.h>
>  #include <asm/sections.h>
>  
> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
> -					unsigned long *image_addr,
> -					unsigned long *image_size,
> -					unsigned long *reserve_addr,
> -					unsigned long *reserve_size,
> -					unsigned long dram_base,
> -					efi_loaded_image_t *image)
> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
> +				 unsigned long *image_addr,
> +				 unsigned long *image_size,
> +				 unsigned long *reserve_addr,
> +				 unsigned long *reserve_size,
> +				 unsigned long dram_base,
> +				 efi_loaded_image_t *image)
>  {
>  	efi_status_t status;
>  	unsigned long kernel_size, kernel_memsize = 0;
> -- 
> 2.5.0
> 

Would it make more sense to #undef __init in one of the arm64 efistub
header files? I'm thinking of the case where some poor unsuspecting
developer writes a new function and marks it as __init, and we miss it
during review.

At least if we do the #undef we can document why __init makes no sense
in the EFI stub, and at the same time automatically fix things up.

An alternative would be to write some Kbuild magic that complains if
it sees __init, but that seems like more work than is reasonable.

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 28 January 2016 at 23:58, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Thu, 28 Jan, at 12:07:32PM, Ard Biesheuvel wrote:
>> After moving arm64-stub.c to libstub/, all of its sections are emitted
>> as .init.xxx sections automatically, and the __init annotation of
>> handle_kernel_image() causes it to end up in .init.init.text, which is
>> not recognized as an __init section by the linker scripts. So drop the
>> annotation.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
>> index 78dfbd34b6bf..9e0342745e4f 100644
>> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> @@ -13,13 +13,13 @@
>>  #include <asm/efi.h>
>>  #include <asm/sections.h>
>>
>> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
>> -                                     unsigned long *image_addr,
>> -                                     unsigned long *image_size,
>> -                                     unsigned long *reserve_addr,
>> -                                     unsigned long *reserve_size,
>> -                                     unsigned long dram_base,
>> -                                     efi_loaded_image_t *image)
>> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
>> +                              unsigned long *image_addr,
>> +                              unsigned long *image_size,
>> +                              unsigned long *reserve_addr,
>> +                              unsigned long *reserve_size,
>> +                              unsigned long dram_base,
>> +                              efi_loaded_image_t *image)
>>  {
>>       efi_status_t status;
>>       unsigned long kernel_size, kernel_memsize = 0;
>> --
>> 2.5.0
>>
>
> Would it make more sense to #undef __init in one of the arm64 efistub
> header files? I'm thinking of the case where some poor unsuspecting
> developer writes a new function and marks it as __init, and we miss it
> during review.
>

Yes, I can add it to efistub.h, and make sure it gets included in all the files

Should we #undef it and #define it to a string that is easily grep'ed
for, so it is easy to find the explanatory comment that goes along
with it?
E.g.,

#define __init __init_not_supported_in_efi_stub

> At least if we do the #undef we can document why __init makes no sense
> in the EFI stub, and at the same time automatically fix things up.
>
> An alternative would be to write some Kbuild magic that complains if
> it sees __init, but that seems like more work than is reasonable.

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 28 Jan, at 12:07:32PM, Ard Biesheuvel wrote:
>> After moving arm64-stub.c to libstub/, all of its sections are emitted
>> as .init.xxx sections automatically, and the __init annotation of
>> handle_kernel_image() causes it to end up in .init.init.text, which is
>> not recognized as an __init section by the linker scripts. So drop the
>> annotation.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
>> index 78dfbd34b6bf..9e0342745e4f 100644
>> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> @@ -13,13 +13,13 @@
>>  #include <asm/efi.h>
>>  #include <asm/sections.h>
>>
>> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
>> -                                     unsigned long *image_addr,
>> -                                     unsigned long *image_size,
>> -                                     unsigned long *reserve_addr,
>> -                                     unsigned long *reserve_size,
>> -                                     unsigned long dram_base,
>> -                                     efi_loaded_image_t *image)
>> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
>> +                              unsigned long *image_addr,
>> +                              unsigned long *image_size,
>> +                              unsigned long *reserve_addr,
>> +                              unsigned long *reserve_size,
>> +                              unsigned long dram_base,
>> +                              efi_loaded_image_t *image)
>>  {
>>       efi_status_t status;
>>       unsigned long kernel_size, kernel_memsize = 0;
>> --
>> 2.5.0
>>
>
> Would it make more sense to #undef __init in one of the arm64 efistub
> header files? I'm thinking of the case where some poor unsuspecting
> developer writes a new function and marks it as __init, and we miss it
> during review.
>

Yes, I can add it to efistub.h, and make sure it gets included in all the files

Should we #undef it and #define it to a string that is easily grep'ed
for, so it is easy to find the explanatory comment that goes along
with it?
E.g.,

#define __init __init_not_supported_in_efi_stub

> At least if we do the #undef we can document why __init makes no sense
> in the EFI stub, and at the same time automatically fix things up.
>
> An alternative would be to write some Kbuild magic that complains if
> it sees __init, but that seems like more work than is reasonable.

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
> On 28 January 2016 at 23:58, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> >
> > Would it make more sense to #undef __init in one of the arm64 efistub
> > header files? I'm thinking of the case where some poor unsuspecting
> > developer writes a new function and marks it as __init, and we miss it
> > during review.
> >
> 
> Yes, I can add it to efistub.h, and make sure it gets included in all the files
> 
> Should we #undef it and #define it to a string that is easily grep'ed
> for, so it is easy to find the explanatory comment that goes along
> with it?
> E.g.,
> 
> #define __init __init_not_supported_in_efi_stub

This would produce a compilation failure if someone tags something as
__init right? Looks fine to me.

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> >
> > Would it make more sense to #undef __init in one of the arm64 efistub
> > header files? I'm thinking of the case where some poor unsuspecting
> > developer writes a new function and marks it as __init, and we miss it
> > during review.
> >
> 
> Yes, I can add it to efistub.h, and make sure it gets included in all the files
> 
> Should we #undef it and #define it to a string that is easily grep'ed
> for, so it is easy to find the explanatory comment that goes along
> with it?
> E.g.,
> 
> #define __init __init_not_supported_in_efi_stub

This would produce a compilation failure if someone tags something as
__init right? Looks fine to me.

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 29 January 2016 at 17:00, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> On 28 January 2016 at 23:58, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> >
>> > Would it make more sense to #undef __init in one of the arm64 efistub
>> > header files? I'm thinking of the case where some poor unsuspecting
>> > developer writes a new function and marks it as __init, and we miss it
>> > during review.
>> >
>>
>> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>>
>> Should we #undef it and #define it to a string that is easily grep'ed
>> for, so it is easy to find the explanatory comment that goes along
>> with it?
>> E.g.,
>>
>> #define __init __init_not_supported_in_efi_stub
>
> This would produce a compilation failure if someone tags something as
> __init right? Looks fine to me.

Yes, but it makes the error message difficult to decipher, since
__init is printed and not what it resolves to. Locally, I tried this,
which seems to work:

#undef __init
#define __init __attribute__((__init_not_supported_in_efi_stub))
#pragma GCC diagnostic error "-Wattributes"

which produces

  CC      drivers/firmware/efi/libstub/arm-stub.o
/home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
error: ‘__init_not_supported_in_efi_stub’ attribute directive ignored
[-Werror=attributes]
 {
 ^
cc1: some warnings being treated as errors

which is slightly dodgy, but at least puts the string right in the error message

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 29 January 2016 at 17:00, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >
>> > Would it make more sense to #undef __init in one of the arm64 efistub
>> > header files? I'm thinking of the case where some poor unsuspecting
>> > developer writes a new function and marks it as __init, and we miss it
>> > during review.
>> >
>>
>> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>>
>> Should we #undef it and #define it to a string that is easily grep'ed
>> for, so it is easy to find the explanatory comment that goes along
>> with it?
>> E.g.,
>>
>> #define __init __init_not_supported_in_efi_stub
>
> This would produce a compilation failure if someone tags something as
> __init right? Looks fine to me.

Yes, but it makes the error message difficult to decipher, since
__init is printed and not what it resolves to. Locally, I tried this,
which seems to work:

#undef __init
#define __init __attribute__((__init_not_supported_in_efi_stub))
#pragma GCC diagnostic error "-Wattributes"

which produces

  CC      drivers/firmware/efi/libstub/arm-stub.o
/home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
error: ?__init_not_supported_in_efi_stub? attribute directive ignored
[-Werror=attributes]
 {
 ^
cc1: some warnings being treated as errors

which is slightly dodgy, but at least puts the string right in the error message

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Fri, 29 Jan, at 05:03:16PM, Ard Biesheuvel wrote:
> On 29 January 2016 at 17:00, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
> >> On 28 January 2016 at 23:58, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW75QkVnex5nT@public.gmane.orgk> wrote:
> >> >
> >> > Would it make more sense to #undef __init in one of the arm64 efistub
> >> > header files? I'm thinking of the case where some poor unsuspecting
> >> > developer writes a new function and marks it as __init, and we miss it
> >> > during review.
> >> >
> >>
> >> Yes, I can add it to efistub.h, and make sure it gets included in all the files
> >>
> >> Should we #undef it and #define it to a string that is easily grep'ed
> >> for, so it is easy to find the explanatory comment that goes along
> >> with it?
> >> E.g.,
> >>
> >> #define __init __init_not_supported_in_efi_stub
> >
> > This would produce a compilation failure if someone tags something as
> > __init right? Looks fine to me.
> 
> Yes, but it makes the error message difficult to decipher, since
> __init is printed and not what it resolves to. Locally, I tried this,
> which seems to work:
> 
> #undef __init
> #define __init __attribute__((__init_not_supported_in_efi_stub))
> #pragma GCC diagnostic error "-Wattributes"
> 
> which produces
> 
>   CC      drivers/firmware/efi/libstub/arm-stub.o
> /home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
> error: ‘__init_not_supported_in_efi_stub’ attribute directive ignored
> [-Werror=attributes]
>  {
>  ^
> cc1: some warnings being treated as errors
> 
> which is slightly dodgy, but at least puts the string right in the error message

What about,

#define __init __compiletime_error("__init not supported in EFI boot stub")

?

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Fri, 29 Jan, at 05:03:16PM, Ard Biesheuvel wrote:
> On 29 January 2016 at 17:00, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
> >> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> >> >
> >> > Would it make more sense to #undef __init in one of the arm64 efistub
> >> > header files? I'm thinking of the case where some poor unsuspecting
> >> > developer writes a new function and marks it as __init, and we miss it
> >> > during review.
> >> >
> >>
> >> Yes, I can add it to efistub.h, and make sure it gets included in all the files
> >>
> >> Should we #undef it and #define it to a string that is easily grep'ed
> >> for, so it is easy to find the explanatory comment that goes along
> >> with it?
> >> E.g.,
> >>
> >> #define __init __init_not_supported_in_efi_stub
> >
> > This would produce a compilation failure if someone tags something as
> > __init right? Looks fine to me.
> 
> Yes, but it makes the error message difficult to decipher, since
> __init is printed and not what it resolves to. Locally, I tried this,
> which seems to work:
> 
> #undef __init
> #define __init __attribute__((__init_not_supported_in_efi_stub))
> #pragma GCC diagnostic error "-Wattributes"
> 
> which produces
> 
>   CC      drivers/firmware/efi/libstub/arm-stub.o
> /home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
> error: ?__init_not_supported_in_efi_stub? attribute directive ignored
> [-Werror=attributes]
>  {
>  ^
> cc1: some warnings being treated as errors
> 
> which is slightly dodgy, but at least puts the string right in the error message

What about,

#define __init __compiletime_error("__init not supported in EFI boot stub")

?

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 2 February 2016 at 12:08, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Fri, 29 Jan, at 05:03:16PM, Ard Biesheuvel wrote:
>> On 29 January 2016 at 17:00, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> > On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> >> On 28 January 2016 at 23:58, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW+G/Ez6ZCGd0@public.gmane.orguk> wrote:
>> >> >
>> >> > Would it make more sense to #undef __init in one of the arm64 efistub
>> >> > header files? I'm thinking of the case where some poor unsuspecting
>> >> > developer writes a new function and marks it as __init, and we miss it
>> >> > during review.
>> >> >
>> >>
>> >> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>> >>
>> >> Should we #undef it and #define it to a string that is easily grep'ed
>> >> for, so it is easy to find the explanatory comment that goes along
>> >> with it?
>> >> E.g.,
>> >>
>> >> #define __init __init_not_supported_in_efi_stub
>> >
>> > This would produce a compilation failure if someone tags something as
>> > __init right? Looks fine to me.
>>
>> Yes, but it makes the error message difficult to decipher, since
>> __init is printed and not what it resolves to. Locally, I tried this,
>> which seems to work:
>>
>> #undef __init
>> #define __init __attribute__((__init_not_supported_in_efi_stub))
>> #pragma GCC diagnostic error "-Wattributes"
>>
>> which produces
>>
>>   CC      drivers/firmware/efi/libstub/arm-stub.o
>> /home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
>> error: ‘__init_not_supported_in_efi_stub’ attribute directive ignored
>> [-Werror=attributes]
>>  {
>>  ^
>> cc1: some warnings being treated as errors
>>
>> which is slightly dodgy, but at least puts the string right in the error message
>
> What about,
>
> #define __init __compiletime_error("__init not supported in EFI boot stub")
>

That only works for invocations, i.e., it needs to be used in header
files, and will trigger the error if a call to the function remains
after optimization. We want it at function definition time instead.

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 2 February 2016 at 12:08, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 29 Jan, at 05:03:16PM, Ard Biesheuvel wrote:
>> On 29 January 2016 at 17:00, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> > On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> >> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >> >
>> >> > Would it make more sense to #undef __init in one of the arm64 efistub
>> >> > header files? I'm thinking of the case where some poor unsuspecting
>> >> > developer writes a new function and marks it as __init, and we miss it
>> >> > during review.
>> >> >
>> >>
>> >> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>> >>
>> >> Should we #undef it and #define it to a string that is easily grep'ed
>> >> for, so it is easy to find the explanatory comment that goes along
>> >> with it?
>> >> E.g.,
>> >>
>> >> #define __init __init_not_supported_in_efi_stub
>> >
>> > This would produce a compilation failure if someone tags something as
>> > __init right? Looks fine to me.
>>
>> Yes, but it makes the error message difficult to decipher, since
>> __init is printed and not what it resolves to. Locally, I tried this,
>> which seems to work:
>>
>> #undef __init
>> #define __init __attribute__((__init_not_supported_in_efi_stub))
>> #pragma GCC diagnostic error "-Wattributes"
>>
>> which produces
>>
>>   CC      drivers/firmware/efi/libstub/arm-stub.o
>> /home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
>> error: ?__init_not_supported_in_efi_stub? attribute directive ignored
>> [-Werror=attributes]
>>  {
>>  ^
>> cc1: some warnings being treated as errors
>>
>> which is slightly dodgy, but at least puts the string right in the error message
>
> What about,
>
> #define __init __compiletime_error("__init not supported in EFI boot stub")
>

That only works for invocations, i.e., it needs to be used in header
files, and will trigger the error if a call to the function remains
after optimization. We want it at function definition time instead.

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Tue, 02 Feb, at 12:09:41PM, Ard Biesheuvel wrote:
> On 2 February 2016 at 12:08, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> >
> > What about,
> >
> > #define __init __compiletime_error("__init not supported in EFI boot stub")
> >
> 
> That only works for invocations, i.e., it needs to be used in header
> files, and will trigger the error if a call to the function remains
> after optimization. We want it at function definition time instead.

Good point.

OK, how about we just do the #undef, call it good, and I add the task
of printing some helpful error message to my growing TODO list?

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Matt Fleming]

On Tue, 02 Feb, at 12:09:41PM, Ard Biesheuvel wrote:
> On 2 February 2016 at 12:08, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> >
> > What about,
> >
> > #define __init __compiletime_error("__init not supported in EFI boot stub")
> >
> 
> That only works for invocations, i.e., it needs to be used in header
> files, and will trigger the error if a call to the function remains
> after optimization. We want it at function definition time instead.

Good point.

OK, how about we just do the #undef, call it good, and I add the task
of printing some helpful error message to my growing TODO list?

Re: [PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 3 February 2016 at 16:19, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 02 Feb, at 12:09:41PM, Ard Biesheuvel wrote:
>> On 2 February 2016 at 12:08, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> >
>> > What about,
>> >
>> > #define __init __compiletime_error("__init not supported in EFI boot stub")
>> >
>>
>> That only works for invocations, i.e., it needs to be used in header
>> files, and will trigger the error if a call to the function remains
>> after optimization. We want it at function definition time instead.
>
> Good point.
>
> OK, how about we just do the #undef, call it good, and I add the task
> of printing some helpful error message to my growing TODO list?

I take it you didn't like my #pragma then? :-)

In any case, I don't care deeply about this, so just #undef'ing it is fine by me

[PATCH 1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

[Ard Biesheuvel]

On 3 February 2016 at 16:19, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 02 Feb, at 12:09:41PM, Ard Biesheuvel wrote:
>> On 2 February 2016 at 12:08, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >
>> > What about,
>> >
>> > #define __init __compiletime_error("__init not supported in EFI boot stub")
>> >
>>
>> That only works for invocations, i.e., it needs to be used in header
>> files, and will trigger the error if a call to the function remains
>> after optimization. We want it at function definition time instead.
>
> Good point.
>
> OK, how about we just do the #undef, call it good, and I add the task
> of printing some helpful error message to my growing TODO list?

I take it you didn't like my #pragma then? :-)

In any case, I don't care deeply about this, so just #undef'ing it is fine by me

[PATCH 2/3] arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections

[Ard Biesheuvel]

The EFI stub is typically built into the decompressor (x86, ARM) so none
of its symbols are annotated as __init. However, on arm64, the stub is
linked into the kernel proper, and the code is __init annotated at the
section level by prepending all names of SHF_ALLOC sections with '.init'.

This results in section names like .init.rodata.str1.8 (for string literals)
and .init.bss (which is tiny), both of which can be moved into the .init.data
output section.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/vmlinux.lds.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index e3928f578891..cbf4db440e9c 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -134,6 +134,7 @@ SECTIONS
 		CON_INITCALL
 		SECURITY_INITCALL
 		INIT_RAM_FS
+		*(.init.rodata.* .init.bss)	/* from the EFI stub */
 	}
 	.exit.data : {
 		ARM_EXIT_KEEP(EXIT_DATA)
-- 
2.5.0

[PATCH 2/3] arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections

[Will Deacon]

On Thu, Jan 28, 2016 at 12:07:33PM +0100, Ard Biesheuvel wrote:
> The EFI stub is typically built into the decompressor (x86, ARM) so none
> of its symbols are annotated as __init. However, on arm64, the stub is
> linked into the kernel proper, and the code is __init annotated at the
> section level by prepending all names of SHF_ALLOC sections with '.init'.
> 
> This results in section names like .init.rodata.str1.8 (for string literals)
> and .init.bss (which is tiny), both of which can be moved into the .init.data
> output section.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/vmlinux.lds.S | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index e3928f578891..cbf4db440e9c 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -134,6 +134,7 @@ SECTIONS
>  		CON_INITCALL
>  		SECURITY_INITCALL
>  		INIT_RAM_FS
> +		*(.init.rodata.* .init.bss)	/* from the EFI stub */
>  	}
>  	.exit.data : {
>  		ARM_EXIT_KEEP(EXIT_DATA)
> -- 
> 2.5.0
> 

Re: [PATCH 2/3] arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections

[Matt Fleming]

On Thu, 28 Jan, at 12:07:33PM, Ard Biesheuvel wrote:
> The EFI stub is typically built into the decompressor (x86, ARM) so none
> of its symbols are annotated as __init. However, on arm64, the stub is
> linked into the kernel proper, and the code is __init annotated at the
> section level by prepending all names of SHF_ALLOC sections with '.init'.
> 
> This results in section names like .init.rodata.str1.8 (for string literals)
> and .init.bss (which is tiny), both of which can be moved into the .init.data
> output section.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/vmlinux.lds.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index e3928f578891..cbf4db440e9c 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -134,6 +134,7 @@ SECTIONS
>  		CON_INITCALL
>  		SECURITY_INITCALL
>  		INIT_RAM_FS
> +		*(.init.rodata.* .init.bss)	/* from the EFI stub */
>  	}
>  	.exit.data : {
>  		ARM_EXIT_KEEP(EXIT_DATA)

FWIW,

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>

[PATCH 2/3] arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections

[Matt Fleming]

On Thu, 28 Jan, at 12:07:33PM, Ard Biesheuvel wrote:
> The EFI stub is typically built into the decompressor (x86, ARM) so none
> of its symbols are annotated as __init. However, on arm64, the stub is
> linked into the kernel proper, and the code is __init annotated at the
> section level by prepending all names of SHF_ALLOC sections with '.init'.
> 
> This results in section names like .init.rodata.str1.8 (for string literals)
> and .init.bss (which is tiny), both of which can be moved into the .init.data
> output section.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/vmlinux.lds.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index e3928f578891..cbf4db440e9c 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -134,6 +134,7 @@ SECTIONS
>  		CON_INITCALL
>  		SECURITY_INITCALL
>  		INIT_RAM_FS
> +		*(.init.rodata.* .init.bss)	/* from the EFI stub */
>  	}
>  	.exit.data : {
>  		ARM_EXIT_KEEP(EXIT_DATA)

FWIW,

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

[PATCH 3/3] efi: arm-init: use read-only early mappings

[Ard Biesheuvel]

The early mappings of the EFI system table contents and the UEFI memory
map are read-only from the OS point of view. So map them read-only to
protect them from inadvertent modification.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/arm-init.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 9e15d571b53c..aa1f743152a2 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -61,8 +61,8 @@ static int __init uefi_init(void)
 	char vendor[100] = "unknown";
 	int i, retval;
 
-	efi.systab = early_memremap(efi_system_table,
-				    sizeof(efi_system_table_t));
+	efi.systab = early_memremap_ro(efi_system_table,
+				       sizeof(efi_system_table_t));
 	if (efi.systab == NULL) {
 		pr_warn("Unable to map EFI system table.\n");
 		return -ENOMEM;
@@ -86,8 +86,8 @@ static int __init uefi_init(void)
 			efi.systab->hdr.revision & 0xffff);
 
 	/* Show what we know for posterity */
-	c16 = early_memremap(efi_to_phys(efi.systab->fw_vendor),
-			     sizeof(vendor) * sizeof(efi_char16_t));
+	c16 = early_memremap_ro(efi_to_phys(efi.systab->fw_vendor),
+				sizeof(vendor) * sizeof(efi_char16_t));
 	if (c16) {
 		for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)
 			vendor[i] = c16[i];
@@ -100,8 +100,8 @@ static int __init uefi_init(void)
 		efi.systab->hdr.revision & 0xffff, vendor);
 
 	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
-	config_tables = early_memremap(efi_to_phys(efi.systab->tables),
-				       table_size);
+	config_tables = early_memremap_ro(efi_to_phys(efi.systab->tables),
+					  table_size);
 	if (config_tables == NULL) {
 		pr_warn("Unable to map EFI config table array.\n");
 		retval = -ENOMEM;
@@ -185,7 +185,7 @@ void __init efi_init(void)
 	efi_system_table = params.system_table;
 
 	memmap.phys_map = params.mmap;
-	memmap.map = early_memremap(params.mmap, params.mmap_size);
+	memmap.map = early_memremap_ro(params.mmap, params.mmap_size);
 	if (memmap.map == NULL) {
 		/*
 		* If we are booting via UEFI, the UEFI memory map is the only
-- 
2.5.0

[PATCH 0/3] arm efi minor fixes

[Mark Rutland]

On Thu, Jan 28, 2016 at 12:07:31PM +0100, Ard Biesheuvel wrote:
> This fixes two minor issues with the handling of .init data in the stub (#1, #2)
> 
> Patch #3 replaces some instances of early_memremap() with early_memremap_ro()
> where the region in question is only accessed in r/o mode.

Nice!

The series looks sensible to me, and having given it a spin on Juno
(atop of v4.5-rc1 defconfig) everything seems to work, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> Ard Biesheuvel (3):
>   arm64: efistub: drop __init annotation from handle_kernel_image()
>   arm64: vmlinux.lds.S: handle .init.rodata.xxx and .init.bss sections
>   efi: arm-init: use read-only early mappings
> 
>  arch/arm64/kernel/vmlinux.lds.S           |  1 +
>  drivers/firmware/efi/arm-init.c           | 14 +++++++-------
>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> -- 
> 2.5.0
>