Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present

[Paolo Bonzini <pbonzini@redhat.com>]

On 30/05/2017 19:35, Roman Penyaev wrote:
> On Tue, May 30, 2017 at 4:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 19/05/2017 18:14, Roman Penyaev wrote:
>>> 2. A bit complicated, which makes sure the CPL field is preserved across
>>>    KVM_GET/SET_SREGS calls and makes svm_set_segment() and svm_get_segment()
>>>    functionality symmethric:
>>
>> I think I prefer this solution.
>>
>>>    KVM SVM side:
>>>    -------------
>>>
>>>    --- a/arch/x86/kvm/svm.c
>>>    +++ b/arch/x86/kvm/svm.c
>>>    @@ -1999,7 +1999,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>>>             * would entail passing the CPL to userspace and back.
>>>             */
>>>            if (seg == VCPU_SREG_SS)
>>>    -               svm->vmcb->save.cpl = (s->attrib >>
>>> SVM_SELECTOR_DPL_SHIFT) & 3;
>>>    +               svm->vmcb->save.cpl = (var->dpl & 3);
>>>
>>>            mark_dirty(svm->vmcb, VMCB_SEG);
>>>    }
>>
>> I wonder why svm_set_segment is setting s->attrib = 0 at all.  The
>> manual only mentions checking P=0.  What about something like:
>>
>>         s->base = var->base;
>>         s->limit = var->limit;
>>         s->selector = var->selector;
>>         s->attrib = (var->type & SVM_SELECTOR_TYPE_MASK);
>>         s->attrib |= (var->s & 1) << SVM_SELECTOR_S_SHIFT;
>>         s->attrib |= (var->dpl & 3) << SVM_SELECTOR_DPL_SHIFT;
>>         s->attrib |= (var->present && !var->unusable) << SVM_SELECTOR_P_SHIFT;
>>         s->attrib |= (var->avl & 1) << SVM_SELECTOR_AVL_SHIFT;
>>         s->attrib |= (var->l & 1) << SVM_SELECTOR_L_SHIFT;
>>         s->attrib |= (var->db & 1) << SVM_SELECTOR_DB_SHIFT;
>>         s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
> 
> Do we care about compatibility issues?  I mean can any old qemu send
> us "garbage" in other members of 'var' structure if 'var->unused' == 1 ?

That shouldn't matter, the processor shouldn't use them if P=0.

> Oh, it seems we require one more field in 'struct kvm_segment' for CPL.

Why?  The point is exactly to use SS's var->dpl.

>>>    QEMU side:
>>>    ----------
>>>
>>>    --- a/target/i386/kvm.c
>>>    +++ b/target/i386/kvm.c
>>>    @@ -1979,6 +1979,8 @@ static int kvm_get_sregs(X86CPU *cpu)
>>>         get_seg(&env->segs[R_FS], &sregs.fs);
>>>         get_seg(&env->segs[R_GS], &sregs.gs);
>>>         get_seg(&env->segs[R_SS], &sregs.ss);
>>>    +    if (sregs.ss.unusable)
>>>    +        env->segs[R_SS].flags |= sregs.ss.dpl << DESC_DPL_SHIFT;
>>>
>>>         get_seg(&env->tr, &sregs.tr);
>>>         get_seg(&env->ldt, &sregs.ldt);
>>
>> I think what QEMU should do is, in get_seg
>>
>>         if (rhs->unusable) {
>>             lhs->flags &= ~DESC_P_MASK;
>>
>> This would preserve the SS.DPL field.  This should still work fine with
>> QEMU commit 4cae9c9 (the loading side would set lhs->unusable).
> 
> Indeed, it will preserve the *old* SS.DPL field, but will not take the *new*
> one from kvm side.  And what if extend get_seg() with additional 'segtype'
> argument:

Or:

    lhs->flags = (rhs->type << DESC_TYPE_SHIFT) |
                 (rhs->present * DESC_P_MASK) |
                 (rhs->dpl << DESC_DPL_SHIFT) |
                 (rhs->db << DESC_B_SHIFT) |
                 (rhs->s * DESC_S_MASK) |
                 (rhs->l << DESC_L_SHIFT) |
                 (rhs->g * DESC_G_MASK) |
                 (rhs->avl * DESC_AVL_MASK);
    if (rhs->unusable) {
        lhs->flags = 0;
    }

which could also be simply

   lhs->flags = ... |
                ((rhs->present && !rhs->unusable) * DESC_P_MASK) | ...;

as in the KVM code.
`
> Then we always keep convention and keep dpl alive along the way U->K->U to
> restore it as cpl.

Yes, exactly.

Paolo