Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1500705 > unrolled thread

Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Jiri Slaby <jslaby@suse.cz> - 2016-10-14 08:00 +0200
    Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Vegard Nossum <vegard.nossum@oracle.com> - 2016-10-14 08:30 +0200
      Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Jiri Slaby <jslaby@suse.cz> - 2016-10-14 08:40 +0200

#1500705 — Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

On 06/26/2016, 07:17 PM, Linus Torvalds wrote:
> On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>
>> This is the best I could come up with: assuming gcc is not allowed to
>> reason about what's inside the asm(), this is the only way I could
>> think of to lose the array information without incurring unnecessary
>> overheads. It should also be relatively safe as there is no way to
>> accidentally use the underlying arrays without explicitly declaring
>> them.
> 
> Ugh. I worry about the other places where we do things like this,
> depending on the linker just assigning the addresses and us being able
> to compare them.
> 
> If there is a compiler option to disable this optimization, I would
> almost prefer that.. Because we really do have a whole slew of these
> things.

Any update on this? Couple months later and I still hit this.

Quick checking shows, that a lot code depends on comparing two arrays
(undefined behaviour):
ftrace_init
  count = __stop_mcount_loc - __start_mcount_loc;
tracer_alloc_buffers
  if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)


FWIW this indeed fixes the get_builtin_firmware case for me:
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -97,9 +97,11 @@ extern struct builtin_fw __end_builtin_fw[];
 bool get_builtin_firmware(struct cpio_data *cd, const char *name)
 {
 #ifdef CONFIG_FW_LOADER
-       struct builtin_fw *b_fw;
+       struct builtin_fw *b_fw = __start_builtin_fw;

-       for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
+       OPTIMIZER_HIDE_VAR(b_fw);
+
+       for (; b_fw != __end_builtin_fw; b_fw++) {
                if (!strcmp(name, b_fw->name)) {
                        cd->size = b_fw->size;
                        cd->data = b_fw->data;



What about adding:
#define for_each_vmlinux_symbol(sym, start, stop) \
  for (sym = start, OPTIMIZER_HIDE_VAR(sym); sym != stop; sym++)

and converting at least the iterators?

What to do with the array subtractions and comparisons (like tracing), I
don't know (yet).

thanks,
-- 
js
suse labs

[toc] | [next] | [standalone]

#1500710

On 10/14/2016 07:52 AM, Jiri Slaby wrote:
> On 06/26/2016, 07:17 PM, Linus Torvalds wrote:
>> On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>>
>>> This is the best I could come up with: assuming gcc is not allowed to
>>> reason about what's inside the asm(), this is the only way I could
>>> think of to lose the array information without incurring unnecessary
>>> overheads. It should also be relatively safe as there is no way to
>>> accidentally use the underlying arrays without explicitly declaring
>>> them.
>>
>> Ugh. I worry about the other places where we do things like this,
>> depending on the linker just assigning the addresses and us being able
>> to compare them.
>>
>> If there is a compiler option to disable this optimization, I would
>> almost prefer that.. Because we really do have a whole slew of these
>> things.
>
> Any update on this? Couple months later and I still hit this.

I didn't find any compiler flags or anything that would let us get by
with the current code.

I have a branch fixing a bunch of these (many of them in tracing code,
like you wrote) using my old approach (declaring the arrays with a
macro) but it looks so ugly I got discouraged from submitting any of it.
I also don't have a great way to actually test a lot of the fixes.

Could something like this work?

#define DECLARE_EXTERNAL_ARRAY(type, name) \
	extern type __##name##_start;
	static inline type name##_start(void) \
	{ \
		type tmp = __##name##_start; \
		OPTIMIZER_HIDE_VAR(tmp); \
		return tmp; \
	} \
	static inline type name##_end(void) \
	{ \
		type tmp = __##name##_end; \
		OPTIMIZER_HIDE_VAR(tmp); \
		return tmp; \
	} \
	static inline size_t name##_size(void) \
	{ \
		return name##_end() - name##_start(); \
	}

#define ext_for_each(var, name) \
	for (var = name##_start(); var != name##_end(); var++)

and then for the firmware case you would do

DECLARE_EXTERNAL_ARRAY(struct builtin_fw, builtin_fw);

struct builtin_fw *fw;
ext_for_each(fw, builtin_fw)
	...;

You'd also have to adjust the names in the linker script accordingly.

The points here being:

1) DECLARE_*() follows the kernel convention (you get what you expect,
more or less)

2) the real variables defined in the linker script are hidden behind
some generated names so you don't use them by accident

3) no need to sprinkle your code with OPTIMIZER_HIDE_VAR() or anything
else, but you do need to use function calls to access the variables
(e.g. firmware_start() and firmware_end()).

What do you think?


Vegard

[toc] | [prev] | [next] | [standalone]

#1500714

On 10/14/2016, 08:25 AM, Vegard Nossum wrote:
> On 10/14/2016 07:52 AM, Jiri Slaby wrote:
>> On 06/26/2016, 07:17 PM, Linus Torvalds wrote:
>>> On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum
>>> <vegard.nossum@gmail.com> wrote:
>>>>
>>>> This is the best I could come up with: assuming gcc is not allowed to
>>>> reason about what's inside the asm(), this is the only way I could
>>>> think of to lose the array information without incurring unnecessary
>>>> overheads. It should also be relatively safe as there is no way to
>>>> accidentally use the underlying arrays without explicitly declaring
>>>> them.
>>>
>>> Ugh. I worry about the other places where we do things like this,
>>> depending on the linker just assigning the addresses and us being able
>>> to compare them.
>>>
>>> If there is a compiler option to disable this optimization, I would
>>> almost prefer that.. Because we really do have a whole slew of these
>>> things.
>>
>> Any update on this? Couple months later and I still hit this.
> 
> I didn't find any compiler flags or anything that would let us get by
> with the current code.

FWIW, this changed with 73447cc5d17 in gcc. There are no flags added by
the commit, so I assume this sole optimization cannot be turned off.

> I have a branch fixing a bunch of these (many of them in tracing code,
> like you wrote) using my old approach (declaring the arrays with a
> macro) but it looks so ugly I got discouraged from submitting any of it.
> I also don't have a great way to actually test a lot of the fixes.
> 
> Could something like this work?
> 
> #define DECLARE_EXTERNAL_ARRAY(type, name) \
>     extern type __##name##_start;
>     static inline type name##_start(void) \
>     { \
>         type tmp = __##name##_start; \
>         OPTIMIZER_HIDE_VAR(tmp); \
>         return tmp; \
>     } \
>     static inline type name##_end(void) \
>     { \
>         type tmp = __##name##_end; \
>         OPTIMIZER_HIDE_VAR(tmp); \
>         return tmp; \
>     } \
>     static inline size_t name##_size(void) \
>     { \
>         return name##_end() - name##_start(); \
>     }
> 
> #define ext_for_each(var, name) \
>     for (var = name##_start(); var != name##_end(); var++)
> 
> and then for the firmware case you would do
> 
> DECLARE_EXTERNAL_ARRAY(struct builtin_fw, builtin_fw);
> 
> struct builtin_fw *fw;
> ext_for_each(fw, builtin_fw)
>     ...;
> 
> You'd also have to adjust the names in the linker script accordingly.
> 
> The points here being:
> 
> 1) DECLARE_*() follows the kernel convention (you get what you expect,
> more or less)
> 
> 2) the real variables defined in the linker script are hidden behind
> some generated names so you don't use them by accident
> 
> 3) no need to sprinkle your code with OPTIMIZER_HIDE_VAR() or anything
> else, but you do need to use function calls to access the variables
> (e.g. firmware_start() and firmware_end()).
> 
> What do you think?

FWIW LGTM

thanks,
-- 
js
suse labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web