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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.11.16 at 15:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/11/16 14:24, Jan Beulich wrote:
>>>>> 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?
> 
> That would avoid the triple fault, but doesn't make the BUG_ON() useful.

And that's because? (I'm sorry if I'm overlooking the obvious.)

>>> 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.
> 
> get_stack_bottom() takes the stack pointer, or's it up to the 8-page
> boundary, overlays a struct cpu_info, then returns the address of es.
> 
> There is no value of %rsp which will ever cause it to fail.  The only
> think which will cause a failure is the layout of struct cpu_info, but
> the BUILD_BUG_ON() will catch that.

Yes. Otherwise a BUILD_BUG_ON() wouldn't work in the first place.
But please also take into consideration my other reply: Moving the
BUILD_BUG_ON() into get_stack_bottom() is inappropriate, and
having it here without the BUG_ON() risks someone updating code
elsewhere such that the BUILD_BUG_ON() wouldn't trigger, but the
BUG_ON() would. That could be avoided only if we could make the
expression handed to BUILD_BUG_ON() use get_stack_bottom().

Jan


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