Re: [Xen-devel] [PATCH RFC 6/9] x86/boot: Install trap handlers much earlier on boot

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.14 at 13:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/05/14 11:53, Jan Beulich wrote:
>>>>> On 15.05.14 at 11:48, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +void __cpuinit load_system_tables(void)
>>> +{
>>> +   unsigned int cpu = smp_processor_id();
>>> +   unsigned long stack_bottom = get_stack_bottom(),
>>> +           stack_top = stack_bottom & ~(STACK_SIZE - 1);
>>> +
>>> +   struct tss_struct *tss = &this_cpu(init_tss);
>>> +   struct desc_struct *gdt =
>>> +           this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
>>> +   struct desc_struct *compat_gdt =
>>> +           this_cpu(compat_gdt_table)- FIRST_RESERVED_GDT_ENTRY;
>>> +
>>> +   struct desc_ptr gdtr = {
>>> +           .base = (unsigned long)gdt,
>>> +           .limit = LAST_RESERVED_GDT_BYTE,
>>> +   };
>>> +   struct desc_ptr idtr = {
>>> +           .base = (unsigned long)idt_tables[cpu],
>>> +           .limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>> +   };
>>> +
>>> +   /* Main stack for interrupts/exceptions */
>>> +   tss->rsp0 = stack_bottom;
>>> +   tss->bitmap = IOBMP_INVALID_OFFSET;
>>> +
>>> +   /* MCE, NMI and Double Fault handlers get their own stacks */
>>> +   tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
>>> +   tss->ist[IST_DF  - 1] = stack_top + IST_DF  * PAGE_SIZE;
>> Hard tab.
> 
> These should all be hard tabs, following the file's style.

At the line start yes, but not in the middle of a line please.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -558,15 +558,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>          .stop_bits = 1
>>>      };
>>>  
>>> +    /* Critical region without IDT or TSS.  Any fault is deadly! */
>> Which raises the question whether the next patch shouldn't be
>> dropped.
> 
> With this patch, the BSP code looks like:
> 
> * patch ingnore_int everywhere into idt_table
> * call into C
> * patch real trap handlers into idt_table
> * load system tables such that faults all work.
> 
> Therefore, the ignore_int() patching is almost completely pointless,
> which is why it is completely removed in the subsequent patch.

Oh, right, this is indeed the last action before calling __start_xen.

>>> @@ -345,6 +338,10 @@ void start_secondary(void *unused)
>>>       */
>>>      spin_debug_disable();
>>>  
>>> +    load_system_tables();
>>> +
>>> +    /* Full trap support from here on in */
>>> +
>>>      percpu_traps_init();
>> I guess this ends up being confusing, even if it's just because the
>> name of the function perhaps no longer reflects its purpose.
> 
> Yes - there is a little more poor resultant naming than I would like.  I
> am also not completely happy with the names "load_system_tables()" and
> "init_idt_traps()", but can't think of more appropriate names offhand.

I think those latter names are okay. I may just be desirable to
rename percpu_traps_init() along with all the traps related stuff
getting removed from it.

>>> @@ -3515,23 +3515,40 @@ void __init trap_init(void)
>>>      set_intr_gate(TRAP_bounds,&bounds);
>>>      set_intr_gate(TRAP_invalid_op,&invalid_op);
>>>      set_intr_gate(TRAP_no_device,&device_not_available);
>>> +    set_intr_gate(TRAP_double_fault, &double_fault);
>>>      set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
>>>      set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
>>>      set_intr_gate(TRAP_no_segment,&segment_not_present);
>>>      set_intr_gate(TRAP_stack_error,&stack_segment);
>>>      set_intr_gate(TRAP_gp_fault,&general_protection);
>>> -    set_intr_gate(TRAP_page_fault,&page_fault);
>>> +    set_intr_gate(TRAP_page_fault,&early_page_fault);
>>>      set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
>>>      set_intr_gate(TRAP_copro_error,&coprocessor_error);
>>>      set_intr_gate(TRAP_alignment_check,&alignment_check);
>>>      set_intr_gate(TRAP_machine_check,&machine_check);
>>>      set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
>>>  
>>> +    /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
>>> +    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
>>> +    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
>>> +    set_ist(&idt_table[TRAP_machine_check], IST_MCE);
>> Mind putting them right aside the respective set_intr_gate() above?
> 
> Sorted in a later patch.  For now I think it is clearer as obvious code
> motion out of cpu_init().

Okay, if a subsequent patch deals with that, I guess it's fine then
this way.

>>> +void __init trap_init(void)
>>> +{
>>> +    set_intr_gate(TRAP_page_fault, &page_fault);
>>> +
>>> +    /* The 32-on-64 hypercall entry vector is only accessible from ring 1. 
>>> */
>>> +    _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, 
>>> &compat_hypercall);
>>> +
>>> +    /* Fast trap for int80 (faster than taking the #GP-fixup path). */
>>> +    _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
>> Long lines?
> 
> Again, code motion, although this long line was because of
> "DESC_TYPE_trap_gate".  "SYS_DESC_trap" in the first patch would fix the
> issue.

Adjusting formatting while moving code is not a problem I would say,
and should be done if the code was violating coding style.

Jan

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