Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 12:59:54PM +0200, Paolo Bonzini wrote:
> 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.
OK. I can add that :) I still prefer to have ifdefs outside the
function, not inside.

> 
> 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().

>  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.
Will do, but it is inlined regardless. It's very small.


> 
> > 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

--
			Gleb.