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

[Junaid Shahid <junaids@google.com>]

On Friday, December 16, 2016 09:04:56 PM Xiao Guangrong wrote:
> >  void kvm_mmu_set_mask_ptes(u64 user_mask, u64 accessed_mask,
> > -		u64 dirty_mask, u64 nx_mask, u64 x_mask, u64 p_mask);
> > +		u64 dirty_mask, u64 nx_mask, u64 x_mask, u64 p_mask,
> > +		u64 acc_track_mask);
> 
> Actually, this is the mask cleared by acc-track rather that _set_ by
> acc-track, maybe suppress_by_acc_track_mask is a better name.

Well, the original reason behind it was that a PTE is an access-track PTE if when masked by acc_track_mask, it yields acc_track_value. But we can change the name if it is confusing. Though suppress_by_acc_track_mask isn’t quite right since only the RWX bits are cleared, but the Special bit is set and the mask includes both of these.

> > +#define VMX_EPT_MT_MASK				(7ull << VMX_EPT_MT_EPTE_SHIFT)
> 
> I saw no space using this mask, can be dropped.

Ok. I’ll drop it.

> > +/* The mask for the R/X bits in EPT PTEs */
> > +#define PT64_EPT_READABLE_MASK			0x1ull
> > +#define PT64_EPT_EXECUTABLE_MASK		0x4ull
> > +
> 
> Can we move this EPT specific stuff out of mmu.c?

We need these in order to define the shadow_acc_track_saved_bits_mask and since we don’t have vmx.h included in mmu.c so I had to define these here. Is adding an #include for vmx.h better? Alternatively, we can have the shadow_acc_track_saved_bits_mask passed by kvm_intel when it loads, which was the case in the original version but I had changed it to a constant based on previous feedback.

> > +static inline bool is_access_track_spte(u64 spte)
> > +{
> > +	return shadow_acc_track_mask != 0 &&
> > +	       (spte & shadow_acc_track_mask) == shadow_acc_track_value;
> > +}
> 
> spte & SPECIAL_MASK && !is_mmio(spte) is more clearer.

We can change to that. But it seems less flexible as it assumes that there is never going to be a 3rd type of Special PTE.

> > +	/*
> > +	 * Verify that the write-protection that we do below will be fixable
> > +	 * via the fast page fault path. Currently, that is always the case, at
> > +	 * least when using EPT (which is when access tracking would be used).
> > +	 */
> > +	WARN_ONCE((spte & PT_WRITABLE_MASK) &&
> > +		  !spte_can_locklessly_be_made_writable(spte),
> > +		  "kvm: Writable SPTE is not locklessly dirty-trackable\n");
> 
> This code is right but i can not understand the comment here... :(

Basically, I was just trying to say that since making the PTE an acc-track PTE will remove the write access as well, so we better have the ability to restore the write access later in fast_page_fault. I’ll try to make the comment more clear.

> >
> > -		/*
> > -		 * Currently, to simplify the code, only the spte
> > -		 * write-protected by dirty-log can be fast fixed.
> > -		 */
> > -		if (!spte_can_locklessly_be_made_writable(spte))
> > +		remove_acc_track = is_access_track_spte(spte);
> > +
> 
> Why not check cached R/X permission can satisfy R/X access before goto atomic path?

Yes, I guess we can do that since if the restored PTE doesn’t satisfy the access we are just going to get another fault anyway.

> > +void vmx_enable_tdp(void)
> > +{
> > +	kvm_mmu_set_mask_ptes(VMX_EPT_READABLE_MASK,
> > +		enable_ept_ad_bits ? VMX_EPT_ACCESS_BIT : 0ull,
> > +		enable_ept_ad_bits ? VMX_EPT_DIRTY_BIT : 0ull,
> > +		0ull, VMX_EPT_EXECUTABLE_MASK,
> > +		cpu_has_vmx_ept_execute_only() ? 0ull : VMX_EPT_READABLE_MASK,
> > +		enable_ept_ad_bits ? 0ull : SPTE_SPECIAL_MASK | VMX_EPT_RWX_MASK);
> 
> I think commonly set SPTE_SPECIAL_MASK (move set SPTE_SPECIAL_MASK to mmu.c) for
> mmio-mask and acc-track-mask can make the code more clearer...

Ok. So you mean that vmx.c should just pass VMX_EPT_RWX_MASK here and VMX_EPT_MISCONFIG_WX_VALUE for the mmio mask and then mmu.c should add in SPTE_SPECIAL_MASK before storing these values in shadow_acc_track_mask and shadow_mmio_mask?


Thanks,
Junaid