Re: [PATCH v2 4/5] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits.

[Paolo Bonzini <pbonzini@redhat.com>]

> > >> Let's separate the three conditions (R/W/X):
> > >>
> > >> 		if ((error_code & PFERR_FETCH_MASK) {
> > >> 			if ((spte & (shadow_x_mask|shadow_nx_mask))
> > >> 			    == shadow_x_mask) {
> > >> 				fault_handled = true;
> > >> 				break;
> > >> 			}
> > >> 		}
> > >> 		if (error_code & PFERR_WRITE_MASK) {
> > >> 			if (is_writable_pte(spte)) {
> > >> 				fault_handled = true;
> > >> 				break;
> > >> 			}
> > >> 			remove_write_prot =
> > >> 				spte_can_locklessly_be_made_writable(spte);
> > >> 		}
> > >> 		if (!(error_code & PFERR_PRESENT_MASK)) {
> > >> 			if (!is_access_track_spte(spte)) {
> > >> 				fault_handled = true;
> > >> 				break;
> > >> 			}
> > >> 			remove_acc_track = true;
> > >> 		}
> > > 
> > > I think the third block is incorrect e.g. it will set fault_handled =
> > > true even
> > > for a completely zero PTE.
> > 
> > A completely zero PTE would have been filtered before by the
> > is_shadow_present_pte check, wouldn't it?
> 
> Oh, the is_shadow_present_pte check was actually removed in the patch. We could
> add it back, minus the ret = true statement, and then it would filter the zero
> PTE case. But I still think that the other form:
> 
>                 if ((error_code & PFERR_USER_MASK) &&
>                     (spte & PT_PRESENT_MASK)) {
>                         fault_handled = true;
>                         break;
>                 }
> 
> is simpler as it is directly analogous to the cases for fetch and write.
> Please let me know if you think otherwise.

Fair enough, but add a comment to explain the error_code check because I
don't get it. :)

Paolo