From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bandan Das Subject: Re: [PATCH 3/3] KVM: nVMX: check for null vmcs12 when L1 does invept Date: Wed, 26 Mar 2014 16:22:28 -0400 Message-ID: References: <1395286089-5406-1-git-send-email-bsd@redhat.com> <1395286089-5406-4-git-send-email-bsd@redhat.com> <532AB624.4020400@siemens.com> <532D764E.20607@web.de> <532F3322.7060404@web.de> Mime-Version: 1.0 Content-Type: text/plain Cc: kvm@vger.kernel.org, Paolo Bonzini , Gleb Natapov To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:22683 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756099AbaCZUWb (ORCPT ); Wed, 26 Mar 2014 16:22:31 -0400 In-Reply-To: <532F3322.7060404@web.de> (Jan Kiszka's message of "Sun, 23 Mar 2014 20:16:50 +0100") Sender: kvm-owner@vger.kernel.org List-ID: Jan Kiszka writes: > On 2014-03-22 17:43, Bandan Das wrote: >> Jan Kiszka writes: >> >>> On 2014-03-20 21:58, Bandan Das wrote: >>>> Jan Kiszka writes: >>>> >>>>> On 2014-03-20 04:28, Bandan Das wrote: >>>>>> Some L1 hypervisors such as Xen seem to be calling invept after >>>>>> vmclear or before vmptrld on L2. In this case, proceed with >>>>>> falling through and syncing roots as a case where >>>>>> context wide invalidation can't be supported >>>>> >>>>> Can we also base this behaviour on a statement in the SDM? But on first >>>>> glance, I do not find anything like this over there. >>>> >>>> The SDM has nothing of this sort explicitly mentioned but 28.3.3.1 >>>> "Operations that invalidate Cached Mappings" does mention that >>>> the instruction may invalidate mappings associated with other >>>> EP4TAs (even in single context). >>> >>> Yes, "may". So we are implementing undefined behavior in order to please >>> a broken hypervisor that relies on it? Then please state this in the >>> patch and probably also inform Xen about their issue. >> >> Why undefined behavior ? We don't do anything specific for >> the single context invalidation case ianyway .e If the eptp matches what >> vmcs12 has, single context invalidation does fall though to the global >> invalidation case already. All this change does is add the "L1 calls >> invept after vmclear and before vmptrld" to the list of cases to fall >> though to global invalidation since nvmx doesn't have any knowledge of >> the current eptp for this case. > > OK, I think I misunderstood what the guest expects and how we currently > achieve this: we do not track the mapping between guest and host eptp, > thus cannot properly emulate its behaviour. We therefore need to flush > everything. > >> >> Or do you think we should rethink this approach ? > > Well, I wonder if we should expose single-context invept support at all. > > I'm also wondering if we are returning proper flags on > > if ((operand.eptp & eptp_mask) != > (nested_ept_get_cr3(vcpu) & eptp_mask)) > break; That does sound plausible but then we would have to get rid of this little optimization in the code you have quoted above. We would have to flush and sync roots for all invept calls. So, my preference is to keep it. > Neither nested_vmx_succeed nor nested_vmx_fail* is called if this > condition evaluates to true. It appears we should always call nested_vmx_succeed(). If you are ok, I will send a v2. Thanks, Bandan > Jan