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

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

[Paolo Bonzini]

Il 27/04/2014 19:25, Kevin O'Connor ha scritto:
> I was wondering about that as well.  The Intel docs state that the CPL
> is bits 0-1 of the CS.selector register, and that protected mode
> starts immediately after setting the PE bit.  The CS.selector field
> should be the value of %cs in real mode, which is the value added to
> eip (after shifting right by 4).
> 
> I guess that means that the real mode code that enables the PE bit
> must run with a code segment aligned to a value of 4.  (Which
> effectively means code alignment of 64 bytes because of the segment
> shift.)

It turns out that this is not a requirement; which means that the 
protected mode transition is exactly the only place where CPL is not 
redundant.  The CPL remains zero until you reload CS with a long jump.

Your patch gets it right because after a CR0 write it doesn't attempt 
to recompute the CPL, but you need the following partial revert in 
order to satisfy virtualization extensions (SVM).  Without it, the
guest will triple fault after setting CR0.PE=1, unless CS's low 2 bits
are 00.  The hypervisor gets a CR0_WRITE vmexit, but then the processor
fails to execute guest code from a non-conforming ring-0 code segment
at CPL>0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e9cbdab..478f356 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -124,9 +124,9 @@
 #define ID_MASK                 0x00200000
 
 /* hidden flags - used internally by qemu to represent additional cpu
-   states. Only the INHIBIT_IRQ, SMM and SVMI are not redundant. We
-   avoid using the IOPL_MASK, TF_MASK, VM_MASK and AC_MASK bit
-   positions to ease oring with eflags. */
+   states. Only the CPL, INHIBIT_IRQ, SMM and SVMI are not
+   redundant. We avoid using the IOPL_MASK, TF_MASK, VM_MASK and AC_MASK
+   bit positions to ease oring with eflags. */
 /* current cpl */
 #define HF_CPL_SHIFT         0
 /* true if soft mmu is being used */
@@ -1052,6 +1052,16 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
                             target_ulong *base, unsigned int *limit,
                             unsigned int *flags);
 
+/* wrapper, just in case memory mappings must be changed */
+static inline void cpu_x86_set_cpl(CPUX86State *s, int cpl)
+{
+#if HF_CPL_MASK == 3
+    s->hflags = (s->hflags & ~HF_CPL_MASK) | cpl;
+#else
+#error HF_CPL_MASK is hardcoded
+#endif
+}
+
 /* op_helper.c */
 /* used for debug or cpu save/restore */
 void cpu_get_fp80(uint64_t *pmant, uint16_t *pexp, floatx80 f);
diff --git a/target-i386/svm_helper.c b/target-i386/svm_helper.c
index 846eaa5..29ca012 100644
--- a/target-i386/svm_helper.c
+++ b/target-i386/svm_helper.c
@@ -282,6 +282,9 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
                           env->vm_vmcb + offsetof(struct vmcb, save.dr7));
     env->dr[6] = ldq_phys(cs->as,
                           env->vm_vmcb + offsetof(struct vmcb, save.dr6));
+    cpu_x86_set_cpl(env, ldub_phys(cs->as,
+                                   env->vm_vmcb + offsetof(struct vmcb,
+                                                           save.cpl)));
 
     /* FIXME: guest state consistency checks */

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

