From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support
Date: Mon, 29 Jul 2013 13:52:45 +0300
Message-ID: <20130729105245.GD18009@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>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org,
	Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Yang Zhang <yang.z.zhang@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:4382 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750898Ab3G2Kwu (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 29 Jul 2013 06:52:50 -0400
Content-Disposition: inline
In-Reply-To: <51F62EF3.6060104@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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;
> > +#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. 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.

> > 
> > +	/*
> > +	 * 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.

--
			Gleb.