| Message ID | 20180308080020.22828-13-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | first batch of EFI changes for v4.17 | expand |
On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote: > From: Colin Ian King <colin.king@canonical.com> > > Don't populate the const read-only array 'buf' on the stack but instead > make it static. Makes the object code smaller by 64 bytes: > > Before: > text data bss dec hex filename > 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o > > After: > text data bss dec hex filename > 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o > > (gcc version 7.2.0 x86_64) > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/x86/boot/compressed/eboot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index 886a9115af62..f2251c1c9853 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) > > static void setup_quirks(struct boot_params *boot_params) > { > - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; Perhaps static const efi_char16_t apple[] ... is better. > efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long) > efi_table_attr(efi_system_table, fw_vendor, sys_table); > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote: > On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> Don't populate the const read-only array 'buf' on the stack but instead >> make it static. Makes the object code smaller by 64 bytes: >> >> Before: >> text data bss dec hex filename >> 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o >> >> After: >> text data bss dec hex filename >> 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o >> >> (gcc version 7.2.0 x86_64) >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/x86/boot/compressed/eboot.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c >> index 886a9115af62..f2251c1c9853 100644 >> --- a/arch/x86/boot/compressed/eboot.c >> +++ b/arch/x86/boot/compressed/eboot.c >> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) >> >> static void setup_quirks(struct boot_params *boot_params) >> { >> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; >> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > > Perhaps > > static const efi_char16_t apple[] ... > > is better. > Why would that be any better? I have always found the 'const' placement after the type to be much clearer. const void * void const * void * const I.e., #2 and #3 are equivalent, and so 'const' associates to the left not to the right, unless it is at the beginning. Personally, I don't mind either way, but saying it is 'better' is a stretch imo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 March 2018 at 07:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote: >> On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> Don't populate the const read-only array 'buf' on the stack but instead >>> make it static. Makes the object code smaller by 64 bytes: >>> >>> Before: >>> text data bss dec hex filename >>> 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o >>> >>> After: >>> text data bss dec hex filename >>> 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o >>> >>> (gcc version 7.2.0 x86_64) >>> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> arch/x86/boot/compressed/eboot.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c >>> index 886a9115af62..f2251c1c9853 100644 >>> --- a/arch/x86/boot/compressed/eboot.c >>> +++ b/arch/x86/boot/compressed/eboot.c >>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) >>> >>> static void setup_quirks(struct boot_params *boot_params) >>> { >>> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; >>> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; >> >> Perhaps >> >> static const efi_char16_t apple[] ... >> >> is better. >> > > Why would that be any better? I have always found the 'const' > placement after the type to be much clearer. > > const void * > void const * > void * const > > I.e., #2 and #3 are equivalent, That would be #1 and #2, of course > and so 'const' associates to the left > not to the right, unless it is at the beginning. > > Personally, I don't mind either way, but saying it is 'better' is a stretch imo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > From: Colin Ian King <colin.king@canonical.com> > > Don't populate the const read-only array 'buf' on the stack but instead > make it static. Makes the object code smaller by 64 bytes: > > Before: > text data bss dec hex filename > 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o > > After: > text data bss dec hex filename > 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o > > (gcc version 7.2.0 x86_64) > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/x86/boot/compressed/eboot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > index 886a9115af62..f2251c1c9853 100644 > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) > > static void setup_quirks(struct boot_params *boot_params) > { > - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long) > efi_table_attr(efi_system_table, fw_vendor, sys_table); As a general policy, please don't put 'static' variables into the local scope, use file scope instead - right before setup_quirks() would be fine. This makes it abundantly clear that it's not on the stack. Also, would it make sense to rename it to something more descriptive like "apple_unicode_str[]" or so? Plus an unicode string literal initializer would be pretty descriptive as well, instead of the weird looking character array, i.e. something like: static efi_char16_t const apple_unicode_str[] = u"Apple"; ... or so? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 March 2018 at 07:47, Ingo Molnar <mingo@kernel.org> wrote: > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> From: Colin Ian King <colin.king@canonical.com> >> >> Don't populate the const read-only array 'buf' on the stack but instead >> make it static. Makes the object code smaller by 64 bytes: >> >> Before: >> text data bss dec hex filename >> 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o >> >> After: >> text data bss dec hex filename >> 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o >> >> (gcc version 7.2.0 x86_64) >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/x86/boot/compressed/eboot.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c >> index 886a9115af62..f2251c1c9853 100644 >> --- a/arch/x86/boot/compressed/eboot.c >> +++ b/arch/x86/boot/compressed/eboot.c >> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) >> >> static void setup_quirks(struct boot_params *boot_params) >> { >> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; >> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; >> efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long) >> efi_table_attr(efi_system_table, fw_vendor, sys_table); > > As a general policy, please don't put 'static' variables into the local scope, > use file scope instead - right before setup_quirks() would be fine. > > This makes it abundantly clear that it's not on the stack. > Fair enough. I didn't know there was such a policy, but since these have local scope by definition, it doesn't pollute the global namespace so it's fine > Also, would it make sense to rename it to something more descriptive like > "apple_unicode_str[]" or so? > > Plus an unicode string literal initializer would be pretty descriptive as well, > instead of the weird looking character array, i.e. something like: > > static efi_char16_t const apple_unicode_str[] = u"Apple"; > > ... or so? > is u"xxx" the same as L"xxx"? In any case, this is for historical reasons: at some point (and I don't remember the exact details) we had a conflict at link time with objects using 4 byte wchar_t, so we started using this notation to be independent of the size of wchar_t. That issue no longer exists so we should be able to get rid of this. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Also, would it make sense to rename it to something more descriptive like > > "apple_unicode_str[]" or so? > > > > Plus an unicode string literal initializer would be pretty descriptive as well, > > instead of the weird looking character array, i.e. something like: > > > > static efi_char16_t const apple_unicode_str[] = u"Apple"; > > > > ... or so? > > > > is u"xxx" the same as L"xxx"? So "L" literals map to wchar_t, which wide character type is implementation specific IIRC, could be 16-bit or 32-bit wide. u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide characters - which I assume is the EFI type as well? > In any case, this is for historical reasons: at some point (and I > don't remember the exact details) we had a conflict at link time with > objects using 4 byte wchar_t, so we started using this notation to be > independent of the size of wchar_t. That issue no longer exists so we > should be able to get rid of this. Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and having a different type in the kernel build and the host build side - but u"xyz" should solve that. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 March 2018 at 08:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 9 March 2018 at 08:04, Ingo Molnar <mingo@kernel.org> wrote: >> >> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>> > Also, would it make sense to rename it to something more descriptive like >>> > "apple_unicode_str[]" or so? >>> > >>> > Plus an unicode string literal initializer would be pretty descriptive as well, >>> > instead of the weird looking character array, i.e. something like: >>> > >>> > static efi_char16_t const apple_unicode_str[] = u"Apple"; >>> > >>> > ... or so? >>> > >>> >>> is u"xxx" the same as L"xxx"? >> >> So "L" literals map to wchar_t, which wide character type is implementation >> specific IIRC, could be 16-bit or 32-bit wide. >> >> u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide >> characters - which I assume is the EFI type as well? >> >>> In any case, this is for historical reasons: at some point (and I >>> don't remember the exact details) we had a conflict at link time with >>> objects using 4 byte wchar_t, so we started using this notation to be >>> independent of the size of wchar_t. That issue no longer exists so we >>> should be able to get rid of this. >> >> Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and >> having a different type in the kernel build and the host build side - but u"xyz" >> should solve that. >> > > Excellent! > > Do you mind taking this patch as is? I will follow up with a patch > that updates all occurrences of this pattern (we have a few of them), > i.e., use u"" notation and move them to file scope. OK, I misremembered: the other occurrences have already been moved to file scope a while ago. I will follow up with a patch that switches to u"" notation, please let me know if I should respin this or not -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote: > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > Don't populate the const read-only array 'buf' on the stack but instead > > make it static. Makes the object code smaller by 64 bytes: > > > > Before: > > text data bss dec hex filename > > 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o > > > > After: > > text data bss dec hex filename > > 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o > > > > (gcc version 7.2.0 x86_64) > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/x86/boot/compressed/eboot.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > > index 886a9115af62..f2251c1c9853 100644 > > --- a/arch/x86/boot/compressed/eboot.c > > +++ b/arch/x86/boot/compressed/eboot.c > > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) > > > > static void setup_quirks(struct boot_params *boot_params) > > { > > - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > > + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > > efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long) > > efi_table_attr(efi_system_table, fw_vendor, sys_table); > > As a general policy, please don't put 'static' variables into the local > scope, use file scope instead - right before setup_quirks() would be fine. Well, I believe the end result is the same and the closer the declaration is to where it's used, the easier the code is to read and understand. I object to patches like this because they paper over a missing compiler optimization: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725 I have told Colin before that it would be more useful to look into fixing the underlying compiler issue rather than polluting the kernel with "static" keywords, but he keeps sending these patches so I've given up responding: https://lkml.org/lkml/2017/8/25/636 > Plus an unicode string literal initializer would be pretty descriptive > as well, instead of the weird looking character array, i.e. something > like: > > static efi_char16_t const apple_unicode_str[] = u"Apple"; > > ... or so? Last time I checked this didn't work, I believe it's because it's C11 and the kernel is compiled with -std=gnu89. And I'm also not sure if the oldest gcc version that we support understands u"". Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 9 March 2018 at 08:04, Ingo Molnar <mingo@kernel.org> wrote: > > > > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > >> > Also, would it make sense to rename it to something more descriptive like > >> > "apple_unicode_str[]" or so? > >> > > >> > Plus an unicode string literal initializer would be pretty descriptive as well, > >> > instead of the weird looking character array, i.e. something like: > >> > > >> > static efi_char16_t const apple_unicode_str[] = u"Apple"; > >> > > >> > ... or so? > >> > > >> > >> is u"xxx" the same as L"xxx"? > > > > So "L" literals map to wchar_t, which wide character type is implementation > > specific IIRC, could be 16-bit or 32-bit wide. > > > > u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide > > characters - which I assume is the EFI type as well? > > > >> In any case, this is for historical reasons: at some point (and I > >> don't remember the exact details) we had a conflict at link time with > >> objects using 4 byte wchar_t, so we started using this notation to be > >> independent of the size of wchar_t. That issue no longer exists so we > >> should be able to get rid of this. > > > > Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and > > having a different type in the kernel build and the host build side - but u"xyz" > > should solve that. > > > > Excellent! Please double check the generated code though, all of this is from memory. > Do you mind taking this patch as is? I will follow up with a patch > that updates all occurrences of this pattern (we have a few of them), > i.e., use u"" notation and move them to file scope. Sure, done! Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2018-03-09 at 07:44 +0000, Ard Biesheuvel wrote: > On 9 March 2018 at 07:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote: > > > On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote: > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > > > Don't populate the const read-only array 'buf' on the stack but instead > > > > make it static. Makes the object code smaller by 64 bytes: > > > > > > > > Before: > > > > text data bss dec hex filename > > > > 9264 1 16 9281 2441 arch/x86/boot/compressed/eboot.o > > > > > > > > After: > > > > text data bss dec hex filename > > > > 9200 1 16 9217 2401 arch/x86/boot/compressed/eboot.o > > > > > > > > (gcc version 7.2.0 x86_64) > > > > > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > --- > > > > arch/x86/boot/compressed/eboot.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c > > > > index 886a9115af62..f2251c1c9853 100644 > > > > --- a/arch/x86/boot/compressed/eboot.c > > > > +++ b/arch/x86/boot/compressed/eboot.c > > > > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) > > > > > > > > static void setup_quirks(struct boot_params *boot_params) > > > > { > > > > - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > > > > + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; > > > > > > Perhaps > > > > > > static const efi_char16_t apple[] ... > > > > > > is better. > > > > > > > Why would that be any better? It'd be more like the the style used in the rest of the kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 886a9115af62..f2251c1c9853 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params) static void setup_quirks(struct boot_params *boot_params) { - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 }; efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long) efi_table_attr(efi_system_table, fw_vendor, sys_table);