[Kevin O'Connor]

On Tue, May 13, 2014 at 08:24:47PM +0200, Paolo Bonzini wrote:
> Il 27/04/2014 19:25, Kevin O'Connor ha scritto:
> > I was wondering about that as well.  The Intel docs state that the CPL
> > is bits 0-1 of the CS.selector register, and that protected mode
> > starts immediately after setting the PE bit.  The CS.selector field
> > should be the value of %cs in real mode, which is the value added to
> > eip (after shifting right by 4).
> > 
> > I guess that means that the real mode code that enables the PE bit
> > must run with a code segment aligned to a value of 4.  (Which
> > effectively means code alignment of 64 bytes because of the segment
> > shift.)
> 
> It turns out that this is not a requirement; which means that the 
> protected mode transition is exactly the only place where CPL is not 
> redundant.  The CPL remains zero until you reload CS with a long jump.

That doesn't sound right.  What happens if the processor takes an NMI,
SMI, or VMEXIT between the point it enables protected mode but before
it long jumps?  The processor would have to save and restore the CPL
somewhere for all of these situations.

If you are right, I think the whole series needs to be reworked.

-Kevin

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

[Paolo Bonzini]

Il 13/05/2014 20:39, Kevin O'Connor ha scritto:
> On Tue, May 13, 2014 at 08:24:47PM +0200, Paolo Bonzini wrote:
>> Il 27/04/2014 19:25, Kevin O'Connor ha scritto:
>>> I was wondering about that as well.  The Intel docs state that the CPL
>>> is bits 0-1 of the CS.selector register, and that protected mode
>>> starts immediately after setting the PE bit.  The CS.selector field
>>> should be the value of %cs in real mode, which is the value added to
>>> eip (after shifting right by 4).
>>>
>>> I guess that means that the real mode code that enables the PE bit
>>> must run with a code segment aligned to a value of 4.  (Which
>>> effectively means code alignment of 64 bytes because of the segment
>>> shift.)
>>
>> It turns out that this is not a requirement; which means that the
>> protected mode transition is exactly the only place where CPL is not
>> redundant.  The CPL remains zero until you reload CS with a long jump.
>
> That doesn't sound right.  What happens if the processor takes an NMI,
> SMI, or VMEXIT between the point it enables protected mode but before
> it long jumps?  The processor would have to save and restore the CPL
> somewhere for all of these situations.

For VMEXITs it's up to the hypervisor to make it work properly.  I just 
posted today fixes for KVM.

I guess the answer for NMIs is "good luck".  But in the case of NMIs, 
wouldn't it be broken anyway, because the IDT format is different 
between real mode and protected mode?

For SMIs, http://www.sandpile.org/x86/smm.htm says that the CPL is 
stored somewhere in SMRAM.  I think your patches are an improvement 
anyway, we can build a more complete fix on top of them.

Paolo

> If you are right, I think the whole series needs to be reworked.

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

[Paolo Bonzini]

Il 13/05/2014 20:57, Paolo Bonzini ha scritto:
> Il 13/05/2014 20:39, Kevin O'Connor ha scritto:
>> That doesn't sound right.  What happens if the processor takes an NMI,
>> SMI, or VMEXIT between the point it enables protected mode but before
>> it long jumps?  The processor would have to save and restore the CPL
>> somewhere for all of these situations.
>
> For VMEXITs it's up to the hypervisor to make it work properly.  I just
> posted today fixes for KVM.
>
> I guess the answer for NMIs is "good luck".  But in the case of NMIs,
> wouldn't it be broken anyway, because the IDT format is different
> between real mode and protected mode?
>
> For SMIs, http://www.sandpile.org/x86/smm.htm says that the CPL is
> stored somewhere in SMRAM.  I think your patches are an improvement
> anyway, we can build a more complete fix on top of them.

On second thought, the CPL should always be equal to SS.DPL, even during 
real mode transitions.  Unlike CS.RPL, SS.DPL is hidden in the internal 
segment descriptor registers and is always zero.

I say *should*, because of course there is an exception. :)  CPL is 
forced to 3 by SYSRET, even if it loads SS with a selector whose RPL is 
not 3.  So we're better off saving CPL in SMRAM (and perhaps make the 
SMRAM map equal to that of real processors).

In any case, I think this is an example of how your patches are an 
improvement; it would be trivial to make CPL=SS.DPL on top of them.

Paolo

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

