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

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

The following reply was made to PR port-xen/53267; it has been noted by GNATS.

From: Cherry G.Mathew <cherry%zyx.in@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost, kre%munnari.OZ.AU@localhost
Subject: Re: port-xen/53267 (XEN amd64 DomU (Dom0??) NetBSD current USERMODE() fails)
Date: Fri, 11 May 2018 17:06:08 +0530

 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: