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 19:54:21 +0300 Message-ID: <20130729165421.GD28372@redhat.com> References: <20130729105245.GD18009@redhat.com> <51F64B2A.6020503@redhat.com> <20130729114323.GG18009@redhat.com> <51F65A98.2040002@redhat.com> <20130729123410.GI18009@redhat.com> <51F66A0B.20108@redhat.com> <20130729132035.GK18009@redhat.com> <51F67851.8070408@redhat.com> <20130729162450.GB28372@redhat.com> <51F699FC.4010905@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]:26110 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754197Ab3G2Qyc (ORCPT ); Mon, 29 Jul 2013 12:54:32 -0400 Content-Disposition: inline In-Reply-To: <51F699FC.4010905@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jul 29, 2013 at 06:36:12PM +0200, Paolo Bonzini wrote: > Il 29/07/2013 18:24, Gleb Natapov ha scritto: > > On Mon, Jul 29, 2013 at 04:12:33PM +0200, Paolo Bonzini wrote: > >> Il 29/07/2013 15:20, Gleb Natapov ha scritto: > >>>> 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. > >> > >> However, if the branches are dynamically well-predicted, > >> > >> if (simple) > >> ... > >> if (complex) > >> ... > >> > >> is likely faster than > >> > >> if (simple | complex) > >> > >> because the branches then are very very cheap, and it pays off to not > >> always evaluate the complex branch. > > > > Good point about about "|" always evaluating both. Is this the case > > with if (simple !=0 | complex != 0) too where theoretically compiler may > > see that if simple !=0 is true no need to evaluate the second one? > > Yes (only if complex doesn't have any side effects, which is the case here). > > >> Yeah, I also thought of always checking bad_mt_xwr and even using it to > >> subsume the present check too, i.e. turning it into > >> is_rsvd_bits_set_or_nonpresent. It checks the same bits that are used > >> in the present check (well, a superset). You can then check for > >> presence separately if you care, which you don't in > >> prefetch_invalid_gpte. It requires small changes in the callers but > >> nothing major. > > > > I do not get what is_rsvd_bits_set_or_nonpresent() will check exactly > > and why do we needed it, there are two places where we check > > present/reserved and in one of them we need to know which one it is. > > You can OR bad_mt_xwr with 0x5555555555555555ULL (I think). Then your With 0x1010101. > implementation of is_rsvd_bits_set() using bad_mt_xwr will return true > in all cases where the pte is non-present. You can then call > is_present_pte to discriminate the two cases. > > if (is_rsvd_bits_set_or_nonpresent) { > if (!present) > ... > else > ... > } > > In more abstract terms this is: > > if (simple) > ... > if (complex) > ... > > to > > if (simple_or_complex) { > if (simple) > ... > else > ... > } > > This can actually make sense if simple is almost always false, because > then you save something from not evaluating it on the fast path. > > But in this case, adding bad_mt_xwr to the non-EPT case is a small loss. > > > Anyway order of checks in prefetch_invalid_gpte() is not relevant to > > that patchset, so lets better leave it to a separate discussion. > > Yes. > > Paolo > > >> > >> But it still seems to me that we're in the above "if (simple || > >> complex)" case and having a separate "if (!present)" check will be faster. > >> > >> Paolo > > > > -- > > Gleb. > > -- Gleb.