Re: [PATCH RFC 5/9] x86/traps: Functional prep work

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/05/14 13:15, Jan Beulich wrote:
>>>> On 15.05.14 at 12:45, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/14 11:36, Jan Beulich wrote:
>>>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>          .stop_bits = 1
>>>>      };
>>>>  
>>>> +    set_processor_id(0);
>>>> +    set_current((struct vcpu *)0xfffff000); /* debug sanity */
>>>> +    this_cpu(curr_vcpu) = idle_vcpu[0] = current;
>>> The this_cpu() part wasn't there in the original code - is that really
>>> needed, and ...
>> I was attempting to go for similarity between __start_xen and
>> start_secondary, which reminds me I need a further fix regarding cr4,
>> which still loads CR4.MCE on APs before having a TRAP_machine_check
>> handler available.
>>
>>>> +
>>>> +    sort_exception_tables();
>>>> +
>>>>      percpu_init_areas();
>>> ... is that really safe/meaningful before this function got called?
>> There is no specific relationship between sort_exception_tables() and
>> percpu_init_areas(), both of which are tweaking well defined state
>> inside the .data section.
>>
>> sort_excetpion_tables() is a prerequisite for getting extable fixups to
>> work in the trap handlers, but as indicated, it would be nice to turn it
>> into something more like "assert exception tables are sorted" and making
>> the linker do the work.
> The comment wasn't about sort_exception_tables(), but about the
> (at least apparent) conflict of this_cpu() getting used before
> percpu_init_areas().
>
> Jan
>

Ah - I see what you mean.

The BSP per_cpu_offset is 0, so the code as patched does work correctly.

It would however become a latent bug if the implementation of per_cpu
variables changed such that the BSP didn't use the copy of the per_cpu
data in the .data section.

I shall just drop the this_cpu() bit.  Consistency with start_secondary
is not worth this latent bug.

~Andrew