Re: [Qemu-devel] [PATCH] SMI handler should set the CPL to zero and save and restore it on rsm.

[Kevin O'Connor <kevin@koconnor.net>]

On Wed, May 14, 2014 at 08:20:59PM -0400, Kevin O'Connor wrote:
> On Wed, May 14, 2014 at 10:05:47AM +0200, Paolo Bonzini wrote:
> > CPL isn't even altered when CS is reloaded, because you cannot jump out
> > of ring-0 except with an inter-privilege IRET, and that reloads SS too.
> > 
> > An IRET or task switch is also the only way to set EFLAGS.VM, and it will
> > hardcode SS.DPL=3, again matching CPL=3.
> > 
> > Finally, to get out of real mode you need to have CPL=0, and whatever got
> > you at CPL has also loaded SS with a ring-0 stack.  This means that SS.DPL=0
> > right after clearing CR0.PE.
> > 
> > Using SS.DPL as the CPL really sounds like the right approach.  I 
> > tried it on my KVM testcase, and it works well.  For QEMU, even the
> > special case of SYSRET will be handled fine because QEMU does set 
> > SS.DPL = 3:
> > 
> >         cpu_x86_load_seg_cache(env, R_SS, selector + 8,
> >                                0, 0xffffffff,
> >                                DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |
> >                                DESC_S_MASK | (3 << DESC_DPL_SHIFT) |
> >                                DESC_W_MASK | DESC_A_MASK);
> > 
> > SS.DPL=CPL=3, SS.RPL=selector & 3 is a mix of Intel behavior (which is 
> > SS.DPL=SS.RPL=CPL=3) and AMD behavior (because they set CPL=3 but 
> > SS.DPL=SS.RPL=selector & 3).  We may want to match Intel behavior,
> > but that's a different change.
> > 
> > Can you check if this patch works for you, and if so reply with
> > Tested-by/Reviewed-by?
> 
> Your patch causes Freedos to crash when emm386 is loaded, so I think
> it broke VM86 mode.  Below are some logs I took from qemu at the point
> of the crash.

FYI, with the patch below my quick test cases all look okay.

-Kevin


--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -974,7 +974,6 @@ static inline void cpu_x86_load_seg_cache(CPUX86State *env,
     /* update the hidden flags */
     {
         if (seg_reg == R_CS) {
-            int cpl = selector & 3;
 #ifdef TARGET_X86_64
             if ((env->hflags & HF_LMA_MASK) && (flags & DESC_L_MASK)) {
                 /* long mode */
@@ -984,16 +983,17 @@ static inline void cpu_x86_load_seg_cache(CPUX86State *env,
 #endif
             {
                 /* legacy / compatibility case */
-                if (!(env->cr[0] & CR0_PE_MASK)) {
-                    cpl = 0;
-                } else if (env->eflags & VM_MASK) {
-                    cpl = 3;
-                }
                 new_hflags = (env->segs[R_CS].flags & DESC_B_MASK)
                     >> (DESC_B_SHIFT - HF_CS32_SHIFT);
                 env->hflags = (env->hflags & ~(HF_CS32_MASK | HF_CS64_MASK)) |
                     new_hflags;
             }
+        }
+        if (seg_reg == R_SS) {
+            int cpl = (flags >> DESC_DPL_SHIFT) & 3;
+            if (env->eflags & VM_MASK) {
+                cpl = 3;
+            }
 #if HF_CPL_MASK != 3
 #error HF_CPL_MASK is hardcoded
 #endif