Messages in this thread | | | From | Andy Lutomirski <> | Date | Thu, 21 May 2015 23:51:50 -0700 | Subject | Re: [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers |
| |
On Thu, May 21, 2015 at 11:22 PM, Ingo Molnar <mingo@kernel.org> wrote: > > * Andy Lutomirski <luto@kernel.org> wrote: > >> --- a/arch/x86/include/asm/segment.h >> +++ b/arch/x86/include/asm/segment.h >> @@ -231,9 +231,17 @@ >> #define TLS_SIZE (GDT_ENTRY_TLS_ENTRIES* 8) >> >> #ifdef __KERNEL__ >> + >> +/* >> + * early_idt_handlers is an array of entry points. For simplicity, it's >> + * a real array. We allocate nine bytes for each entry: two one-byte >> + * push instructions and a five-byte jump in the worst case. >> + */ >> +#define EARLY_IDT_HANDLER_STRIDE 9 > > So how come that two plus five equals nine? ;-)
A "one-byte push" would be pretty impressive :)
> > It's two two-bype pushes (sometimes a 2-byte NOP), plus a 5-byte jump, > in the worst case. > > I'd also mention that it's an array of 32 small trampolines that set > up parameters and jump to a common entry point.
Will do.
> >> +/* Build the early_idt_handlers array */ >> ENTRY(early_idt_handlers) > > Please rename the function accordingly: early_idt_handlers_array, to > make clear this is never jumped to directly, only through the IDT.
Makes sense.
> >> # 36(%esp) %eflags >> # 32(%esp) %cs >> @@ -531,19 +532,18 @@ ENTRY(early_idt_handlers) >> # 24(%rsp) error code >> i = 0 >> .rept NUM_EXCEPTION_VECTORS >> - .if (EXCEPTION_ERRCODE_MASK >> i) & 1 >> - ASM_NOP2 >> - .else >> + .fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc >> + .ifeq (EXCEPTION_ERRCODE_MASK >> i) & 1 >> pushl $0 # Dummy error code, to make stack frame uniform >> .endif >> pushl $i # 20(%esp) Vector number >> jmp early_idt_handler >> i = i + 1 >> .endr >> + .fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc >> ENDPROC(early_idt_handlers) >> >> - /* This is global to keep gas from relaxing the jumps */ >> -ENTRY(early_idt_handler) >> +early_idt_handler: > > Please rename this as well to make it clearer what's happening, > something like: > > early_idt_handler_common: > > would work for me. > > Ditto for the 64-bit side.
Sounds good. I'll do all this tomorrow and resend.
--Andy
| |