| Message ID | 5829C95A020000780011E6EA@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/11/16 13:25, Jan Beulich wrote: > Commit 279840d5ea ("x86/boot: install trap handlers much earlier on > boot"), perhaps not really intentionally, removed this check. Add it No - that was deliberate. The check isn't useful (see below). > back, > - preventing it to trigger before any output is set up, "preventing it from triggering ..." > - accompanying it with a (weaker, due to its open coding of what > get_stack_bottom() does) build time check. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While not strictly needed for 4.8, I thought I'd still submit it as a > possible candidate. > > --- 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. 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). 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. ~Andrew > + > /* Main stack for interrupts/exceptions. */ > tss->rsp0 = stack_bottom; > tss->bitmap = IOBMP_INVALID_OFFSET; > > >
>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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
>>> On 14.11.16 at 15:24, <JBeulich@suse.com> wrote: >>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> wrote: >> 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. Actually no - the 16-byte alignment requirement exists only here, not for every caller of this function. Jan
On 14/11/16 14:24, Jan Beulich wrote: >>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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. > >> 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. ~Andrew
>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote: > On 14/11/16 14:24, Jan Beulich wrote: >>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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
On 14/11/16 15:02, Jan Beulich wrote: >>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote: >> On 14/11/16 14:24, Jan Beulich wrote: >>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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
>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote: > On 14/11/16 15:02, Jan Beulich wrote: >>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote: >>> On 14/11/16 14:24, Jan Beulich wrote: >>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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. And that's the problem: Everything will break in that case (being off by 8 bytes). I recall from the time when I did put together the old patch I did point you to. If you want to try out, just add a single 8-byte field to struct cpu_info and try booting the resulting hypervisor. IOW the checks being added are to verify the comment in the struct cpu_info declaration. Jan
On 14/11/16 16:54, Jan Beulich wrote: >>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote: >> On 14/11/16 15:02, Jan Beulich wrote: >>>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote: >>>> On 14/11/16 14:24, Jan Beulich wrote: >>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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. > And that's the problem: Everything will break in that case (being off > by 8 bytes). Not quite. Everything will indeed break if it is off by 8, but everything will also be similarly-broken if it is off by 16 or off by -8. Checking for 16-byte alignment doesn't catch half the broken cases. ~Andrew
>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote: > On 14/11/16 16:54, Jan Beulich wrote: >>>>> On 14.11.16 at 17:38, <andrew.cooper3@citrix.com> wrote: >>> On 14/11/16 15:02, Jan Beulich wrote: >>>>>>> On 14.11.16 at 15:38, <andrew.cooper3@citrix.com> wrote: >>>>> On 14/11/16 14:24, Jan Beulich wrote: >>>>>>>>> On 14.11.16 at 14:45, <andrew.cooper3@citrix.com> 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. >> And that's the problem: Everything will break in that case (being off >> by 8 bytes). > > Not quite. > > Everything will indeed break if it is off by 8, but everything will also > be similarly-broken if it is off by 16 or off by -8. Off by -8 will be caught as well. And there's no possible off-by-16, as then the calculations will be right again (read: you can freely add two longs to struct cpu_info, but you mustn't add just one). Jan
>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote: >>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote: >> Everything will indeed break if it is off by 8, but everything will also >> be similarly-broken if it is off by 16 or off by -8. > > Off by -8 will be caught as well. And there's no possible off-by-16, > as then the calculations will be right again (read: you can freely > add two longs to struct cpu_info, but you mustn't add just one). This discussion appears to have stalled, and so far I've seen no reason to change other than the position of the checks. I'm hesitant to send out v2 though without having reached agreement on the other aspects. Jan
On 22/11/16 10:14, Jan Beulich wrote: >>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote: >>>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote: >>> Everything will indeed break if it is off by 8, but everything will also >>> be similarly-broken if it is off by 16 or off by -8. >> Off by -8 will be caught as well. And there's no possible off-by-16, >> as then the calculations will be right again (read: you can freely >> add two longs to struct cpu_info, but you mustn't add just one). > This discussion appears to have stalled, and so far I've seen no > reason to change other than the position of the checks. I'm > hesitant to send out v2 though without having reached agreement > on the other aspects. Fine - lets get this fix back in for 4.8, then argue over how it should be improved. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On Fri, Nov 25, 2016 at 10:34:30AM +0000, Andrew Cooper wrote: > On 22/11/16 10:14, Jan Beulich wrote: > >>>> On 15.11.16 at 11:46, <JBeulich@suse.com> wrote: > >>>>> On 15.11.16 at 11:26, <andrew.cooper3@citrix.com> wrote: > >>> Everything will indeed break if it is off by 8, but everything will also > >>> be similarly-broken if it is off by 16 or off by -8. > >> Off by -8 will be caught as well. And there's no possible off-by-16, > >> as then the calculations will be right again (read: you can freely > >> add two longs to struct cpu_info, but you mustn't add just one). > > This discussion appears to have stalled, and so far I've seen no > > reason to change other than the position of the checks. I'm > > hesitant to send out v2 though without having reached agreement > > on the other aspects. > > Fine - lets get this fix back in for 4.8, then argue over how it should > be improved. > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
--- 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)); + /* Main stack for interrupts/exceptions. */ tss->rsp0 = stack_bottom; tss->bitmap = IOBMP_INVALID_OFFSET;
Commit 279840d5ea ("x86/boot: install trap handlers much earlier on boot"), perhaps not really intentionally, removed this check. Add it back, - preventing it to trigger before any output is set up, - accompanying it with a (weaker, due to its open coding of what get_stack_bottom() does) build time check. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While not strictly needed for 4.8, I thought I'd still submit it as a possible candidate. x86: re-add stack alignment check Commit 279840d5ea ("x86/boot: install trap handlers much earlier on boot"), perhaps not really intentionally, removed this check. Add it back, - preventing it to trigger before any output is set up, - accompanying it with a (weaker, due to its open coding of what get_stack_bottom() does) build time check. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While not strictly needed for 4.8, I thought I'd still submit it as a possible candidate. --- 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)); + /* Main stack for interrupts/exceptions. */ tss->rsp0 = stack_bottom; tss->bitmap = IOBMP_INVALID_OFFSET;