Re: [PATCH] kvm: x86: vmx: remove MMIO_GEN_MASK

[Paolo Bonzini <pbonzini@redhat.com>]

On 18/11/2014 10:14, Tiejun Chen wrote:
> Actually MMIO_MAX_GEN is same as MMIO_GEN_MASK, and
> especially in all cases we already AND this so its
> also unnecessary to generate such a WARN_ON().
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Good idea, but I actually prefer keeping MMIO_GEN_MASK since it is often
used together with an "&".

Then, the WARN_ON is better written as

-	WARN_ON(gen > MMIO_MAX_GEN);
+	WARN_ON(gen & ~MMIO_GEN_MASK);

Paolo

> ---
>  arch/x86/kvm/mmu.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index ac1c4de..7712345 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -213,15 +213,12 @@ EXPORT_SYMBOL_GPL(kvm_mmu_set_mmio_spte_mask);
>  #define MMIO_GEN_SHIFT			20
>  #define MMIO_GEN_LOW_SHIFT		10
>  #define MMIO_GEN_LOW_MASK		((1 << MMIO_GEN_LOW_SHIFT) - 2)
> -#define MMIO_GEN_MASK			((1 << MMIO_GEN_SHIFT) - 1)
>  #define MMIO_MAX_GEN			((1 << MMIO_GEN_SHIFT) - 1)
>  
>  static u64 generation_mmio_spte_mask(unsigned int gen)
>  {
>  	u64 mask;
>  
> -	WARN_ON(gen > MMIO_MAX_GEN);
> -
>  	mask = (gen & MMIO_GEN_LOW_MASK) << MMIO_SPTE_GEN_LOW_SHIFT;
>  	mask |= ((u64)gen >> MMIO_GEN_LOW_SHIFT) << MMIO_SPTE_GEN_HIGH_SHIFT;
>  	return mask;
> @@ -240,7 +237,7 @@ static unsigned int get_mmio_spte_generation(u64 spte)
>  
>  static unsigned int kvm_current_mmio_generation(struct kvm *kvm)
>  {
> -	return kvm_memslots(kvm)->generation & MMIO_GEN_MASK;
> +	return kvm_memslots(kvm)->generation & MMIO_MAX_GEN;
>  }
>  
>  static void mark_mmio_spte(struct kvm *kvm, u64 *sptep, u64 gfn,
>