Re: [Xen-devel] [PATCH] x86/boot: Rationalise stack handling during early boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.01.2020 11:43, Andrew Cooper wrote:
> On 09/01/2020 10:28, Jan Beulich wrote:
>> On 08.01.2020 18:00, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -249,23 +249,24 @@ static void __init noreturn 
>>> efi_arch_post_exit_boot(void)
>>>                     "or     $"__stringify(X86_CR4_PGE)", %[cr4]\n\t"
>>>                     "mov    %[cr4], %%cr4\n\t"
>>>  #endif
>>> -                   "movabs $__start_xen, %[rip]\n\t"
>>>                     "lgdt   boot_gdtr(%%rip)\n\t"
>>> -                   "mov    stack_start(%%rip), %%rsp\n\t"
>>>                     "mov    %[ds], %%ss\n\t"
>>>                     "mov    %[ds], %%ds\n\t"
>>>                     "mov    %[ds], %%es\n\t"
>>>                     "mov    %[ds], %%fs\n\t"
>>>                     "mov    %[ds], %%gs\n\t"
>>> -                   "movl   %[cs], 8(%%rsp)\n\t"
>>> -                   "mov    %[rip], (%%rsp)\n\t"
>>> -                   "lretq  %[stkoff]-16"
>>> +
>>> +                   /* Jump to higher mappings. */
>>> +                   "mov    stack_start(%%rip), %%rsp\n\t"
>>> +                   "movabs $__start_xen, %[rip]\n\t"
>>> +                   "push   %[cs]\n\t"
>> Either you need %q[cs] here (assuming q gets ignored for
>> immediate operands, which I didn't check) or ...
>>
>>> +                   "push   %[rip]\n\t"
>>> +                   "lretq"
>>>                     : [rip] "=&r" (efer/* any dead 64-bit variable */),
>>>                       [cr4] "+&r" (cr4)
>>>                     : [cr3] "r" (idle_pg_table),
>>>                       [cs] "ir" (__HYPERVISOR_CS),
>> ... you need to use just "i" here, for there not being 32-bit
>> PUSH forms.
> 
> Lets just go with i.
> 
> "m" is also an option, and clang will probably find some way of pulling
> it out of the stringtable, but anything other than an immediate encoding
> here is going to be silly.

No, sadly "m" is not an option when the expression is a constant:
Gcc at least is unable to materialize a memory variable in this
case, and will give some funny error message instead.

>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -554,7 +554,7 @@ static int do_boot_cpu(int apicid, int cpu)
>>>          printk("Booting processor %d/%d eip %lx\n",
>>>                 cpu, apicid, start_eip);
>>>  
>>> -    stack_start = stack_base[cpu];
>>> +    stack_start = stack_base[cpu] + STACK_SIZE - sizeof(struct cpu_info);
>>>  
>>>      /* This grunge runs the startup process for the targeted processor. */
>> Further down smp_prepare_cpus() has
>>
>>     stack_base[0] = stack_start;
>>
>> which I think you need to also adjust (or replace, e.g. by giving
>> stack_base[] an initializer).
> 
> In practice, this variable is never used because we never offline the BSP.

traps.c uses stack_base[], for example, and hence it needs to be
correct also for the BSP. Even more important is perhaps
get_cpu_current()'s use.

> However, the code as-is is correct.  The value in stack_start has
> changed in this patch, but is still the correct value for the BSP.

But it's no longer the stack base (which is what stack_base[] holds
for all other CPUs), or else you wouldn't have had a need to change
do_boot_cpu().

> It could also be made into an initialiser, but that would cause
> stack_base[] to move from BSS into data, and it is a NR_CPUS sized
> datastructure.

I do realize this.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel