[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] x86: re-add stack alignment check



>>> On 14.11.16 at 14:45, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/11/16 13:25, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -643,6 +643,11 @@ void load_system_tables(void)
>>              .limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>      };
>>  
>> +    /* Bottom-of-stack must be 16-byte aligned! */
>> +    BUILD_BUG_ON((sizeof(struct cpu_info) -
>> +                  offsetof(struct cpu_info, guest_cpu_user_regs.es)) & 0xf);
>> +    BUG_ON(system_state != SYS_STATE_early_boot && (stack_bottom & 0xf));
> 
> This will still triple fault the system if it triggers on an AP. 
> Exceptions aren't hooked up yet.

Hmm, true. They could be moved to the very end of the function
though?

> The reason I dropped the check was that there was no way it was going to
> fail on the BSP (because of the alignment of cpu0_stack) or APs (because
> of the alloc_xenheap_pages(STACK_ORDER, memflags) allocation).

These being page aligned has nothing to do with the BUG_ON()
triggering. I found its dropping being questionable in the context of
the old discussion rooted at this patch of mine
https://lists.xenproject.org/archives/html/xen-devel/2010-07/msg00642.html
I'd like to particularly point you to the various dependencies which
weren't properly spelled out or enforced back then. Specifically
it (not) triggering depends on the number and type of fields in
struct cpu_info following the guest_cpu_user_regs field.

> The BUILD_BUG_ON() is useful to retain, but I would suggest making
> get_stack_bottom() a static inline and putting the BUILD_BUG_ON() there.

I would agree if we were to not add back the BUG_ON(), but as
per above I'm not convinced yet.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.