[PATCH RFC] x86/traps: Make the main trap handlers safe for use early during Xen boot

[PATCH RFC] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Andrew Cooper]

Most of this patch is an analysis of the safety of the trap handlers.

Traps 0, 4, 5, 9-12, 16, 17 and 19 all end up in do_trap().  do_trap() is
mostly safe, performing an exception table search and possibly panic()s.

There is one complication with traps 16 and 19 which will see about calling
the fpu_exception_callback.  This involves following current which is not
valid early on boot.  The has_hvm_container_vcpu(curr) check is preceeded with
a system_state check, so in the exceedingly unlikely case that Xen takes an
x87/SIMP trap while booting, it will panic() instead of following a bogus
current vcpu.

Traps 1, 3, 6-8, 13 and 15 are completely safe with respect to running during
early boot.  They all have well formed and obvious differences between faults
in Xen and faults in guests, with the Xen faults doing little more than
exception table walks or panic()s.

Trap 2 is a complicted codepath, but appears safe.  For the possible injection
of NMIs into dom0 there is a NULL domian pointer check.  The possible softirq
raised for PCI SERR will be devered until we start the idle vcpu, but is safe.

Trap 14 is very complicated.  The code is certainly unsafe for boot as
fixup_page_fault() will dereference current to find the running domain.  There
exists an explicit do_early_page_fault() handler which shall continue to be
used.

Trap 18 has a default handler before the MCE infrastructure is set up, which
has always been unsafe and liable to deadlock itself with the console lock.
As it is expected never to trigger, and if it did we would be in serious
problems, the simple printk() is replaced with a fatal error path.

Trap 20 (Virtualisation Exception) is currently not implemented.  It is fatal
one way or another, and will become more explicitly so with later changes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

I would appreciate some careful review of this.  As part of developing the
other early init fixes, I will test what I can of this, but doubt it will be
comprehensive testing.
---
 xen/arch/x86/cpu/mcheck/mce.c |    5 +++--
 xen/arch/x86/traps.c          |    3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index a81952c..5488411 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -72,8 +72,9 @@ custom_param("mce_verbosity", mce_set_verbosity);
 /* Handle unconfigured int18 (should never happen) */
 static void unexpected_machine_check(struct cpu_user_regs *regs, long error_code)
 {
-    printk(XENLOG_ERR "CPU#%d: Unexpected int18 (Machine Check).\n",
-           smp_processor_id());
+    console_force_unlock();
+    printk("Unexpected Machine Check Exception");
+    fatal_trap(TRAP_machine_check, regs);
 }
 
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 40366f1..34c4589 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -561,7 +561,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code)
     }
 
     if ( ((trapnr == TRAP_copro_error) || (trapnr == TRAP_simd_error)) &&
-         has_hvm_container_vcpu(curr) && curr->arch.hvm_vcpu.fpu_exception_callback )
+         system_state == SYS_STATE_active && has_hvm_container_vcpu(curr) &&
+         curr->arch.hvm_vcpu.fpu_exception_callback )
     {
         curr->arch.hvm_vcpu.fpu_exception_callback(
             curr->arch.hvm_vcpu.fpu_exception_callback_arg, regs);
-- 
1.7.10.4

Re: [PATCH RFC] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Jan Beulich]

>>> On 13.05.14 at 16:51, <andrew.cooper3@citrix.com> wrote:
> Most of this patch is an analysis of the safety of the trap handlers.
> 
> Traps 0, 4, 5, 9-12, 16, 17 and 19 all end up in do_trap().  do_trap() is
> mostly safe, performing an exception table search and possibly panic()s.
> 
> There is one complication with traps 16 and 19 which will see about calling
> the fpu_exception_callback.  This involves following current which is not
> valid early on boot.  The has_hvm_container_vcpu(curr) check is preceeded 
> with
> a system_state check, so in the exceedingly unlikely case that Xen takes an
> x87/SIMP trap while booting, it will panic() instead of following a bogus
> current vcpu.
> 
> Traps 1, 3, 6-8, 13 and 15 are completely safe with respect to running during
> early boot.  They all have well formed and obvious differences between faults
> in Xen and faults in guests, with the Xen faults doing little more than
> exception table walks or panic()s.

I think 15 should be moved into the undefined / reserved category too.
Nothing should be generating this.

I would similarly question 9 (TRAP_copro_seg), which supposedly is
valid only for old 32-bit CPUs.

