[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0155f13f-f65b-27a6-e646-44fffccbbf40@redhat.com>
Date: Tue, 30 May 2017 16:47:04 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Roman Penyaev <roman.penyaev@...fitbricks.com>,
Mikhail Sennikovskii <mikhail.sennikovskii@...fitbricks.com>,
Gleb Natapov <gleb@...nel.org>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present
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.
Powered by blists - more mailing lists