[Kevin O'Connor]

On Tue, May 13, 2014 at 02:39:20PM -0400, Kevin O'Connor wrote:
> On Tue, May 13, 2014 at 08:24:47PM +0200, Paolo Bonzini wrote:
> > Il 27/04/2014 19:25, Kevin O'Connor ha scritto:
> > > I was wondering about that as well.  The Intel docs state that the CPL
> > > is bits 0-1 of the CS.selector register, and that protected mode
> > > starts immediately after setting the PE bit.  The CS.selector field
> > > should be the value of %cs in real mode, which is the value added to
> > > eip (after shifting right by 4).
> > > 
> > > I guess that means that the real mode code that enables the PE bit
> > > must run with a code segment aligned to a value of 4.  (Which
> > > effectively means code alignment of 64 bytes because of the segment
> > > shift.)
> > 
> > It turns out that this is not a requirement; which means that the 
> > protected mode transition is exactly the only place where CPL is not 
> > redundant.  The CPL remains zero until you reload CS with a long jump.
> 
> That doesn't sound right.

FYI, I ran a couple of tests on a real machine where I set protected
mode while %cs=0xf003 and I can confirm that it doesn't cause faults.
So, you are correct - the CPL appears to be stored separately from
%cs[1:0] and it appears CPL isn't altered until CS is reloaded.

-Kevin

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

[Paolo Bonzini]

Il 14/05/2014 00:07, Kevin O'Connor ha scritto:
> On Tue, May 13, 2014 at 02:39:20PM -0400, Kevin O'Connor wrote:
>> On Tue, May 13, 2014 at 08:24:47PM +0200, Paolo Bonzini wrote:
>>> Il 27/04/2014 19:25, Kevin O'Connor ha scritto:
>>>> I was wondering about that as well.  The Intel docs state that the CPL
>>>> is bits 0-1 of the CS.selector register, and that protected mode
>>>> starts immediately after setting the PE bit.  The CS.selector field
>>>> should be the value of %cs in real mode, which is the value added to
>>>> eip (after shifting right by 4).
>>>>
>>>> I guess that means that the real mode code that enables the PE bit
>>>> must run with a code segment aligned to a value of 4.  (Which
>>>> effectively means code alignment of 64 bytes because of the segment
>>>> shift.)
>>>
>>> It turns out that this is not a requirement; which means that the 
>>> protected mode transition is exactly the only place where CPL is not 
>>> redundant.  The CPL remains zero until you reload CS with a long jump.
>>
>> That doesn't sound right.
> 
> FYI, I ran a couple of tests on a real machine where I set protected
> mode while %cs=0xf003 and I can confirm that it doesn't cause faults.
> So, you are correct - the CPL appears to be stored separately from
> %cs[1:0] and it appears CPL isn't altered until CS is reloaded.

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?

------------- 8< -------------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] target-i386: get CPL from SS.DPL

CS.RPL is not equal to the CPL in the few instructions between
setting CR0.PE and reloading CS.  We get this right in the common
case, because writes to CR0 do not modify the CPL, but it would
not be enough if an SMI comes exactly during that brief period.
Were this to happen, the RSM instruction would erroneously set
CPL to the low two bits of the real-mode selector; and if they are
not 00, the next instruction fetch cannot access the code segment
and causes a triple fault.

However, SS.DPL *is* always equal to the CPL.  In real processors
(AMD only) there is a weird case of SYSRET setting SS.DPL=SS.RPL
from the STAR register while forcing CPL=3, but we do not emulate
that.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e9cbdab..65a44d9 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -986,7 +986,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 */
@@ -996,15 +995,14 @@ 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 HF_CPL_MASK != 3
 #error HF_CPL_MASK is hardcoded
 #endif



Paolo

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

