All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap()
@ 2014-09-01 10:06 Andrew Cooper
  2014-09-01 16:10 ` Don Slutz
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-09-01 10:06 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich

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;
 
     /* 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);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap()
  2014-09-01 10:06 [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap() Andrew Cooper
@ 2014-09-01 16:10 ` Don Slutz
  2014-09-01 16:27   ` Andrew Cooper
  0 siblings, 1 reply; 4+ messages in thread
From: Don Slutz @ 2014-09-01 16:10 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Jan Beulich, Xen-devel

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

>   
>       /* 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);

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap()
  2014-09-01 16:10 ` Don Slutz
@ 2014-09-01 16:27   ` Andrew Cooper
  2014-09-07  0:18     ` Don Slutz
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-09-01 16:27 UTC (permalink / raw)
  To: Don Slutz; +Cc: Jan Beulich, Xen-devel

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);
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap()
  2014-09-01 16:27   ` Andrew Cooper
@ 2014-09-07  0:18     ` Don Slutz
  0 siblings, 0 replies; 4+ messages in thread
From: Don Slutz @ 2014-09-07  0:18 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Jan Beulich, Don Slutz, Xen-devel


On 09/01/14 12:27, Andrew Cooper wrote:
> 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>
>>> ---

>>> -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

Ok.

Reviewed-by: Don Slutz <dslutz@verizon.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-07  0:18 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-01 10:06 [PATCH] x86/traps: Remove redundant trapnr parameter from fatal_trap() Andrew Cooper
2014-09-01 16:10 ` Don Slutz
2014-09-01 16:27   ` Andrew Cooper
2014-09-07  0:18     ` Don Slutz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.