Subject: Re: [patch V2 35/44] x86/idt: Switch early trap init to IDT tables
To: Thomas Gleixner <tglx@linutronix.de>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>,
        Peter Anvin <hpa@zytor.com>, Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Steven Rostedt <rostedt@goodmis.org>
References: <20170825214648.264521964@linutronix.de>
 <20170825214942.983480384@linutronix.de>
 <7e182f46-d27b-9414-fb6d-ee48906a4180@oracle.com>
 <alpine.DEB.2.20.1708261011001.2093@nanos>
From: Juergen Gross <jgross@suse.com>
Message-ID: <e92da904-7569-0916-2a3a-a65ac55cb408@suse.com>
Date: Sat, 26 Aug 2017 12:59:03 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.20.1708261011001.2093@nanos>
Content-Type: text/plain; charset=utf-8
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2101
Lines: 67

On 26/08/17 10:16, Thomas Gleixner wrote:
> On Fri, 25 Aug 2017, Boris Ostrovsky wrote:
>> On 08/25/2017 05:47 PM, Thomas Gleixner wrote:
>>>  
>>> -/* Set of traps needed for early debugging. */
>>> -void __init early_trap_init(void)
>>> -{
>>> -	/*
>>> -	 * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
>>> -	 * is ready in cpu_init() <-- trap_init(). Before trap_init(),
>>> -	 * CPU runs at ring 0 so it is impossible to hit an invalid
>>> -	 * stack.  Using the original stack works well enough at this
>>> -	 * early stage. DEBUG_STACK will be equipped after cpu_init() in
>>> -	 * trap_init().
>>> -	 */
>>> -	set_intr_gate(X86_TRAP_DB, debug);
>>> -	/* int3 can be called from all */
>>> -	set_system_intr_gate(X86_TRAP_BP, &int3);
>>> -#ifdef CONFIG_X86_32
>>> -	set_intr_gate(X86_TRAP_PF, page_fault);
>>> -#endif
>>> -	load_idt(&idt_descr);
>>
>> This breaks 64-bit Xen PV guests: idt_setup_early_handler() is called
>> from x86_64_start_kernel(), which is not executed on those guests.
> 
> early_trap_init() is called from setup_arch(), not from
> x86_64_start_kernel().
> 
> This patch merily renames the function and moves the code into a different
> source file. So it's not changing the behaviour in any way.

It does: the IDT entries are now statically defined. Before that any
call of set_intr_gate() would have registered the trap handler with
the hypervisor, now this is done only when the IDT is being activated.
Boris' patch is doing that.

> 
>> The patch below fixes it.
> 
> I doubt that :)

Me not. :-)


Juergen

> 
> Thanks,
> 
> 	tglx
> 
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index 03fb07d..ed0fb8a 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1374,6 +1374,7 @@ asmlinkage __visible void __init
>> xen_start_kernel(void)
>>         i386_start_kernel();
>>  #else
>>         cr4_init_shadow(); /* 32b kernel does this in i386_start_kernel() */
>> +       load_idt(&idt_descr);
>>         x86_64_start_reservations((char *)__pa_symbol(&boot_params));
>>  #endif
>>  }
> 
>