From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept Date: Mon, 14 Apr 2014 07:46:06 +0200 Message-ID: <534B761E.6070905@siemens.com> References: <1396299625-8285-1-git-send-email-bsd@redhat.com> <1396299625-8285-2-git-send-email-bsd@redhat.com> <20140410204738.GA28576@amt.cnet> <53478A15.9080903@siemens.com> <53482DF4.3030808@siemens.com> <53483A15.4030006@siemens.com> <20140411193511.GB19778@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Bandan Das , kvm@vger.kernel.org, Paolo Bonzini , Gleb Natapov To: Marcelo Tosatti Return-path: Received: from goliath.siemens.de ([192.35.17.28]:48634 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbaDNFqT (ORCPT ); Mon, 14 Apr 2014 01:46:19 -0400 In-Reply-To: <20140411193511.GB19778@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 2014-04-11 21:35, Marcelo Tosatti wrote: > On Fri, Apr 11, 2014 at 08:53:09PM +0200, Jan Kiszka wrote: >> On 2014-04-11 20:35, Bandan Das wrote: >>> Jan Kiszka writes: >>> >>>> On 2014-04-11 19:26, Bandan Das wrote: >>>>> Jan Kiszka writes: >>>>> >>>>>> On 2014-04-11 02:27, Bandan Das wrote: >>>>>>> Marcelo Tosatti writes: >>>>>>> >>>>>>>> On Mon, Mar 31, 2014 at 05:00:23PM -0400, Bandan Das wrote: >>>>>>>>> For single context invalidation, we fall through to global >>>>>>>>> invalidation in handle_invept() except for one case - when >>>>>>>>> the operand supplied by L1 is different from what we have in >>>>>>>>> vmcs12. However, typically hypervisors will only call invept >>>>>>>>> for the currently loaded eptp, so the condition will >>>>>>>>> never be true. >>>>>>>>> >>>>>>>>> Signed-off-by: Bandan Das >>>>>>>> >>>>>>>> Bandan, >>>>>>>> >>>>>>>> Why not fix INVEPT single-context rather than removing it entirely? >>>>>>>> >>>>>>>> "Single-context. If the INVEPT type is 1, the logical processor >>>>>>>> invalidates all guest-physical mappings and combined mappings associated >>>>>>>> with the EP4TA specified in the INVEPT descriptor. Combined mappings for >>>>>>>> that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction >>>>>>>> may invalidate mappings associated with other EP4TAs.)" >>>>>>>> >>>>>>>> So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough. >>>>>>> >>>>>>> The single context invalidation in handle_invept() doesn't do >>>>>>> anything different. It just falls down to the global case. >>>>>>> And the invept code in Xen and KVM both seemed to fall back >>>>>>> to global invalidation if support for single context wasn't found. >>>>>>> So, it was proposed not to advertise it at all. >>>>>>> >>>>>>> But rethinking this again, I agree with you. If there's a hypervisor >>>>>>> with a single context invept implmentation that does not fallback, >>>>>>> this will unfortunately not work. Jan, do you agree with this ? >>>>>> >>>>>> A hypervisor that doesn't properly check the HW caps is just broken. And >>>>>> one that mandates single context invalidation support is silly. >>>>> >>>>> Well, but we could make life a little bit easier for the unfortunate user >>>>> using the broken hypervisor :) And advertising single context inavalidation >>>>> doesn't really seem to have any downsides. >>>> >>>> Ok, let's try it this way: single-context invalidation is inherently >>>> tied to VPID support (that's how you address a context). However, KVM >>>> does not expose VPID to its guest. So this discussion is mood: no >>>> hypervisor will make use of this feature as it has no means to fill in >>>> the required parameter. >>> >>> I thought (from the spec) invept single context invalidation >>> takes the EP4TA as the second argument. invvpid single context >>> however takes the VPID as its descriptor. >> >> Oops, invept/invvpid mess-up while re-reading the spec - sorry. >> >>> >>> The Xen L1 hypervisor was actually calling single context invept >>> multiple times. That's how I hit this bug. >> >> ...and it's no longer doing it now, I suppose. The question remains, >> which hypervisor we want to cater with a >> "single-context-that-is-current-context" invalidation (that is my >> understanding of Marcelo's proposal). > > My proposal is to implement what is in the spec. > >> On the other hand, if some hypervisor actually uses invept to >> invalidate a non-current mapping, we would regress compared to not >> exposing single context invept. Hope I got this conclusion right. ;) > > In that case INVEPT global would also be broken. I'm all for having a proper invept single context support but that, first of all, requires tracking the vEPTP->EPTP mappings. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux