Re: [RFC v1 0/8] x86/init: Linux linker tables

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Thu, Dec 17, 2015 at 08:25:19PM -0800, H. Peter Anvin wrote:
> On 12/17/15 15:46, Luis R. Rodriguez wrote:
> > 
> > I explain why I do that there but the gist of it is that on Linux we may also
> > want stronger semantics for specific linker table solutions, and solutions such
> > as those devised on the IOMMU init stuff do memmove() for sorting depending on
> > semantics defined (in the simplest case here so far dependency between init
> > sequences), this makes each set of sequences very subsystem specific. An issue
> > with *one* subsystem could make things really bad for others. I thought about
> > this quite a bit and figured its best left to the subsystem maintainers to
> > decide.
> > 
> 
> A table that needs sorting or other runtime handling is just a
> read-write table for the purpose of the linker table construct.  It
> presents to C as an array of initialized data.

Sure but what I was getting at was that since some run time changes to the
table *might* be desirable, depending on the subsystem, the subsystem table needs
to be able to know the start and end address of its table, and that's a
linker script change. But come to think of it, that was me just being pedantic
and careful, I'll try even a run time sort with a few tables of different
structure size to ensure its both possible to just declare them in C and also
sort them without a linker script change.

> > Perhaps a new sections.h file (you tell me) which documents the different
> > section components:
> > 
> > /* document this *really* well */
> > #define SECTION_RODATA		".rodata"
> > #define SECTION_INIT		".init"
> > #define SECTION_INIT_RODATA	".init_rodata"
> > #define SECTION_READ_MOSTLY	".read_mostly"
> > 
> > Then on tables.h we add the section components support:
> 
> Yes, something like that.  How to macroize it cleanly is another matter;
> we may want to use slightly different conventions that iPXE to match our
> own codebase.

Sure, the style below is from iPXE, the one in the patch set matches the
Linux coding style. Other than that I'd welcome feedback on any issues
or recommendations with style on our proposed version of tables.h
Seems you provided some pointers below, so I'll go try to incorporate
those suggestions.

> > #define __table(component, type, name) (component, type, name) 
> > 
> > #define __table_component(table) __table_extract_component table                          
> > #define __table_extract_component(component, type, name) component
> > 
> > #define __table_type(table) __table_extract_type table                          
> > #define __table_extract_type(component, type, name) type
> > 
> > #define __table_name(table) __table_extract_name table                          
> > #define __table_extract_name(component, type, name) name 
> > 
> > #define __table_str(x) #x 
> > 
> > #define __table_section(table, idx) \                                           
> >         "." __table_component (table) ".tbl." __table_name (table) "." __table_str (idx)                      
> > 
> > #define __table_entry(table, idx)                                       \       
> >         __attribute__ ((__section__(__table_section(table, idx)),       \       
> >                         __aligned__(__table_alignment(table))))
> > 
> > A user could then be something as follows:
> > 
> > #define X86_INIT_FNS __table(SECTION_INIT, struct x86_init_fn, "x86_init_fns") 
> > #define __x86_init_fn(order_level) __table_entry(X86_INIT_FNS, order_level)
> 
> Yes, but in particular the common case of function initialization tables
> should be generic.
> 
> I'm kind of thinking a syntax like this:
> 
> DECLARE_LINKTABLE_RO(struct foo, tablename);
> DEFINE_LINKTABLE_RO(struct foo, tablename);
> LINKTABLE_RO(tablename,level) = /* contents */;
> LINKTABLE_SIZE(tablename)
> 
> ... which would turn into something like this once it goes through all
> the preprocessing phases
> 
> /* DECLARE_LINKTABLE_RO */
> extern const struct foo tablename[], tablename__end[];
> 
> /* DEFINE_LINKTABLE_RO */
> DECLARE_LINKTABLE_RO(struct foo, tablename);
> 
> const struct
> foo__attribute__((used,section(".rodata.tbl.tablename.0"))) tablename[0];
> 
> const struct
> foo__attribute__((used,section(".rodata.tbl.tablename.999")))
> tablename__end[0];
> 
> /* LINKTABLE_RO */
> static const __typeof__(tablename)
> __attribute__((used,section(".rodata.tbl.tablename.50")))
> __tbl_tablename_12345
> 
> /* LINKTABLE_SIZE */
> ((tablename__end) - (tablename))
> 
> ... and so on for all the possible sections where we may want tables.

OK thanks so much! Will try working with that.

> Note: I used 0 and 999 above since they sort before and after all
> possible 2-digit decimal numbers, but that's just cosmetic.

Ah neat, so we could still use the two digits 99 and 00 for order
level if we wanted then.

> > If that's what you mean?
> > 
> > I'm a bit wary about having the linker sort any of the above SECTION_*'s, but
> > if we're happy to do that perhaps a simple first step might be to see if 0-day
> > but would be happy with just the sort without any consequences to any
> > architecture. Thoughts?
> 
> I don't see what is dangerous about it.  The section names are such that
> a lexographical sort will do the right thing, and we can simply use
> SORT(.rodata.tbl.*) in the linker script, for example.

OK I'll leave it up to you, I'll go test sorting the sections broadly first.

> >> The other thing is to take a
> >> clue from the implementation in iPXE, which uses priority levels 00 and
> >> 99 (or we could use non-integers which sort appropriately instead of
> >> using "real" levels) to contain the start and end symbols, which
> >> eliminates any need for linker script modifications to add new tables.
> > 
> > This solution uses that as well. The only need for adding custom sections
> > is when they have a requirement for a custom run time sort, and also to
> > ensure they don't cause regressions on other subsystems if they have a buggy
> > sort. The run time sorting is all subsystem specific and up to their own
> > semantics.
> 
> Again, from a linker table POV this is nothing other than a read-write
> table; there is a runtime function that then operates on that read-write
> table.

OK, I'm convinced, I'll go try these changes now.

  Luis