Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

Paolo Bonzini
2016-06-28T08:57:51Z

On 28/06/2016 06:32, Bandan Das wrote:
> + bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
> + (1ull << VMX_EPT_EXECUTABLE_MASK));
>
> if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
> return 0;
>
> - spte = PT_PRESENT_MASK;
> + if (!execonly)
> + spte |= PT_PRESENT_MASK;

This needs a comment:

/*
* There are two cases in which execonly is false: 1) for
* non-EPT page tables, in which case we need to set the
* P bit; 2) for EPT page tables where an X-- page table
* entry is invalid, in which case we need to force the R
* bit of the page table entry to 1.
*/
BUILD_BUG_ON(PT_PRESENT_MASK != VMX_EPT_READABLE_MASK);
if (!execonly)
spte |= PT_PRESENT_MASK;

> if (!speculative)
> spte |= shadow_accessed_mask;
>
> if (enable_ept) {
> - kvm_mmu_set_mask_ptes(0ull,
> + kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,

This should be VMX_EPT_READABLE_MASK.

> @@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
> else
> spte |= shadow_nx_mask;
>
> + /* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */

I don't think this comment is necessary, but it's better to add one in
FNAME(gpte_access).

/*
* In the EPT case, a page table can be executable but not
* readable (on some processors). Therefore, set_spte does not
* automatically set bit 0 if execute-only is supported.
* Instead, since EPT page tables do not have a U bit, we
* repurpose ACC_USER_MASK to signify readability. Likewise,
* when EPT is in use shadow_user_mask is set to
* VMX_EPT_READABLE_MASK.
*/

Thanks,

Paolo

> if (pte_access & ACC_USER_MASK)
> spte |= shadow_user_mask;

Paolo

> (enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
> (enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
> 0ull, VMX_EPT_EXECUTABLE_MASK);