Re: [PATCH v3 7/8] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits.

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Junaid Shahid" <junaids@google.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org, andreslc@google.com, pfeiner@google.com, "guangrong xiao" <guangrong.xiao@linux.intel.com>
> Sent: Wednesday, December 14, 2016 11:36:37 PM
> Subject: Re: [PATCH v3 7/8] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits.
> 
> On Wednesday, December 14, 2016 05:28:48 PM Paolo Bonzini wrote:
> > 
> > Just a few changes bordering on aesthetics...
> > 
> > Please review and let me know if you agree:
> > 
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 6ba62200530a..6b5d8ff66026 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -213,8 +213,8 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask)
> >  
> >  static inline bool is_access_track_spte(u64 spte)
> >  {
> > -	return shadow_acc_track_mask != 0 &&
> > -	       (spte & shadow_acc_track_mask) == shadow_acc_track_value;
> > +	/* Always false if shadow_acc_track_mask is zero.  */
> > +	return (spte & shadow_acc_track_mask) == shadow_acc_track_value;
> >  }
> 
> This looks good.
> 
> >  
> >  /*
> > @@ -526,23 +526,24 @@ static bool spte_can_locklessly_be_made_writable(u64
> > spte)
> >  
> >  static bool spte_has_volatile_bits(u64 spte)
> >  {
> > +	if (is_shadow_present_pte(spte))
> > +		return false;
> > +
> 
> This should be !is_shadow_present_pte

Caught me sending wrong patch. :(
 
> >  	/*
> >  	 * Always atomically update spte if it can be updated
> >  	 * out of mmu-lock, it can ensure dirty bit is not lost,
> >  	 * also, it can help us to get a stable is_writable_pte()
> >  	 * to ensure tlb flush is not missed.
> >  	 */
> > -	if (spte_can_locklessly_be_made_writable(spte))
> > +	if (spte_can_locklessly_be_made_writable(spte) ||
> > +	    is_access_track_spte(spte))
> >  		return true;
> >  
> > -	if (!is_shadow_present_pte(spte))
> > -		return false;
> > -
> > -	if (!shadow_accessed_mask)
> > -		return is_access_track_spte(spte);
> > +	if ((spte & shadow_accessed_mask) == 0 ||
> > +     	    (is_writable_pte(spte) && (spte & shadow_dirty_mask) == 0))
> > +		return true;
> 
> We also need a shadow_accessed_mask != 0 check here, otherwise it will always
> return true when shadow_accessed_mask is 0.

... but this is not the wrong patch, it's a genuine mistake.  Thanks!

Paolo