From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support Date: Mon, 29 Jul 2013 16:20:35 +0300 Message-ID: <20130729132035.GK18009@redhat.com> References: <1374750001-28527-1-git-send-email-gleb@redhat.com> <1374750001-28527-10-git-send-email-gleb@redhat.com> <51F62EF3.6060104@redhat.com> <20130729105245.GD18009@redhat.com> <51F64B2A.6020503@redhat.com> <20130729114323.GG18009@redhat.com> <51F65A98.2040002@redhat.com> <20130729123410.GI18009@redhat.com> <51F66A0B.20108@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Xiao Guangrong , Jun Nakajima , Yang Zhang To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:8393 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756056Ab3G2NUj (ORCPT ); Mon, 29 Jul 2013 09:20:39 -0400 Content-Disposition: inline In-Reply-To: <51F66A0B.20108@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jul 29, 2013 at 03:11:39PM +0200, Paolo Bonzini wrote: > Il 29/07/2013 14:34, Gleb Natapov ha scritto: > >>>> But I think what you _really_ want is not avoiding conditional branches. > >>> The idea is that it is hard for branch prediction to predict correct > >>> result when correct result depends on guest's page table that can > >>> contain anything, so in some places shadow paging code uses boolean > >>> logic to avoid branches, in this case it is hard to avoid if() anyway > >>> since the function invocation is in the if(). > >> > >> Yes, I get the idea, but is_rsvd_bits_set should be predicted unlikely, > >> no? If the guest has to run, it must use mostly valid ptes. :) > >> > > You see, you are confused and you want branch prediction not to be? :) > > If your guest is KVM is_rsvd_bits_set() will be likely much more then > > unlikely because KVM misconfigures EPT entries to cache MMIO addresses, > > so all the "unlikely" cases will be fixed by shadow pages and will not > > reappear (until shadow pages are zapped), but misconfigured entries will > > continue to produces violations. > > But then: > > 1) MMIO is a slow path anyway, losing 10 cycles on a mispredicted branch > is not going to help much. Fast page faults are all I would optimize for. > Of course, for that the check should be fast. > 2) in cases like this you just do not use likely/unlikely; the branch > will be very unlikely in the beginning, and very likely once shadow > pages are filled or in the no-EPT case. Just let the branch predictor > adjust, it will probably do better than boolean tricks. > likely/unlikely are usually useless anyway. If you can avoid if() altogether this is a win since there is no branch to predict. > >> Especially if you change prefetch_invalid_gpte to do the reserved bits > >> test after the present test (so that is_rsvd_bits_set is only called on > >> present pagetables), is_rsvd_bits_set's result should be really > >> well-predicted. > > Nope, for ept page tables present is not a single bit, it is three bits > > which by themselves can have invalid values. > > We're not checking the validity of the bits in the is_present_gpte test, > we're checking it in the is_rsvd_bits_set test (is_present_gpte is doing > just "(pte & 7) != 0"). It doesn't change anything in the outcome of > prefetch_invalid_gpte, and it makes the ordering consistent with > walk_addr_generic which already tests presence before reserved bits. > > So doing this swap should be a win anyway. > > >> At this point (and especially since function invocation > >> is always in "if"s), using boolean logic to avoid branches does not make > >> much sense anymore for this function. > > > > That's true. > > So are you going to change to "if"s? > I think it will be better just to check mmu->bad_mt_xwr always. (I dislike ifdefs if you haven't noticed :)). -- Gleb.