On 30/05/2017 19:35, Roman Penyaev wrote:
> 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 ?
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
