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

[Roman Penyaev]

On Tue, May 30, 2017 at 4:47 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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 ?

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

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

static void get_seg(SegmentCache *lhs, const struct kvm_segment *rhs,
int segtype)
{
...
         if (rhs->unusable) {
             /* Clear P and DPL bits */
             lhs->flags &= ~(DESC_P_MASK | (3 << DESC_DPL_SHIFT));
             if (segtype == R_SS)
                /* Set DPL */
                lhs->flags |= rhs->dpl << DESC_DPL_SHIFT;
         }

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

--
Roman