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

[Bandan Das]

Paolo Bonzini <address@hidden> writes:

> 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.

Are you referring to 26.3.1.2 ? Just a nit, I guess it helps 
to add a pointer in the comments for someone (like me :)) going
through the code.

> 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 <address@hidden>
> ---
>  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)