| Message ID | 1466478746-14153-4-git-send-email-bsd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/06/2016 05:12, Bandan Das wrote: > To support execute only mappings on behalf of L1 hypervisors, > we teach set_spte to honor L1's valid XWR bits. This is only > if host supports EPT execute only. Use ACC_USER_MASK to signify > if the L1 hypervisor has the present bit set. has the "R" bit set. > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > arch/x86/kvm/mmu.c | 11 ++++++++--- > arch/x86/kvm/paging_tmpl.h | 2 +- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 57d8696..3ca1a99 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) > return 0; > > - spte = PT_PRESENT_MASK; > + if (!shadow_xonly_valid) > + spte = PT_PRESENT_MASK; > if (!speculative) > spte |= shadow_accessed_mask; > > @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > else > spte |= shadow_nx_mask; > > - if (pte_access & ACC_USER_MASK) > - spte |= shadow_user_mask; > + if (pte_access & ACC_USER_MASK) { > + if (shadow_xonly_valid) > + spte |= PT_PRESENT_MASK; > + else > + spte |= shadow_user_mask; > + } Can you instead pass VMX_READABLE_MASK to kvm_mmu_set_mask_ptes in vmx.c? > > if (level > PT_PAGE_TABLE_LEVEL) > spte |= PT_PAGE_SIZE_MASK; > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > index 9f5bd06..5366a55 100644 > --- a/arch/x86/kvm/paging_tmpl.h > +++ b/arch/x86/kvm/paging_tmpl.h > @@ -192,7 +192,7 @@ static inline unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, u64 gpte) > #if PTTYPE == PTTYPE_EPT > access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) | > ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | > - ACC_USER_MASK; > + ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); This part is fine. Paolo > #else > BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK); > BUILD_BUG_ON(ACC_EXEC_MASK != 1); > -- 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
On 21/06/2016 10:12, Paolo Bonzini wrote: >> return 0; >> >> - spte = PT_PRESENT_MASK; >> + if (!shadow_xonly_valid) >> + spte = PT_PRESENT_MASK; I forgot to note that you need an "spte = 0;" here. Paolo -- 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
On 06/21/2016 11:12 AM, Bandan Das wrote: > To support execute only mappings on behalf of L1 hypervisors, > we teach set_spte to honor L1's valid XWR bits. This is only > if host supports EPT execute only. Use ACC_USER_MASK to signify > if the L1 hypervisor has the present bit set. > > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > arch/x86/kvm/mmu.c | 11 ++++++++--- > arch/x86/kvm/paging_tmpl.h | 2 +- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 57d8696..3ca1a99 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) > return 0; > > - spte = PT_PRESENT_MASK; > + if (!shadow_xonly_valid) > + spte = PT_PRESENT_MASK; The xonly info can be fetched from vcpu->mmu. shadow_xonly_valid looks like can be dropped. > if (!speculative) > spte |= shadow_accessed_mask; > > @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > else > spte |= shadow_nx_mask; > > - if (pte_access & ACC_USER_MASK) > - spte |= shadow_user_mask; > + if (pte_access & ACC_USER_MASK) { > + if (shadow_xonly_valid) > + spte |= PT_PRESENT_MASK; > + else > + spte |= shadow_user_mask; > + } It can be simplified by setting shadow_user_mask to PT_PRESENT_MASK if ept enabled. -- 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
Paolo Bonzini <pbonzini@redhat.com> writes: > On 21/06/2016 05:12, Bandan Das wrote: >> To support execute only mappings on behalf of L1 hypervisors, >> we teach set_spte to honor L1's valid XWR bits. This is only >> if host supports EPT execute only. Use ACC_USER_MASK to signify >> if the L1 hypervisor has the present bit set. > > has the "R" bit set. Yep, noted! >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> arch/x86/kvm/mmu.c | 11 ++++++++--- >> arch/x86/kvm/paging_tmpl.h | 2 +- >> 2 files changed, 9 insertions(+), 4 deletions(-) ... >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >> if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) >> return 0; >> >> - spte = PT_PRESENT_MASK; >> + if (!shadow_xonly_valid) >> + spte = PT_PRESENT_MASK; >> if (!speculative) >> spte |= shadow_accessed_mask; >> >> @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >> else >> spte |= shadow_nx_mask; >> >> - if (pte_access & ACC_USER_MASK) >> - spte |= shadow_user_mask; >> + if (pte_access & ACC_USER_MASK) { >> + if (shadow_xonly_valid) >> + spte |= PT_PRESENT_MASK; >> + else >> + spte |= shadow_user_mask; >> + } > > Can you instead pass VMX_READABLE_MASK to kvm_mmu_set_mask_ptes in vmx.c? So, leave spte = PT_PRESENT_MASK as is and make VMX_READABLE_MASK 1ULL if execute only is supported ? And then : if !(pte_access & ACC_USER_MASK) { spte &= ~VMX_READABLE_MASK >> if (level > PT_PAGE_TABLE_LEVEL) >> spte |= PT_PAGE_SIZE_MASK; >> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h >> index 9f5bd06..5366a55 100644 >> --- a/arch/x86/kvm/paging_tmpl.h >> +++ b/arch/x86/kvm/paging_tmpl.h >> @@ -192,7 +192,7 @@ static inline unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, u64 gpte) >> #if PTTYPE == PTTYPE_EPT >> access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) | >> ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | >> - ACC_USER_MASK; >> + ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); > > This part is fine. > > Paolo > >> #else >> BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK); >> BUILD_BUG_ON(ACC_EXEC_MASK != 1); >> -- 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
On 22/06/2016 18:10, Bandan Das wrote: >>> >> + if (!shadow_xonly_valid) >>> >> + spte = PT_PRESENT_MASK; >>> >> if (!speculative) >>> >> spte |= shadow_accessed_mask; >>> >> >>> >> @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >>> >> else >>> >> spte |= shadow_nx_mask; >>> >> >>> >> - if (pte_access & ACC_USER_MASK) >>> >> - spte |= shadow_user_mask; >>> >> + if (pte_access & ACC_USER_MASK) { >>> >> + if (shadow_xonly_valid) >>> >> + spte |= PT_PRESENT_MASK; >>> >> + else >>> >> + spte |= shadow_user_mask; >>> >> + } >> > >> > Can you instead pass VMX_READABLE_MASK to kvm_mmu_set_mask_ptes in vmx.c? > So, leave spte = PT_PRESENT_MASK as is and make VMX_READABLE_MASK 1ULL if > execute only is supported ? > And then : > if !(pte_access & ACC_USER_MASK) { > spte &= ~VMX_READABLE_MASK > No, I meant something like spte = 0; if (!shadow_xonly_valid) spte = PT_PRESENT_MASK; ... if (pte_access & ACC_USER_MASK) spte |= shadow_user_mask; Paolo -- 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
Xiao Guangrong <guangrong.xiao@linux.intel.com> writes: > On 06/21/2016 11:12 AM, Bandan Das wrote: >> To support execute only mappings on behalf of L1 hypervisors, >> we teach set_spte to honor L1's valid XWR bits. This is only >> if host supports EPT execute only. Use ACC_USER_MASK to signify >> if the L1 hypervisor has the present bit set. >> >> Signed-off-by: Bandan Das <bsd@redhat.com> >> --- >> arch/x86/kvm/mmu.c | 11 ++++++++--- >> arch/x86/kvm/paging_tmpl.h | 2 +- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index 57d8696..3ca1a99 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >> if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) >> return 0; >> >> - spte = PT_PRESENT_MASK; >> + if (!shadow_xonly_valid) >> + spte = PT_PRESENT_MASK; > > The xonly info can be fetched from vcpu->mmu. shadow_xonly_valid looks like > can be dropped. I added shadow_xonly_valid mainly for is_shadow_present_pte and since it seems it isn't needed there, I will drop it. >> if (!speculative) >> spte |= shadow_accessed_mask; >> >> @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >> else >> spte |= shadow_nx_mask; >> >> - if (pte_access & ACC_USER_MASK) >> - spte |= shadow_user_mask; >> + if (pte_access & ACC_USER_MASK) { >> + if (shadow_xonly_valid) >> + spte |= PT_PRESENT_MASK; >> + else >> + spte |= shadow_user_mask; >> + } > > It can be simplified by setting shadow_user_mask to PT_PRESENT_MASK > if ept enabled. Ok, sounds good. -- 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
Paolo Bonzini <pbonzini@redhat.com> writes: > On 22/06/2016 18:10, Bandan Das wrote: >>>> >> + if (!shadow_xonly_valid) >>>> >> + spte = PT_PRESENT_MASK; >>>> >> if (!speculative) >>>> >> spte |= shadow_accessed_mask; >>>> >> >>>> >> @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, >>>> >> else >>>> >> spte |= shadow_nx_mask; >>>> >> >>>> >> - if (pte_access & ACC_USER_MASK) >>>> >> - spte |= shadow_user_mask; >>>> >> + if (pte_access & ACC_USER_MASK) { >>>> >> + if (shadow_xonly_valid) >>>> >> + spte |= PT_PRESENT_MASK; >>>> >> + else >>>> >> + spte |= shadow_user_mask; >>>> >> + } >>> > >>> > Can you instead pass VMX_READABLE_MASK to kvm_mmu_set_mask_ptes in vmx.c? >> So, leave spte = PT_PRESENT_MASK as is and make VMX_READABLE_MASK 1ULL if >> execute only is supported ? >> And then : >> if !(pte_access & ACC_USER_MASK) { >> spte &= ~VMX_READABLE_MASK >> > > No, I meant something like > > spte = 0; > if (!shadow_xonly_valid) > spte = PT_PRESENT_MASK; > ... > if (pte_access & ACC_USER_MASK) > spte |= shadow_user_mask; Ok, Xiao mentioned this too. I will fix it in the next version. Thanks for the review. > Paolo > -- > 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 -- 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
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 57d8696..3ca1a99 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access)) return 0; - spte = PT_PRESENT_MASK; + if (!shadow_xonly_valid) + spte = PT_PRESENT_MASK; if (!speculative) spte |= shadow_accessed_mask; @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, else spte |= shadow_nx_mask; - if (pte_access & ACC_USER_MASK) - spte |= shadow_user_mask; + if (pte_access & ACC_USER_MASK) { + if (shadow_xonly_valid) + spte |= PT_PRESENT_MASK; + else + spte |= shadow_user_mask; + } if (level > PT_PAGE_TABLE_LEVEL) spte |= PT_PAGE_SIZE_MASK; diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 9f5bd06..5366a55 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -192,7 +192,7 @@ static inline unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, u64 gpte) #if PTTYPE == PTTYPE_EPT access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) | ((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) | - ACC_USER_MASK; + ((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0); #else BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK); BUILD_BUG_ON(ACC_EXEC_MASK != 1);
To support execute only mappings on behalf of L1 hypervisors, we teach set_spte to honor L1's valid XWR bits. This is only if host supports EPT execute only. Use ACC_USER_MASK to signify if the L1 hypervisor has the present bit set. Signed-off-by: Bandan Das <bsd@redhat.com> --- arch/x86/kvm/mmu.c | 11 ++++++++--- arch/x86/kvm/paging_tmpl.h | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-)