From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support Date: Mon, 29 Jul 2013 12:59:54 +0200 Message-ID: <51F64B2A.6020503@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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Xiao Guangrong , Jun Nakajima , Yang Zhang To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:26633 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751183Ab3G2LA3 (ORCPT ); Mon, 29 Jul 2013 07:00:29 -0400 In-Reply-To: <20130729105245.GD18009@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Il 29/07/2013 12:52, Gleb Natapov ha scritto: > On Mon, Jul 29, 2013 at 10:59:31AM +0200, Paolo Bonzini wrote: >> Il 25/07/2013 12:59, Gleb Natapov ha scritto: >>> +#if PTTYPE == PTTYPE_EPT >>> +#define CHECK_BAD_MT_XWR(G) mmu->bad_mt_xwr & (1ull << ((G) & 0x3f)); >>> +#else >>> +#define CHECK_BAD_MT_XWR(G) 0; No semicolons here, BTW. >>> +#endif >>> + >>> static bool FNAME(is_rsvd_bits_set)(struct kvm_mmu *mmu, u64 gpte, int level) >>> { >>> int bit7; >>> >>> bit7 = (gpte >> 7) & 1; >>> - return (gpte & mmu->rsvd_bits_mask[bit7][level-1]) != 0; >>> + return ((gpte & mmu->rsvd_bits_mask[bit7][level-1]) != 0) || >>> + CHECK_BAD_MT_XWR(gpte); >>> } >>> >>> +#undef CHECK_BAD_MT_XWR >> >> Instead of a macro, you can do >> >> if (...) >> return true; >> #if PTTYPE == PTTYPE_EPT >> if (...) >> return true; >> #endif >> return false; >> >> The compiler should be smart enough to generate the same code for >> non-EPT PTTYPE. >> > The idea behind this rsvd_bits_mask trickery is to produce code that > does not have conditional branches. If you want to have no conditional branches, you need to use "|" not "||" (and you also need an "!= 0" in CHECK_BAD_MT_XWR). As you wrote it, the compiler is most likely to generate exactly the same code that I suggested. But I think what you _really_ want is not avoiding conditional branches. What you want is always inline is_rsvd_bits_set, so that the compiler can merge these "if"s with the one where the caller calls is_rsvd_bits_set. So just mark is_rsvd_bits_set as inline. > I don't want to rely on compiler to do > the right things. On the other hand I am not sure that just dropping this > ifdefs here and checking mmu->bad_mt_xwr for non ept case is not a > good idea. The overhead should not be measurable. That's also a possibility, but I think if you mark is_rsvd_bits_set as inline it is better to leave the ifs separate. >>> >>> + /* >>> + * Use PFERR_RSVD_MASK in erorr_code to to tell if EPT >>> + * misconfiguration requires to be injected. The detection is >>> + * done by is_rsvd_bits_set() above. >> >> erorr_code -> error_code >> >> This patch has warnings for unused static functions. You can squash >> them, or split them differently according to file boundaries (i.e. mmu.c >> first, vmx.c second). >> > I prefer to have an in between patch with a warning, but do not divide > code that logically belongs together between patches. Fine. Paolo