SMI handler should set the CPL to zero and save and restore it on rsm.

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

Il 15/05/2014 03:32, Kevin O'Connor ha scritto:
> 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.
>
> --- 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
>

Looks like a bug entering VM86 mode.  I'll take a look, thanks for 
confirming what works and what doesn't!

Paolo