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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
