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

[Paolo Bonzini <pbonzini@redhat.com>]

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