From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751804AbdE3OrL (ORCPT ); Tue, 30 May 2017 10:47:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36796 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751127AbdE3OrJ (ORCPT ); Tue, 30 May 2017 10:47:09 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9715280C09 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=pbonzini@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9715280C09 Subject: Re: [RFC] KVM: SVM: do not drop VMCB CPL to 0 if SS is not present To: Roman Penyaev , Mikhail Sennikovskii , Gleb Natapov , kvm@vger.kernel.org, linux-kernel@vger.kernel.org References: From: Paolo Bonzini Message-ID: <0155f13f-f65b-27a6-e646-44fffccbbf40@redhat.com> Date: Tue, 30 May 2017 16:47:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 30 May 2017 14:47:08 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.