Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653290 > unrolled thread

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

Started byPaolo Bonzini <pbonzini@redhat.com>
First post2017-05-30 16:50 +0200
Last post2017-05-31 13:00 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Paolo Bonzini <pbonzini@redhat.com> - 2017-05-30 16:50 +0200
    Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Roman Penyaev <roman.penyaev@profitbricks.com> - 2017-05-30 19:40 +0200
      Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Paolo Bonzini <pbonzini@redhat.com> - 2017-05-30 23:10 +0200
        Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Roman Penyaev <roman.penyaev@profitbricks.com> - 2017-05-31 12:20 +0200
          Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present Paolo Bonzini <pbonzini@redhat.com> - 2017-05-31 13:00 +0200

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

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-05-30 16:50 +0200
SubjectRe: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present
Message-ID<tMQD7-ar-9@gated-at.bofh.it>

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;

>    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;
	} else {
	    ...
	}

This would preserve the SS.DPL field.  This should still work fine with
QEMU commit 4cae9c9 (the loading side would set lhs->unusable).

Thanks,

Paolo

> 
> Current email is an RFC since for us is not fully clear is it really
> needed to preserve DPL across KVM_SET/GET_SREGS calls when segment
> is unusable.  E.g. there was a commit:
> 
> 4cae9c97967a ("target-i386: kvm: clear unusable segments' flags in migration")
> 
> which in purpose drops all segment flags to zero on QEMU side in order
> to fix guests migration.

[toc] | [next] | [standalone]

#1653429

FromRoman Penyaev <roman.penyaev@profitbricks.com>
Date2017-05-30 19:40 +0200
Message-ID<tMThD-1Qx-3@gated-at.bofh.it>
In reply to#1653290
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 ?

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

[toc] | [prev] | [next] | [standalone]

#1653558

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-05-30 23:10 +0200
Message-ID<tMWyT-3ZT-37@gated-at.bofh.it>
In reply to#1653429

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

[toc] | [prev] | [next] | [standalone]

#1654093

FromRoman Penyaev <roman.penyaev@profitbricks.com>
Date2017-05-31 12:20 +0200
Message-ID<tN8To-3pQ-17@gated-at.bofh.it>
In reply to#1653558
On Tue, May 30, 2017 at 11:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> 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.

Could you please point me where did you find that?  E.g. what I see in
AMD manual 24593—Rev. 3.28—March 2017, section "Segment State in the VMCB",
top of the page 453:

  NOTE: For the Stack Segment attributes, P is observed in legacy and
        compatibility mode. In 64-bit mode, P is ignored because all
        stack segments are treated as present.

So I am confused.

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

Yes, yes, let's use dpl as it is used now on svm_get_segment().

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

True.  Fully symmetric.  So something like that:

Kernel:
-------
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d09bc3e7882c..ecb76d9bf0cb 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1466,6 +1466,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
                 */
                if (var->unusable)
                        var->db = 0;
+               /* This is symmetric with svm_set_segment() */
                var->dpl = to_svm(vcpu)->vmcb->save.cpl;
                break;
        }
@@ -1610,18 +1611,14 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
        s->base = var->base;
        s->limit = var->limit;
        s->selector = var->selector;
-       if (var->unusable)
-               s->attrib = 0;
-       else {
-               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 & 1) << 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;
-       }
+       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 & 1) && !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;

        /*
         * This is always accurate, except if SYSRET returned to a segment
@@ -1630,7 +1627,8 @@ 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;
+               /* This is symmetric with svm_get_segment() */
+               svm->vmcb->save.cpl = (var->dpl & 3);

        mark_dirty(svm->vmcb, VMCB_SEG);
 }


QEMU:
-----
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 011d4a55b136..faee904d9d59 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1300,18 +1300,14 @@ static void get_seg(SegmentCache *lhs, const
struct kvm_segment *rhs)
     lhs->selector = rhs->selector;
     lhs->base = rhs->base;
     lhs->limit = rhs->limit;
-    if (rhs->unusable) {
-        lhs->flags = 0;
-    } else {
-        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);
-    }
+    lhs->flags = (rhs->type << DESC_TYPE_SHIFT) |
+                 ((rhs->present && !rhs->unusable) * 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);
 }

--
Roman

[toc] | [prev] | [next] | [standalone]

#1654125

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-05-31 13:00 +0200
Message-ID<tN9w6-3Ek-11@gated-at.bofh.it>
In reply to#1654093

----- Original Message -----
> From: "Roman Penyaev" <roman.penyaev@profitbricks.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Mikhail Sennikovskii" <mikhail.sennikovskii@profitbricks.com>, "Gleb Natapov" <gleb@kernel.org>,
> kvm@vger.kernel.org, linux-kernel@vger.kernel.org
> Sent: Wednesday, May 31, 2017 12:17:01 PM
> Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present
> 
> On Tue, May 30, 2017 at 11:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> > 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.
> 
> Could you please point me where did you find that?  E.g. what I see in
> AMD manual 24593—Rev. 3.28—March 2017, section "Segment State in the VMCB",
> top of the page 453:
> 
>   NOTE: For the Stack Segment attributes, P is observed in legacy and
>         compatibility mode. In 64-bit mode, P is ignored because all
>         stack segments are treated as present.

You're right and in fact the same applies to unusable=1 on Intel.  But
on the other hand, if the garbage got there somehow (e.g. via SMM) it's
the right thing to use it.

> True.  Fully symmetric.  So something like that:
> 
> Kernel:
> -------
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d09bc3e7882c..ecb76d9bf0cb 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1466,6 +1466,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
>                  */
>                 if (var->unusable)
>                         var->db = 0;
> +               /* This is symmetric with svm_set_segment() */
>                 var->dpl = to_svm(vcpu)->vmcb->save.cpl;
>                 break;
>         }
> @@ -1610,18 +1611,14 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>         s->base = var->base;
>         s->limit = var->limit;
>         s->selector = var->selector;
> -       if (var->unusable)
> -               s->attrib = 0;
> -       else {
> -               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 & 1) << 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;
> -       }
> +       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 & 1) && !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;
> 
>         /*
>          * This is always accurate, except if SYSRET returned to a segment
> @@ -1630,7 +1627,8 @@ 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;
> +               /* This is symmetric with svm_get_segment() */
> +               svm->vmcb->save.cpl = (var->dpl & 3);
> 
>         mark_dirty(svm->vmcb, VMCB_SEG);
>  }
> 
> 
> QEMU:
> -----
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 011d4a55b136..faee904d9d59 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1300,18 +1300,14 @@ static void get_seg(SegmentCache *lhs, const
> struct kvm_segment *rhs)
>      lhs->selector = rhs->selector;
>      lhs->base = rhs->base;
>      lhs->limit = rhs->limit;
> -    if (rhs->unusable) {
> -        lhs->flags = 0;
> -    } else {
> -        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);
> -    }
> +    lhs->flags = (rhs->type << DESC_TYPE_SHIFT) |
> +                 ((rhs->present && !rhs->unusable) * 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);
>  }


Yes, I think both are the right thing to do.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web