Xen devel - May 2009 - [PATCH 2/5] xen/x86-64: clean up warnings about IST-using traps

Ignore known IST-using traps.  Aside from the debugger traps, they''re
low-level faults which Xen will handle for us, so the kernel needn''t
worry about them.  Keep warning in case unknown trap starts using IST.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/xen/enlighten.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index cb49f57..88f3aa4 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -439,12 +439,30 @@ static int cvt_gate_to_trap(int vector, const gate_desc
*val,
 
 	addr = gate_offset(*val);
 #ifdef CONFIG_X86_64
+	/*
+	 * Look for known traps using IST, and substitute them
+	 * appropriately.  The debugger ones are the only ones we care
+	 * about.  Xen will handle faults like double_fault and
+	 * machine_check, so we should never see them.  Warn if
+	 * there''s an unexpected IST-using fault handler.
+	 */
 	if (addr == (unsigned long)debug)
 		addr = (unsigned long)xen_debug;
 	else if (addr == (unsigned long)int3)
 		addr = (unsigned long)xen_int3;
-	else
-		WARN_ON(val->ist != 0);
+	else if (addr == (unsigned long)double_fault ||
+		 addr == (unsigned long)stack_segment) {
+		/* Don''t need to handle these */
+		return 0;
+#ifdef CONFIG_X86_MCE
+	} else if (addr == (unsigned long)machine_check) {
+		return 0;
+#endif
+	} else {
+		/* Some other trap using IST? */
+		if (WARN_ON(val->ist != 0))
+			return 0;
+	}
 #endif	/* CONFIG_X86_64 */
 	info->address = addr;
 
-- 
1.6.0.6




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Jeremy Fitzhardinge <jeremy@goop.org> 07.05.09 20:56
>>>
>Ignore known IST-using traps.  Aside from the debugger traps,
they''re
>low-level faults which Xen will handle for us, so the kernel
needn''t
>worry about them.  Keep warning in case unknown trap starts using IST.
>
>Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>---
> arch/x86/xen/enlighten.c |   22 ++++++++++++++++++++--
> 1 files changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>index cb49f57..88f3aa4 100644
>--- a/arch/x86/xen/enlighten.c
>+++ b/arch/x86/xen/enlighten.c
>@@ -439,12 +439,30 @@ static int cvt_gate_to_trap(int vector, const
gate_desc *val,
> 
> 	addr = gate_offset(*val);
> #ifdef CONFIG_X86_64
>+	/*
>+	 * Look for known traps using IST, and substitute them
>+	 * appropriately.  The debugger ones are the only ones we care
>+	 * about.  Xen will handle faults like double_fault and
>+	 * machine_check, so we should never see them.  Warn if
>+	 * there''s an unexpected IST-using fault handler.
>+	 */
> 	if (addr == (unsigned long)debug)
> 		addr = (unsigned long)xen_debug;
> 	else if (addr == (unsigned long)int3)
> 		addr = (unsigned long)xen_int3;
>-	else
>-		WARN_ON(val->ist != 0);
>+	else if (addr == (unsigned long)double_fault ||
>+		 addr == (unsigned long)stack_segment) {
I don''t think you want to exclude handling stack faults: Ordinary
memory
references using rsp or rbp as the base register will cause these instead
of general protection faults when the resulting effective address is non-
canonical.
>+		/* Don''t need to handle these */
>+		return 0;
>+#ifdef CONFIG_X86_MCE
>+	} else if (addr == (unsigned long)machine_check) {
>+		return 0;
>+#endif
>+	} else {
>+		/* Some other trap using IST? */
>+		if (WARN_ON(val->ist != 0))
>+			return 0;
>+	}
> #endif	/* CONFIG_X86_64 */
> 	info->address = addr;
> 
>-- 
>1.6.0.6
Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Jan Beulich <jbeulich@novell.com> wrote:
> >>> Jeremy Fitzhardinge <jeremy@goop.org> 07.05.09 20:56
>>>
> >Ignore known IST-using traps.  Aside from the debugger traps,
they''re
> >low-level faults which Xen will handle for us, so the kernel
needn''t
> >worry about them.  Keep warning in case unknown trap starts using IST.
> >
> >Signed-off-by: Jeremy Fitzhardinge
<jeremy.fitzhardinge@citrix.com>
> >---
> > arch/x86/xen/enlighten.c |   22 ++++++++++++++++++++--
> > 1 files changed, 20 insertions(+), 2 deletions(-)
> >
> >diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >index cb49f57..88f3aa4 100644
> >--- a/arch/x86/xen/enlighten.c
> >+++ b/arch/x86/xen/enlighten.c
> >@@ -439,12 +439,30 @@ static int cvt_gate_to_trap(int vector, const
gate_desc *val,
> > 
> > 	addr = gate_offset(*val);
> > #ifdef CONFIG_X86_64
> >+	/*
> >+	 * Look for known traps using IST, and substitute them
> >+	 * appropriately.  The debugger ones are the only ones we care
> >+	 * about.  Xen will handle faults like double_fault and
> >+	 * machine_check, so we should never see them.  Warn if
> >+	 * there''s an unexpected IST-using fault handler.
> >+	 */
> > 	if (addr == (unsigned long)debug)
> > 		addr = (unsigned long)xen_debug;
> > 	else if (addr == (unsigned long)int3)
> > 		addr = (unsigned long)xen_int3;
> >-	else
> >-		WARN_ON(val->ist != 0);
> >+	else if (addr == (unsigned long)double_fault ||
> >+		 addr == (unsigned long)stack_segment) {
> 
> I don''t think you want to exclude handling stack faults: Ordinary 
> memory references using rsp or rbp as the base register will cause 
> these instead of general protection faults when the resulting 
> effective address is non- canonical.
Yes. Also, patches 1-2-3 really just do the same thing, one fixes 
the other one. Would be nice to have a single patch for that whole 
ugly topic of 64-bit Xen not handling ISTs properly.

If this was a CPU we''d say "sorry, dont run Linux on it then"
...

I''ve applied the reservation fix to x86/urgent, and the #5 patch to 
x86/xen (it''s more of a cleanup, not a fix for .30, right?).

	Ingo

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
>> else if (addr == (unsigned long)int3)
>> 		addr = (unsigned long)xen_int3;
>> -	else
>> -		WARN_ON(val->ist != 0);
>> +	else if (addr == (unsigned long)double_fault ||
>> +		 addr == (unsigned long)stack_segment) {
>>     
>
> I don''t think you want to exclude handling stack faults: Ordinary
memory
> references using rsp or rbp as the base register will cause these instead
> of general protection faults when the resulting effective address is non-
> canonical.
>   
So even usermode memory accesses?  Ew.  Well, we can run the trap 
handler on the normal kernel stack in that case.  I''ll cook something
up.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Jeremy Fitzhardinge <jeremy@goop.org> 08.05.09 17:23
>>>
>Jan Beulich wrote:
>>> else if (addr == (unsigned long)int3)
>>> 		addr = (unsigned long)xen_int3;
>>> -	else
>>> -		WARN_ON(val->ist != 0);
>>> +	else if (addr == (unsigned long)double_fault ||
>>> +		 addr == (unsigned long)stack_segment) {
>>>     
>>
>> I don''t think you want to exclude handling stack faults:
Ordinary memory
>> references using rsp or rbp as the base register will cause these
instead
>> of general protection faults when the resulting effective address is
non-
>> canonical.
>>   
>
>So even usermode memory accesses?  Ew.
Yes, just try it out - you''ll observe them generate SIGBUS instead of
SIGSEGV.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ingo Molnar wrote:
> Yes. Also, patches 1-2-3 really just do the same thing, one fixes 
> the other one. Would be nice to have a single patch for that whole 
> ugly topic of 64-bit Xen not handling ISTs properly.
>   
Well, 1 is sufficient on its own to fix the breakpoint/watchpoint 
problem.  Looks like I need to do something similar with stack.  The 2,3 
are more general cleanups that aren''t essential to the functional fix, 
and should definitely be folded together.
> If this was a CPU we''d say "sorry, dont run Linux on it
then" ...
>   
??  Guests don''t really need IST because all the tricky traps that 
really require it (NMI, MCE, etc) are handled by Xen.  The stack 
exception was an oversight on my part, because I thought it was 
something that would only happen with a bad kernel stack (but I guess 
that''s doublefault).
> I''ve applied the reservation fix to x86/urgent, and the #5 patch
to
> x86/xen (it''s more of a cleanup, not a fix for .30, right?).
>   
Right.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel