[Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Paolo Bonzini]

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 (except during task switches
as noted in the previous patch).  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>
---
 target-i386/cpu.h     | 8 +++-----
 target-i386/kvm.c     | 2 +-
 target-i386/machine.c | 8 ++++++++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 33d80b4..ae753d0 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
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d894ef..3931d4c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1430,7 +1430,7 @@ static int kvm_get_sregs(X86CPU *cpu)
        HF_OSFXSR_MASK | HF_LMA_MASK | HF_CS32_MASK | \
        HF_SS32_MASK | HF_CS64_MASK | HF_ADDSEG_MASK)
 
-    hflags = (env->segs[R_CS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
+    hflags = (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
     hflags |= (env->cr[0] & CR0_PE_MASK) << (HF_PE_SHIFT - CR0_PE_SHIFT);
     hflags |= (env->cr[0] << (HF_MP_SHIFT - CR0_MP_SHIFT)) &
                 (HF_MP_MASK | HF_EM_MASK | HF_TS_MASK);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 168cab6..bdff447 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -312,6 +312,14 @@ static int cpu_post_load(void *opaque, int version_id)
         env->segs[R_SS].flags &= ~(env->segs[R_SS].flags & DESC_DPL_MASK);
     }
 
+    /* Older versions of QEMU incorrectly used CS.DPL as the CPL when
+     * running under KVM.  This is wrong for conforming code segments.
+     * Luckily, in our implementation the CPL field of hflags is redundant
+     * and we can get the right value from the SS descriptor privilege level.
+     */
+    env->hflags &= ~HF_CPL_MASK;
+    env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
+
     /* XXX: restore FPU round state */
     env->fpstt = (env->fpus_vmstate >> 11) & 7;
     env->fpus = env->fpus_vmstate & ~0x3800;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Kevin O'Connor]

On Thu, May 15, 2014 at 06:56:56PM +0200, Paolo Bonzini wrote:
> 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 (except during task switches
> as noted in the previous patch).  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.

It looks to me like there could be a couple of places in the code
where cpu_x86_load_seg_cache(R_CS) is called, but
cpu_x86_load_seg_cache(R_SS) may not be.  In particular,
helper_ret_protected() and cpu_x86_load_seg_cache_sipi().  Are these
still okay?

-Kevin

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Paolo Bonzini]

Il 15/05/2014 20:38, Kevin O'Connor ha scritto:
> On Thu, May 15, 2014 at 06:56:56PM +0200, Paolo Bonzini wrote:
>> 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 (except during task switches
>> as noted in the previous patch).  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.
>
> It looks to me like there could be a couple of places in the code
> where cpu_x86_load_seg_cache(R_CS) is called, but
> cpu_x86_load_seg_cache(R_SS) may not be.  In particular,
> helper_ret_protected() and cpu_x86_load_seg_cache_sipi().  Are these
> still okay?

Yes, helper_ret_protected() skips the SS load only if rpl == cpl (so if 
CS.RPL == SS.DPL, and the invariant is respected). 
cpu_x86_load_seg_cache_sipi() runs in real mode only.

Paolo

[Qemu-devel] [PATCH v2 0/3] target-i386: fix CPL computation

[Paolo Bonzini]

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.

So this patch fixes the CPL to come from SS.DPL instead, which
requires a couple changes to make access rights correct for VM86
mode, and to override the CPL to CS.RPL during task switches (more
details in patch 2).  The Intel documentation mentions in a couple
places that SS.DPL is indeed the current privilege level.

Tested with FreeDOS (checking with info registers that it runs
in real vs. VM86 mode depending on the chosen boot option),
kvm-unit-tests and runcom.c.

v1->v2: fix user-mode emulation

Paolo Bonzini (3):
  target-i386: fix segment flags for SMM, user-mode emulation and VM86 mode
  target-i386: rework CPL checks during task switch, preparing for next patch
  target-i386: get CPL from SS.DPL

 bsd-user/main.c          |  2 +-
 linux-user/main.c        |  2 +-
 target-i386/cpu.h        |  8 +++-----
 target-i386/gdbstub.c    |  4 +++-
 target-i386/kvm.c        |  2 +-
 target-i386/machine.c    |  8 ++++++++
 target-i386/seg_helper.c | 34 +++++++++++++++++-----------------
 target-i386/smm_helper.c | 24 ++++++++++++++++++------
 8 files changed, 52 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] target-i386: fix segment flags for SMM, user-mode emulation and VM86 mode

[Paolo Bonzini]

With the next patch, these need to be correct or VM86 tasks
have the wrong CPL.  The flags are basically what the Intel VMX
documentation say is mandatory for entry into a VM86 guest.

The CPL also needs to be set before the user-mode segments
in linux-user and bsd-user.

For consistency, SMM ought to have the same flags except with
CPL=0.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 bsd-user/main.c          |  2 +-
 linux-user/main.c        |  2 +-
 target-i386/gdbstub.c    |  4 +++-
 target-i386/seg_helper.c | 11 ++++++++---
 target-i386/smm_helper.c | 24 ++++++++++++++++++------
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 4ba61da..0e8c26c 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -1004,7 +1004,7 @@ int main(int argc, char **argv)
 
 #if defined(TARGET_I386)
     env->cr[0] = CR0_PG_MASK | CR0_WP_MASK | CR0_PE_MASK;
-    env->hflags |= HF_PE_MASK;
+    env->hflags |= HF_PE_MASK | HF_CPL_MASK;
     if (env->features[FEAT_1_EDX] & CPUID_SSE) {
         env->cr[4] |= CR4_OSFXSR_MASK;
         env->hflags |= HF_OSFXSR_MASK;
diff --git a/linux-user/main.c b/linux-user/main.c
index 882186e..3e21024 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4052,7 +4052,7 @@ int main(int argc, char **argv, char **envp)
 
 #if defined(TARGET_I386)
     env->cr[0] = CR0_PG_MASK | CR0_WP_MASK | CR0_PE_MASK;
-    env->hflags |= HF_PE_MASK;
+    env->hflags |= HF_PE_MASK | HF_CPL_MASK;
     if (env->features[FEAT_1_EDX] & CPUID_SSE) {
         env->cr[4] |= CR4_OSFXSR_MASK;
         env->hflags |= HF_OSFXSR_MASK;
diff --git a/target-i386/gdbstub.c b/target-i386/gdbstub.c
index d34e535..19fe9ad 100644
--- a/target-i386/gdbstub.c
+++ b/target-i386/gdbstub.c
@@ -127,9 +127,11 @@ static int x86_cpu_gdb_load_seg(X86CPU *cpu, int sreg, uint8_t *mem_buf)
         target_ulong base;
 
         if (!(env->cr[0] & CR0_PE_MASK) || (env->eflags & VM_MASK)) {
+            int dpl = (env->eflags & VM_MASK) ? 3 : 0;
             base = selector << 4;
             limit = 0xffff;
-            flags = 0;
+            flags = DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                    DESC_A_MASK | (dpl << DESC_DPL_SHIFT);
         } else {
             if (!cpu_x86_get_descr_debug(env, selector, &base, &limit,
                                          &flags)) {
diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index 6c0142a..dd2c8b4 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -88,8 +88,10 @@ static inline void load_seg_cache_raw_dt(SegmentCache *sc, uint32_t e1,
 static inline void load_seg_vm(CPUX86State *env, int seg, int selector)
 {
     selector &= 0xffff;
-    cpu_x86_load_seg_cache(env, seg, selector,
-                           (selector << 4), 0xffff, 0);
+
+    cpu_x86_load_seg_cache(env, seg, selector, (selector << 4), 0xffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK | (3 << DESC_DPL_SHIFT));
 }
 
 static inline void get_ss_esp_from_tss(CPUX86State *env, uint32_t *ss_ptr,
@@ -2462,9 +2464,12 @@ void helper_verw(CPUX86State *env, target_ulong selector1)
 void cpu_x86_load_seg(CPUX86State *env, int seg_reg, int selector)
 {
     if (!(env->cr[0] & CR0_PE_MASK) || (env->eflags & VM_MASK)) {
+        int dpl = (env->eflags & VM_MASK) ? 3 : 0;
         selector &= 0xffff;
         cpu_x86_load_seg_cache(env, seg_reg, selector,
-                               (selector << 4), 0xffff, 0);
+                               (selector << 4), 0xffff,
+                               DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                               DESC_A_MASK | (dpl << DESC_DPL_SHIFT));
     } else {
         helper_load_seg(env, seg_reg, selector);
     }
diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c
index 2f99493..1e5f5ce 100644
--- a/target-i386/smm_helper.c
+++ b/target-i386/smm_helper.c
@@ -170,12 +170,24 @@ void do_smm_enter(X86CPU *cpu)
     env->dr[7] = 0x00000400;
 
     cpu_x86_load_seg_cache(env, R_CS, (env->smbase >> 4) & 0xffff, env->smbase,
-                           0xffffffff, 0);
-    cpu_x86_load_seg_cache(env, R_DS, 0, 0, 0xffffffff, 0);
-    cpu_x86_load_seg_cache(env, R_ES, 0, 0, 0xffffffff, 0);
-    cpu_x86_load_seg_cache(env, R_SS, 0, 0, 0xffffffff, 0);
-    cpu_x86_load_seg_cache(env, R_FS, 0, 0, 0xffffffff, 0);
-    cpu_x86_load_seg_cache(env, R_GS, 0, 0, 0xffffffff, 0);
+                           0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
+    cpu_x86_load_seg_cache(env, R_DS, 0, 0, 0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
+    cpu_x86_load_seg_cache(env, R_ES, 0, 0, 0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
+    cpu_x86_load_seg_cache(env, R_SS, 0, 0, 0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
+    cpu_x86_load_seg_cache(env, R_FS, 0, 0, 0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
+    cpu_x86_load_seg_cache(env, R_GS, 0, 0, 0xffffffff,
+                           DESC_P_MASK | DESC_S_MASK | DESC_W_MASK |
+                           DESC_A_MASK);
 }
 
 void helper_rsm(CPUX86State *env)
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] target-i386: rework CPL checks during task switch, preparing for next patch

[Paolo Bonzini]

During task switch, all of CS.DPL, CS.RPL, SS.DPL must match (in addition
to all the other requirements) and will be the new CPL.  So far this worked
by carefully setting the CS selector and flags before doing the task
switch; but this will not work once we get the CPL from SS.DPL.

Temporarily assume that the CPL comes from CS.RPL during task switch
to a protected-mode task, until the descriptor of SS is loaded.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/seg_helper.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index dd2c8b4..426b9c6 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -135,11 +135,10 @@ static inline void get_ss_esp_from_tss(CPUX86State *env, uint32_t *ss_ptr,
     }
 }
 
-/* XXX: merge with load_seg() */
-static void tss_load_seg(CPUX86State *env, int seg_reg, int selector)
+static void tss_load_seg(CPUX86State *env, int seg_reg, int selector, int cpl)
 {
     uint32_t e1, e2;
-    int rpl, dpl, cpl;
+    int rpl, dpl;
 
     if ((selector & 0xfffc) != 0) {
         if (load_segment(env, &e1, &e2, selector) != 0) {
@@ -150,18 +149,13 @@ static void tss_load_seg(CPUX86State *env, int seg_reg, int selector)
         }
         rpl = selector & 3;
         dpl = (e2 >> DESC_DPL_SHIFT) & 3;
-        cpl = env->hflags & HF_CPL_MASK;
         if (seg_reg == R_CS) {
             if (!(e2 & DESC_CS_MASK)) {
                 raise_exception_err(env, EXCP0A_TSS, selector & 0xfffc);
             }
-            /* XXX: is it correct? */
             if (dpl != rpl) {
                 raise_exception_err(env, EXCP0A_TSS, selector & 0xfffc);
             }
-            if ((e2 & DESC_C_MASK) && dpl > rpl) {
-                raise_exception_err(env, EXCP0A_TSS, selector & 0xfffc);
-            }
         } else if (seg_reg == R_SS) {
             /* SS must be writable data */
             if ((e2 & DESC_CS_MASK) || !(e2 & DESC_W_MASK)) {
@@ -448,12 +442,13 @@ static void switch_tss(CPUX86State *env, int tss_selector,
 
     /* load the segments */
     if (!(new_eflags & VM_MASK)) {
-        tss_load_seg(env, R_CS, new_segs[R_CS]);
-        tss_load_seg(env, R_SS, new_segs[R_SS]);
-        tss_load_seg(env, R_ES, new_segs[R_ES]);
-        tss_load_seg(env, R_DS, new_segs[R_DS]);
-        tss_load_seg(env, R_FS, new_segs[R_FS]);
-        tss_load_seg(env, R_GS, new_segs[R_GS]);
+        int cpl = new_segs[R_CS] & 3;
+        tss_load_seg(env, R_CS, new_segs[R_CS], cpl);
+        tss_load_seg(env, R_SS, new_segs[R_SS], cpl);
+        tss_load_seg(env, R_ES, new_segs[R_ES], cpl);
+        tss_load_seg(env, R_DS, new_segs[R_DS], cpl);
+        tss_load_seg(env, R_FS, new_segs[R_FS], cpl);
+        tss_load_seg(env, R_GS, new_segs[R_GS], cpl);
     }
 
     /* check that env->eip is in the CS segment limits */
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Paolo Bonzini]

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>
---
 target-i386/cpu.h     | 8 +++-----
 target-i386/kvm.c     | 2 +-
 target-i386/machine.c | 8 ++++++++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1c00f1d..ee410af 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
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d894ef..3931d4c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1430,7 +1430,7 @@ static int kvm_get_sregs(X86CPU *cpu)
        HF_OSFXSR_MASK | HF_LMA_MASK | HF_CS32_MASK | \
        HF_SS32_MASK | HF_CS64_MASK | HF_ADDSEG_MASK)
 
-    hflags = (env->segs[R_CS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
+    hflags = (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
     hflags |= (env->cr[0] & CR0_PE_MASK) << (HF_PE_SHIFT - CR0_PE_SHIFT);
     hflags |= (env->cr[0] << (HF_MP_SHIFT - CR0_MP_SHIFT)) &
                 (HF_MP_MASK | HF_EM_MASK | HF_TS_MASK);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 168cab6..bdff447 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -312,6 +312,14 @@ static int cpu_post_load(void *opaque, int version_id)
         env->segs[R_SS].flags &= ~(env->segs[R_SS].flags & DESC_DPL_MASK);
     }
 
+    /* Older versions of QEMU incorrectly used CS.DPL as the CPL when
+     * running under KVM.  This is wrong for conforming code segments.
+     * Luckily, in our implementation the CPL field of hflags is redundant
+     * and we can get the right value from the SS descriptor privilege level.
+     */
+    env->hflags &= ~HF_CPL_MASK;
+    env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
+
     /* XXX: restore FPU round state */
     env->fpstt = (env->fpus_vmstate >> 11) & 7;
     env->fpus = env->fpus_vmstate & ~0x3800;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Kevin O'Connor]

On Fri, May 16, 2014 at 09:59:25PM +0200, Paolo Bonzini wrote:
> 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.

I was in the process of testing something else, when I encountered a
problem with an old MSDOS 6.22 floppy I had.  I tracked it down to an
error in one of the commits I did in this series (I sent a fix in a
separate email for it).

Unfortunately, after I fixed the problem in my patch, your patch above
breaks it again.  I think it's another VM86 thing.

Steps to reproduce:

1 - grab the DOS 6.22 floppy from: http://bootdisk.com/bootdisk.htm

2 - boot it up and add emm386.exe to config.sys ("edit config.sys" and
add "DEVICE=EMM386.EXE" on the second line of the file).

3 - reboot with modified config.sys

-Kevin

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Paolo Bonzini]

Il 20/05/2014 23:54, Kevin O'Connor ha scritto:
> On Fri, May 16, 2014 at 09:59:25PM +0200, Paolo Bonzini wrote:
>> 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.
>
> I was in the process of testing something else, when I encountered a
> problem with an old MSDOS 6.22 floppy I had.  I tracked it down to an
> error in one of the commits I did in this series (I sent a fix in a
> separate email for it).
>
> Unfortunately, after I fixed the problem in my patch, your patch above
> breaks it again.  I think it's another VM86 thing.
>
> Steps to reproduce:
>
> 1 - grab the DOS 6.22 floppy from: http://bootdisk.com/bootdisk.htm
>
> 2 - boot it up and add emm386.exe to config.sys ("edit config.sys" and
> add "DEVICE=EMM386.EXE" on the second line of the file).
>
> 3 - reboot with modified config.sys

I cannot reproduce this.  I can see the breakage with current master, 
and I can see your patch fixing it.  It keeps working with these 
changes.  Please try branch cpl-queue at 
git://github.com/bonzini/qemu.git and see if it works for you too.

My QEMU command line is simply "-fda boot622.img".

Paolo

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Kevin O'Connor]

On Wed, May 21, 2014 at 01:13:21PM +0200, Paolo Bonzini wrote:
> Il 20/05/2014 23:54, Kevin O'Connor ha scritto:
> >On Fri, May 16, 2014 at 09:59:25PM +0200, Paolo Bonzini wrote:
> >>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.
> >
> >I was in the process of testing something else, when I encountered a
> >problem with an old MSDOS 6.22 floppy I had.  I tracked it down to an
> >error in one of the commits I did in this series (I sent a fix in a
> >separate email for it).
> >
> >Unfortunately, after I fixed the problem in my patch, your patch above
> >breaks it again.  I think it's another VM86 thing.
> >
> >Steps to reproduce:
> >
> >1 - grab the DOS 6.22 floppy from: http://bootdisk.com/bootdisk.htm
> >
> >2 - boot it up and add emm386.exe to config.sys ("edit config.sys" and
> >add "DEVICE=EMM386.EXE" on the second line of the file).
> >
> >3 - reboot with modified config.sys
> 
> I cannot reproduce this.  I can see the breakage with current master, and I
> can see your patch fixing it.  It keeps working with these changes.  Please
> try branch cpl-queue at git://github.com/bonzini/qemu.git and see if it
> works for you too.

Apologies - somehow your patch 1 got misapplied to my tree.  Testing
with the tree above works fine.

-Kevin

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Paolo Bonzini]

Il 21/05/2014 16:05, Kevin O'Connor ha scritto:
> On Wed, May 21, 2014 at 01:13:21PM +0200, Paolo Bonzini wrote:
>> Il 20/05/2014 23:54, Kevin O'Connor ha scritto:
>>> On Fri, May 16, 2014 at 09:59:25PM +0200, Paolo Bonzini wrote:
>>>> 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.
>>>
>>> I was in the process of testing something else, when I encountered a
>>> problem with an old MSDOS 6.22 floppy I had.  I tracked it down to an
>>> error in one of the commits I did in this series (I sent a fix in a
>>> separate email for it).
>>>
>>> Unfortunately, after I fixed the problem in my patch, your patch above
>>> breaks it again.  I think it's another VM86 thing.
>>>
>>> Steps to reproduce:
>>>
>>> 1 - grab the DOS 6.22 floppy from: http://bootdisk.com/bootdisk.htm
>>>
>>> 2 - boot it up and add emm386.exe to config.sys ("edit config.sys" and
>>> add "DEVICE=EMM386.EXE" on the second line of the file).
>>>
>>> 3 - reboot with modified config.sys
>>
>> I cannot reproduce this.  I can see the breakage with current master, and I
>> can see your patch fixing it.  It keeps working with these changes.  Please
>> try branch cpl-queue at git://github.com/bonzini/qemu.git and see if it
>> works for you too.
>
> Apologies - somehow your patch 1 got misapplied to my tree.  Testing
> with the tree above works fine.

Should I take this as a Tested-by? :)

Paolo

Re: [Qemu-devel] [PATCH 3/3] target-i386: get CPL from SS.DPL

[Kevin O'Connor]

On Wed, May 21, 2014 at 04:18:22PM +0200, Paolo Bonzini wrote:
> Il 21/05/2014 16:05, Kevin O'Connor ha scritto:
> >On Wed, May 21, 2014 at 01:13:21PM +0200, Paolo Bonzini wrote:
> >>I cannot reproduce this.  I can see the breakage with current master, and I
> >>can see your patch fixing it.  It keeps working with these changes.  Please
> >>try branch cpl-queue at git://github.com/bonzini/qemu.git and see if it
> >>works for you too.
> >
> >Apologies - somehow your patch 1 got misapplied to my tree.  Testing
> >with the tree above works fine.
> 
> Should I take this as a Tested-by? :)

Sure.  I boot tested several old images and haven't seen any
regressions.

Thanks.
-Kevin