Re: [PATCH v4 06/13] nEPT: Add EPT tables support to paging_tmpl.h

[Paolo Bonzini <pbonzini@redhat.com>]

Il 30/07/2013 13:56, Gleb Natapov ha scritto:
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index 4c4274d..b5273c3 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -3494,6 +3494,11 @@ static inline bool is_last_gpte(struct kvm_mmu *mmu, unsigned level, unsigned gp
>>>  	return mmu->last_pte_bitmap & (1 << index);
>>>  }
>>>  
>>> +#define PTTYPE_EPT 18 /* arbitrary */
>>> +#define PTTYPE PTTYPE_EPT
>>> +#include "paging_tmpl.h"
>>> +#undef PTTYPE
>>> +
>>>  #define PTTYPE 64
>>>  #include "paging_tmpl.h"
>>>  #undef PTTYPE
>>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>>> index 7581395..e38b3c0 100644
>>> --- a/arch/x86/kvm/paging_tmpl.h
>>> +++ b/arch/x86/kvm/paging_tmpl.h
>>> @@ -58,6 +58,21 @@
>>>  	#define PT_GUEST_DIRTY_SHIFT PT_DIRTY_SHIFT
>>>  	#define PT_GUEST_ACCESSED_SHIFT PT_ACCESSED_SHIFT
>>>  	#define CMPXCHG cmpxchg
>>> +#elif PTTYPE == PTTYPE_EPT
>>> +	#define pt_element_t u64
>>> +	#define guest_walker guest_walkerEPT
>>> +	#define FNAME(name) ept_##name
>>> +	#define PT_BASE_ADDR_MASK PT64_BASE_ADDR_MASK
>>> +	#define PT_LVL_ADDR_MASK(lvl) PT64_LVL_ADDR_MASK(lvl)
>>> +	#define PT_LVL_OFFSET_MASK(lvl) PT64_LVL_OFFSET_MASK(lvl)
>>> +	#define PT_INDEX(addr, level) PT64_INDEX(addr, level)
>>> +	#define PT_LEVEL_BITS PT64_LEVEL_BITS
>>> +	#define PT_GUEST_ACCESSED_MASK 0
>>> +	#define PT_GUEST_DIRTY_MASK 0
>>> +	#define PT_GUEST_DIRTY_SHIFT 0
>>> +	#define PT_GUEST_ACCESSED_SHIFT 0
>>> +	#define CMPXCHG cmpxchg64
>>> +	#define PT_MAX_FULL_LEVELS 4
>>>  #else
>>>  	#error Invalid PTTYPE value
>>>  #endif
>>
>> Please add a
>>
>> BUILD_BUG_ON(!!PT_GUEST_ACCESSED_MASK != !!PT_GUEST_DIRTY_MASK);
>> #if PT_GUEST_ACCESSED_MASK
>> BUILD_BUG_ON(PT_GUEST_DIRTY_SHIFT <= PT_GUEST_ACCESSED_SHIFT);
>> BUILD_BUG_ON(PT_GUEST_DIRTY_SHIFT <= PT_WRITABLE_SHIFT);
>> BUILD_BUG_ON(PT_GUEST_DIRTY_MASK != (1 << PT_GUEST_DIRTY_SHIFT));
>> BUILD_BUG_ON(PT_GUEST_ACCESSED_MASK != (1 << PT_GUEST_ACCESSED_SHIFT));
>
> This will not work if I define _SHIFT to be 8/9 now.

They will because I put them under an appropriate "#if". :)

OTOH if you define _SHIFT to be 8/9 you can move the #if so that it only
covers the last two checks.

> But we do not use
> BUILD_BUG_ON() to check values from the same define "namespace". It is
> implied that they are correct and when they change all "namespace"
> remains to be consistent. If you look at BUILD_BUG_ON() that we have
> (and this patch adds) they are from the form:
>   PT_WRITABLE_MASK != ACC_WRITE_MASK
>   ACC_WRITE_MASK != VMX_EPT_WRITABLE_MASK
>   VMX_EPT_READABLE_MASK != PT_PRESENT_MASK
>   VMX_EPT_WRITABLE_MASK != PT_WRITABLE_MASK
> 
> i.e they compare value from different "namespaces".

Yes, all these BUILD_BUG_ONs make sense.

But I think BUILD_BUG_ON() is more generically for consistency checks
and enforcing invariants that the code expects.  Our invariants are:

* A/D bits are enabled or disabled in pairs

* dirty is the left of accessed and writable

* masks should either be zero or match the corresponding shifts

The alternative to BUILD_BUG_ON would be a comment that explains the
invariants, but there's no need to use English if C can do it better! :)

>>>  	pt_element_t pte;
>>>  	pt_element_t __user *uninitialized_var(ptep_user);
>>>  	gfn_t table_gfn;
>>> @@ -322,7 +351,9 @@ retry_walk:
>>>  		accessed_dirty &= pte >>
>>>  			(PT_GUEST_DIRTY_SHIFT - PT_GUEST_ACCESSED_SHIFT);
>>
>> This shift is one of two things that bugged me.  I dislike including
>> "wrong" code just because it is dead.  Perhaps you can #define the
> Meanwhile I changed comment above this to be:
>            /*
>             * On a write fault, fold the dirty bit into accessed_dirty.
>             * For modes without A/D bits support accessed_dirty will be
>             * always clear.
>             */

Good.

>> shifts to 8 and 9 already now, even if the masks stay 0?
>>
> Currently I do not see any problem with that, but we will have to be careful
> that *_SHIFT values will not leak into a code where it could matter.

They would leak with PT_GUEST_*_SHIFT == 0 too, I think?  (And with
worse effects, because they would use bit 0 and/or do shifts with
negative counts).

Perhaps PT_GUEST_*_SHIFT can instead be defined to a function like
__using_nonexistent_pte_bit(), so that compilation fails unless all such
references are optimized away.  See arch/x86/include/asm/cmpxchg.h for
an example:

/*
 * Non-existant functions to indicate usage errors at link time
 * (or compile-time if the compiler implements __compiletime_error().
 */
extern void __xchg_wrong_size(void)
        __compiletime_error("Bad argument size for xchg");

But using 8/9 would be fine for me.  Choose the one that you prefer.

>> Then when you implement nEPT A/D bits you only have to flip the masks
>> from 0 to (1 << PT_GUEST_*_SHIFT).
>>
>>>  
>>> -	if (unlikely(!accessed_dirty)) {
>>> +	if (PT_GUEST_DIRTY_MASK && unlikely(!accessed_dirty)) {
>>
>> If we want to drop the dead-code elimination burden on the compiler,
>> let's go all the way.
>>
>> So, instead of this "if", please make update_accessed_dirty_bits return
>> 0 at once if PT_GUEST_DIRTY_MASK == 0; this way you can do the same
>> thing for protect_clean_gpte and update_accessed_dirty_bits.
>
> OK.

Good. :)

Paolo