Re: [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap()

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

On 01/09/14 17:10, Don Slutz wrote:
> On 09/01/14 06:06, Andrew Cooper wrote:
>> It is always available via regs->entry_vector.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>   xen/arch/x86/cpu/mcheck/mce.c   |    2 +-
>>   xen/arch/x86/nmi.c              |    2 +-
>>   xen/arch/x86/traps.c            |   13 +++++++------
>>   xen/arch/x86/x86_64/entry.S     |    3 +--
>>   xen/include/asm-x86/processor.h |    2 +-
>>   5 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/mcheck/mce.c
>> b/xen/arch/x86/cpu/mcheck/mce.c
>> index 812daf6..05a86fb 100644
>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>> @@ -74,7 +74,7 @@ static void unexpected_machine_check(const struct
>> cpu_user_regs *regs)
>>   {
>>       console_force_unlock();
>>       printk("Unexpected Machine Check Exception\n");
>> -    fatal_trap(TRAP_machine_check, regs);
>> +    fatal_trap(regs);
>>   }
>>     diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
>> index 7d15d5b..055f4ef 100644
>> --- a/xen/arch/x86/nmi.c
>> +++ b/xen/arch/x86/nmi.c
>> @@ -472,7 +472,7 @@ bool_t nmi_watchdog_tick(const struct
>> cpu_user_regs *regs)
>>               console_force_unlock();
>>               printk("Watchdog timer detects that CPU%d is stuck!\n",
>>                      smp_processor_id());
>> -            fatal_trap(TRAP_nmi, regs);
>> +            fatal_trap(regs);
>>           }
>>       }
>>       else
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 7f5306f..10fc2ca 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -394,9 +394,10 @@ static const char *trapstr(unsigned int trapnr)
>>    * are disabled). In such situations we can't do much that is safe.
>> We try to
>>    * print out some tracing and then we just spin.
>>    */
>> -void fatal_trap(int trapnr, const struct cpu_user_regs *regs)
>> +void fatal_trap(const struct cpu_user_regs *regs)
>>   {
>>       static DEFINE_PER_CPU(char, depth);
>> +    unsigned int trapnr = regs->entry_vector;
>
> Should this be:
>
> unsigned int trapnr = regs->entry_vector &
> ~(TRAP_regs_partial|TRAP_syscall);
>
>
> To get rid of the extra bits?
>
>   -Don Slutz

No; I don't think so.  There should be no paths into fatal_trap() which
would set these bits, as these bits are only set as a result of guest
sys/hypercall interaction, while fatal_trap() is a terminal error condition.

Furthermore, if a path is discovered then a) it will be more obvious
from the error message and b) we probably have a security problem to fix.

~Andrew

>
>>         /* Set AC to reduce chance of further SMAP faults */
>>       stac();
>> @@ -1427,7 +1428,7 @@ void do_page_fault(struct cpu_user_regs *regs)
>>           {
>>               console_start_sync();
>>               printk("Xen SM%cP violation\n", (pf_type == smep_fault)
>> ? 'E' : 'A');
>> -            fatal_trap(TRAP_page_fault, regs);
>> +            fatal_trap(regs);
>>           }
>>             if ( pf_type != real_fault )
>> @@ -1498,7 +1499,7 @@ void __init do_early_page_fault(struct
>> cpu_user_regs *regs)
>>           console_start_sync();
>>           printk("Early fatal page fault at %04x:%p (cr2=%p,
>> ec=%04x)\n",
>>                  regs->cs, _p(regs->eip), _p(cr2), regs->error_code);
>> -        fatal_trap(TRAP_page_fault, regs);
>> +        fatal_trap(regs);
>>       }
>>   }
>>   @@ -3256,7 +3257,7 @@ static void pci_serr_error(const struct
>> cpu_user_regs *regs)
>>       default:  /* 'fatal' */
>>           console_force_unlock();
>>           printk("\n\nNMI - PCI system error (SERR)\n");
>> -        fatal_trap(TRAP_nmi, regs);
>> +        fatal_trap(regs);
>>       }
>>   }
>>   @@ -3271,7 +3272,7 @@ static void io_check_error(const struct
>> cpu_user_regs *regs)
>>       default:  /* 'fatal' */
>>           console_force_unlock();
>>           printk("\n\nNMI - I/O ERROR\n");
>> -        fatal_trap(TRAP_nmi, regs);
>> +        fatal_trap(regs);
>>       }
>>         outb((inb(0x61) & 0x0f) | 0x08, 0x61); /* clear-and-disable
>> IOCK */
>> @@ -3291,7 +3292,7 @@ static void unknown_nmi_error(const struct
>> cpu_user_regs *regs, unsigned char re
>>           console_force_unlock();
>>           printk("Uhhuh. NMI received for unknown reason %02x.\n",
>> reason);
>>           printk("Do you have a strange power saving mode enabled?\n");
>> -        fatal_trap(TRAP_nmi, regs);
>> +        fatal_trap(regs);
>>       }
>>   }
>>   diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>> index a3ed216..42835d0 100644
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -536,8 +536,7 @@ exception_with_ints_disabled:
>>     /* No special register assumptions. */
>>   FATAL_exception_with_ints_disabled:
>> -        movzbl UREGS_entry_vector(%rsp),%edi
>> -        movq  %rsp,%rsi
>> +        movq  %rsp,%rdi
>>           call  fatal_trap
>>           ud2
>>   diff --git a/xen/include/asm-x86/processor.h
>> b/xen/include/asm-x86/processor.h
>> index a156e01..9e1f210 100644
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -481,7 +481,7 @@ void show_registers(const struct cpu_user_regs
>> *regs);
>>   void show_execution_state(const struct cpu_user_regs *regs);
>>   #define dump_execution_state()
>> run_in_exception_handler(show_execution_state)
>>   void show_page_walk(unsigned long addr);
>> -void noreturn fatal_trap(int trapnr, const struct cpu_user_regs *regs);
>> +void noreturn fatal_trap(const struct cpu_user_regs *regs);
>>     void compat_show_guest_stack(struct vcpu *v,
>>                                const struct cpu_user_regs *regs, int
>> lines);
>