From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751380AbdE3Rfk (ORCPT ); Tue, 30 May 2017 13:35:40 -0400 Received: from mail-io0-f174.google.com ([209.85.223.174]:36153 "EHLO mail-io0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751037AbdE3Rfi (ORCPT ); Tue, 30 May 2017 13:35:38 -0400 MIME-Version: 1.0 In-Reply-To: <0155f13f-f65b-27a6-e646-44fffccbbf40@redhat.com> References: <0155f13f-f65b-27a6-e646-44fffccbbf40@redhat.com> From: Roman Penyaev Date: Tue, 30 May 2017 19:35:16 +0200 Message-ID: Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present To: Paolo Bonzini Cc: Mikhail Sennikovskii , Gleb Natapov , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 30, 2017 at 4:47 PM, Paolo Bonzini 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