From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bandan Das Subject: Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Date: Tue, 28 Jun 2016 13:30:58 -0400 Message-ID: References: <1467088360-10186-1-git-send-email-bsd@redhat.com> <1467088360-10186-4-git-send-email-bsd@redhat.com> <60e083e8-596a-5641-fcb9-ede8bce32b58@redhat.com> Mime-Version: 1.0 Content-Type: text/plain Cc: kvm@vger.kernel.org, guangrong.xiao@linux.intel.com, linux-kernel@vger.kernel.org To: Paolo Bonzini Return-path: In-Reply-To: <60e083e8-596a-5641-fcb9-ede8bce32b58@redhat.com> (Paolo Bonzini's message of "Tue, 28 Jun 2016 10:57:45 +0200") Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Paolo Bonzini writes: > 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. > */ I think this should be: 2) for EPT page tables where an X-- page table entry is invalid and a EPT misconfig is injected to the guest before we reach here. > 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); > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html