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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/11/16 15:02, Jan Beulich wrote:
>>>> 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().

What problems are we actually trying to detect here?

Irrespective of what actually gets written into tss.rsp0, hardware will
align the stack on entry.

Xen cares that the value written into tss.rsp0 is exactly as expected,
(i.e. some small number of words under the 8-page boundary) so that
constructs such as current and guest_cpu_user_regs() work properly.

~Andrew

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