Re: [PATCH v4 3/5] build: Alloc space for sched list in the link file

[Jonathan Creekmore <jonathan.creekmore@gmail.com>]

Andrew Cooper writes:

> On 08/01/16 21:22, Jonathan Creekmore wrote:
>> Creates a section to contain scheduler entry pointers that are gathered
>> together into an array. This will allow, in a follow-on patch, scheduler
>> entries to be automatically gathered together into the array for
>> automatic parsing.
>>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>>
>> ---
>> Changed since v3:
>>   * Add defensive check for schedulers in the linker
>>
>> Changed since v1:
>>   * rename the __start and __end symbols to better match
>>     the rest of the file
>> ---
>>  xen/arch/arm/xen.lds.S | 5 +++++
>>  xen/arch/x86/xen.lds.S | 5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 0488f37..2492def 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -57,6 +57,10 @@ SECTIONS
>>         . = ALIGN(PAGE_SIZE);
>>         *(.data.page_aligned)
>>         *(.data)
>> +       . = ALIGN(8);
>> +       __start_schedulers_array = .;
>> +       *(.data.schedulers)
>> +       __end_schedulers_array = .;
>>         *(.data.rel)
>>         *(.data.rel.*)
>>         CONSTRUCTORS
>> @@ -193,3 +197,4 @@ SECTIONS
>>   * code running on the boot time identity map cannot cross a section boundary.
>>   */
>>  ASSERT( _end_boot - start <= PAGE_SIZE, "Boot code is larger than 4K")
>> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index e18e08f..63f89af 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -80,6 +80,10 @@ SECTIONS
>>         __stop___pre_ex_table = .;
>>
>>         *(.data.read_mostly)
>> +       . = ALIGN(8);
>> +       __start_schedulers_array = .;
>> +       *(.data.schedulers)
>> +       __end_schedulers_array = .;
>>         *(.data.rel.ro)
>>         *(.data.rel.ro.*)
>>    } :text
>> @@ -226,3 +230,4 @@ ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
>>  #endif
>>
>>  ASSERT((cpu0_stack & (STACK_SIZE - 1)) == 0, "cpu0_stack misaligned")
>> +ASSERT((__end_schedulers_array - __start_schedulers_array) > 0, "no schedulers compiled in")
>
> This patch won't build on its own, as the ASSERT() will fire.
> Therefore, it breaks bisectability.
>
> In this patch (or the previous one), you need to move the schedulers
> list into __section(".data.schedulers"), and undo the movement in patch
> 4 when populating .data.schedulers properly.
>
> Alternatively, you could just merge patches 3 and 4.  I don't think that
> would reduce the clarity of what you were doing.

Quite right. I think merging patches 3 and 4 is probably the most
straightforward way. That way, one patch introduces the new section and
actually populates that section.