[Kevin O'Connor]

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.

-Kevin


==================== Freedos with unmodified QEMU 2.0.0 (and SeaBIOS master)


IN: 
0x000000000012276e:  mov    %cr0,%eax
0x0000000000122771:  or     $0x80000000,%eax
0x0000000000122777:  mov    %eax,%cr0

EAX=00125000 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=000001dc
EIP=000006fe EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
CS =000c 00122070 00002713 00009a00 DPL=0 CS16 [-R-]
SS =0038 00003820 00000200 00009200 DPL=0 DS16 [-W-]
DS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
FS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
GS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=00000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=00126360 CCO=EFLAGS  
EFER=0000000000000000
----------------
IN: 
0x000000000012277a:  iretl  

EAX=80000011 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=000001dc
EIP=0000070a EFL=00000086 [--S--P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
CS =000c 00122070 00002713 00009a00 DPL=0 CS16 [-R-]
SS =0038 00003820 00000200 00009200 DPL=0 DS16 [-W-]
DS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
FS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
GS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=80000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=80000011 CCO=LOGICL  
EFER=0000000000000000
Servicing hardware INT=0x08
     0: v=08 e=0000 i=0 cpl=3 IP=0930:00000000000001c9 pc=00000000000094c9 SP=0920:0000000000000100 env->regs[R_EAX]=0000000080000011
EAX=80000011 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000100
EIP=000001c9 EFL=00023202 [-------] CPL=3 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00000000
CS =0930 00009300 0000ffff 00000000
SS =0920 00009200 0000ffff 00000000
DS =03a6 00003a60 0000ffff 00000000
FS =0000 00000000 0000ffff 00000000
GS =0000 00000000 0000ffff 00000000
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=80000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=80000011 CCO=EFLAGS  
EFER=0000000000000000
----------------
IN: 
0x000000000012279c:  call   0x122070


==================== Freedos with patched QEMU (and SeaBIOS master)


IN: 
0x000000000012276e:  mov    %cr0,%eax
0x0000000000122771:  or     $0x80000000,%eax
0x0000000000122777:  mov    %eax,%cr0

EAX=00125000 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=000001dc
EIP=000006fe EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
CS =000c 00122070 00002713 00009a00 DPL=0 CS16 [-R-]
SS =0038 00003820 00000200 00009200 DPL=0 DS16 [-W-]
DS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
FS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
GS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=00000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=00126360 CCO=EFLAGS  
EFER=0000000000000000
----------------
IN: 
0x000000000012277a:  iretl  

EAX=80000011 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=000001dc
EIP=0000070a EFL=00000086 [--S--P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
CS =000c 00122070 00002713 00009a00 DPL=0 CS16 [-R-]
SS =0038 00003820 00000200 00009200 DPL=0 DS16 [-W-]
DS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
FS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
GS =0014 00003a60 00000b8c 00009300 DPL=0 DS16 [-WA]
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=80000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=80000011 CCO=LOGICL  
EFER=0000000000000000
Servicing hardware INT=0x08
     0: v=08 e=0000 i=0 cpl=0 IP=0930:00000000000001c9 pc=00000000000094c9 SP=0920:0000000000000100 env->regs[R_EAX]=0000000080000011
EAX=80000011 EBX=00000000 ECX=00000000 EDX=00000000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000100
EIP=000001c9 EFL=00023202 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00000000
CS =0930 00009300 0000ffff 00000000
SS =0920 00009200 0000ffff 00000000
DS =03a6 00003a60 0000ffff 00000000
FS =0000 00000000 0000ffff 00000000
GS =0000 00000000 0000ffff 00000000
LDT=0008 00003d64 00000020 00008200 DPL=0 LDT
TR =0010 00110000 00002069 00008900 DPL=0 TSS32-avl
GDT=     00003ce4 0000007f
IDT=     00124784 000007ff
CR0=80000011 CR2=00000000 CR3=00125000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff4ff0 DR7=0000000000000400
CCS=00000000 CCD=80000011 CCO=EFLAGS  
EFER=0000000000000000
check_exception old: 0xffffffff new 0xd
     1: v=0d e=000c i=0 cpl=0 IP=0930:00000000000001c9 pc=00000000000094c9 SP=0920:0000000000000100 env->regs[R_EAX]=0000000080000011

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

[Kevin O'Connor]

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

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

[Paolo Bonzini]

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