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
