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

[Paolo Bonzini]

Il 26/04/2014 21:36, Kevin O'Connor ha scritto:
> Yes, I was thinking of something like that as well.  If QEMU
> internally observes the formula above, then something like the patch
> below should work instead of my original patch.
>
> However, I'm not an expert on QEMU TCG and the patch below would
> require much more testing.

Yeah, the patch is obviously more complex.  On the other hand as you 
point out the code to set hflags was already relying on correct eflags 
as a precondition.

> (As a side note, the patch below also
> obviates the need to save/restore cpl in the svm code, but I left
> saving of cpl in the "hsave page" in case that is part of some
> standard.)

Yes, the hsave format is part of the AMD virtualization extensions, and 
KVM for one relies on the CPL field.

A few comments.

> +#if HF_CPL_MASK != 3
> +#error HF_CPL_MASK is hardcoded
> +#endif
> +            int cpl = selector & 3;
>  #ifdef TARGET_X86_64
>              if ((env->hflags & HF_LMA_MASK) && (flags & DESC_L_MASK)) {
>                  /* long mode */
> -                env->hflags |= HF_CS32_MASK | HF_SS32_MASK | HF_CS64_MASK;
> -                env->hflags &= ~(HF_ADDSEG_MASK);
> +                env->hflags &= ~(HF_ADDSEG_MASK | HF_CPL_MASK);
> +                env->hflags |= HF_CS32_MASK | HF_SS32_MASK | HF_CS64_MASK | 
> cpl;
>              } else
>  #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;
> +                env->hflags &= ~(HF_CS32_MASK | HF_CS64_MASK | HF_CPL_MASK);
> +                env->hflags |= new_hflags | cpl;

Shouldn't the "env->hflags = (env->hflags & ~HF_CPL_MASK) | cpl" be out 
of the "if"?  (The "#if" could be placed there too).

> @@ -703,7 +700,8 @@ void helper_vmexit(CPUX86State *env, uint32_t exit_code, 
> uint64_t exit_info_1)
>      cpu_load_eflags(env, ldq_phys(cs->as,
>                                    env->vm_hsave + offsetof(struct vmcb,
>                                                             save.rflags)),
> -                    ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | DF_MASK));
> +                    ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | DF_MASK |
> +                      VM_MASK));
>      CC_OP = CC_OP_EFLAGS;

This CC_OP assignment should be moved to cpu_load_eflags (unrelated bug).

Also, VM_MASK is already cleared below:

    /* Forces CR0.PE = 1, RFLAGS.VM = 0. */
    env->cr[0] |= CR0_PE_MASK;
    env->eflags &= ~VM_MASK;

You need to move both of them above, not just env->eflags.  For CR0.PE, 
CR0_PE_MASK should be ORed to cpu_x86_update_cr0's argument.

Can you send v2 of the patch with the movements split across multiple 
patches (followed by a last patch to kill cpu_x86_set_cpl)?

Paolo

>      svm_load_seg_cache(env, env->vm_hsave + offsetof(struct vmcb, save.es),

> @@ -728,7 +726,6 @@ void helper_vmexit(CPUX86State *env, uint32_t exit_code, 
> uint64_t exit_info_1)
>                            env->vm_hsave + offsetof(struct vmcb, save.dr7));
>
>      /* other setups */
> -    cpu_x86_set_cpl(env, 0);
>      stq_phys(cs->as, env->vm_vmcb + offsetof(struct vmcb, control.exit_code),
>               exit_code);
>      stq_phys(cs->as, env->vm_vmcb + offsetof(struct vmcb, control.exit_info_1),