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.
>
>> --- 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.
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.
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.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
