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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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