> I would appreciate some careful review of this.  As part of developing the
> other early init fixes, I will test what I can of this, but doubt it will be
> comprehensive testing.

The main thing I wonder is why you submit this separately - this is
going to become useful only once these get wired up earlier than
they are right now.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -561,7 +561,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code)
>      }
>  
>      if ( ((trapnr == TRAP_copro_error) || (trapnr == TRAP_simd_error)) &&
> -         has_hvm_container_vcpu(curr) && curr->arch.hvm_vcpu.fpu_exception_callback )
> +         system_state == SYS_STATE_active && has_hvm_container_vcpu(curr) &&

This seems too specific a check - I think this ought to be "system_state >=
SYS_STATE_active".

Jan

Re: [PATCH RFC] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Andrew Cooper]

On 13/05/14 16:26, Jan Beulich wrote:
>>>> On 13.05.14 at 16:51, <andrew.cooper3@citrix.com> wrote:
>> Most of this patch is an analysis of the safety of the trap handlers.
>>
>> Traps 0, 4, 5, 9-12, 16, 17 and 19 all end up in do_trap().  do_trap() is
>> mostly safe, performing an exception table search and possibly panic()s.
>>
>> There is one complication with traps 16 and 19 which will see about calling
>> the fpu_exception_callback.  This involves following current which is not
>> valid early on boot.  The has_hvm_container_vcpu(curr) check is preceeded 
>> with
>> a system_state check, so in the exceedingly unlikely case that Xen takes an
>> x87/SIMP trap while booting, it will panic() instead of following a bogus
>> current vcpu.
>>
>> Traps 1, 3, 6-8, 13 and 15 are completely safe with respect to running during
>> early boot.  They all have well formed and obvious differences between faults
>> in Xen and faults in guests, with the Xen faults doing little more than
>> exception table walks or panic()s.
> I think 15 should be moved into the undefined / reserved category too.
> Nothing should be generating this.
>
> I would similarly question 9 (TRAP_copro_seg), which supposedly is
> valid only for old 32-bit CPUs.

Agreed - I was going to move them into the reserved-exception category
with a later patch in the series, but their current handling is safe
with respect to their current handling.

>
>> I would appreciate some careful review of this.  As part of developing the
>> other early init fixes, I will test what I can of this, but doubt it will be
>> comprehensive testing.
> The main thing I wonder is why you submit this separately - this is
> going to become useful only once these get wired up earlier than
> they are right now.

For some early review, especially if there were glaring holes in my
logic.  I am currently working on the bsp changes.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -561,7 +561,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code)
>>      }
>>  
>>      if ( ((trapnr == TRAP_copro_error) || (trapnr == TRAP_simd_error)) &&
>> -         has_hvm_container_vcpu(curr) && curr->arch.hvm_vcpu.fpu_exception_callback )
>> +         system_state == SYS_STATE_active && has_hvm_container_vcpu(curr) &&
> This seems too specific a check - I think this ought to be "system_state >=
> SYS_STATE_active".
>
> Jan
>

I considered that, but the valid values greater than active are suspend
and resume, which absolutely shouldn't be running x86_emulate
codepaths.  I don't think it is safe to assume that any future values
greater than active will be safe contexts for this.

~Andrew

Re: [PATCH RFC] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Jan Beulich]

>>> On 13.05.14 at 18:13, <andrew.cooper3@citrix.com> wrote:
> On 13/05/14 16:26, Jan Beulich wrote:
>>>>> On 13.05.14 at 16:51, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -561,7 +561,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code)
>>>      }
>>>  
>>>      if ( ((trapnr == TRAP_copro_error) || (trapnr == TRAP_simd_error)) &&
>>> -         has_hvm_container_vcpu(curr) && curr->arch.hvm_vcpu.fpu_exception_callback )
>>> +         system_state == SYS_STATE_active && has_hvm_container_vcpu(curr) &&
>> This seems too specific a check - I think this ought to be "system_state >=
>> SYS_STATE_active".
> 
> I considered that, but the valid values greater than active are suspend
> and resume, which absolutely shouldn't be running x86_emulate
> codepaths.  I don't think it is safe to assume that any future values
> greater than active will be safe contexts for this.

I can see your point, but my perspective is different: All you really
want to guard against here is de-referencing a non yet properly set
current. And that would be achieved with the range check, not the
equality one.

Jan