Re: port-xen/53267 (XEN amd64 DomU (Dom0??) NetBSD current USERMODE() fails)

[Cherry G.Mathew <cherry%zyx.in@localhost>]

Robert Elz <kre%munnari.OZ.AU@localhost> writes:

> The following reply was made to PR port-xen/53267; it has been noted by GNATS.
>
> From: Robert Elz <kre%munnari.OZ.AU@localhost>
> To: "Cherry G.Mathew" <cherry%zyx.in@localhost>
> Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%netbsd.org@localhost
> Subject: Re: port-xen/53267 (XEN amd64 DomU (Dom0??) NetBSD current USERMODE() fails)
> Date: Fri, 11 May 2018 17:38:09 +0700
>
>      Date:        Fri, 11 May 2018 14:37:12 +0530
>      From:        "Cherry G.Mathew" <cherry%zyx.in@localhost>
>      Message-ID:  <87o9hm7e7z.fsf%zyx.in@localhost>
>  
>    | This was a pretty intrusive change ( we switched to the x86 common
>    | interrupt code ), so I'll need a bit of time to figure it out.
>  
>  Yes, I saw ... and also explains the problem, as regular x86 run the
>  kernel and users at different priv levels, but xen does not.   The old
>  way, some magic must have been being done to make it appear as if
>  there were different levels, which is no longer happening.

Actually it's a bit more complicated, depending on if you're talking
about amd64 or i386. 

In any case, the Xen hypervisor trap code "emulates" CPL by frobbing the
appropriate CS register bitfield in the saved stackframe. 

On amd64,
From amd64/include/segments.h:

#ifdef XEN
#define SEL_KPL         3               /* kernel privilege level */
#define SEL_XPL         0               /* Xen Hypervisor privilege
#level */
#else
#define SEL_KPL         0               /* kernel privilege level */
#endif

[...]

#ifdef XEN
/*
 * As KPL == UPL, Xen emulate interrupt in kernel context by pushing
 * a fake CS with XPL privilege
 */
#define KERNELMODE(c)           (ISPL(c) == SEL_XPL)
#else
#define KERNELMODE(c)           (ISPL(c) == SEL_KPL)
#endif

Thus although "KPL" is considered to be at level3 on amd64 (mostly for
bit flipping to do with re-using segment entry setup code for the kernel
from native but with the appropriately lowered privilege level), the
actual value of CS stored on the stack frame reflects what native code
would expect for the kernel - ie; a CPL of 0.

>  I have no idea how all of this works (I don't know x86 architecture at
>  all) so I cannot really help with a soltution (I admit to making one wild
>  guess, and trying it out - that kernel did not make it through autoconf
>  before hanging ... fortunately DomU's are easy to destroy, and bogus
>  code is easy to throw away!)
>  

Thanks for binary searching this - I really appreciate the patience it
took.

>  One suggestion though - there is no need for the XEN USERMODE()
>  macro to work anything like the x86 one does - it does not need to
>  inspect bits in %cs (whatever that is).   Other ports do not.   Any method
>  that tells what the mode was before the current interrupt/trap will work.
>  

It's a bit more complicated because of the various configurations
(xen/native/amd64/i386) that x86 supports. I probably don't want to
touch it myself.

Please find a patch that could bandaid the current problem. The issue is
that the interrupt despatch code actually passes the real stack frame as
a second parameter to all callbacks, however the clock handler is the
only one which actually uses it (and passes it on to hardclock(), where
I believe the USERMODE() related accounting takes place).

This fact got overlooked during the API change, so the regs parameter
that the clock handler got was basically nonsense.

There's a couple of other patches I have that will sort out this API
mess in a cleaner way at which point this kludge should go away.

Please let me know how this goes.

-- 
~cherry

Index: clock.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/clock.c,v
retrieving revision 1.65
diff -u -r1.65 clock.c
--- clock.c	6 Nov 2017 15:27:09 -0000	1.65
+++ clock.c	11 May 2018 11:21:12 -0000
@@ -49,7 +49,8 @@
 #include <dev/clock_subr.h>
 #include <x86/rtc.h>
 
-static int xen_timer_handler(void *);
+static int xen_timer_handler(void *, struct intrframe *);
+static int (*xen_timer_handler_stub)(void *) = (void *) xen_timer_handler;
 static struct intrhand *ih;
 
 /* A timecounter: Xen system_time extrapolated with a TSC. */
@@ -524,7 +525,7 @@
 	KASSERT(evtch != -1);
 
 	ih = intr_establish_xname(0, &xen_pic, evtch, IST_LEVEL, IPL_CLOCK,
-	    xen_timer_handler, ci, true, "clock");
+	    xen_timer_handler_stub, ci, true, "clock");
 
 	KASSERT(ih != NULL);
 
@@ -535,11 +536,10 @@
 
 /* ARGSUSED */
 static int
-xen_timer_handler(void *arg)
+xen_timer_handler(void *arg, struct intrframe *regs)
 {
 	int64_t delta;
 	struct cpu_info *ci = curcpu();
-	struct intrframe *regs = arg;
 
 	int err;
 again: