diff mbox series

[05/21] KVM: VMX: Teach EPT violation helper about private mem

Message ID 20240904030751.117579-6-rick.p.edgecombe@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX MMU Part 2 | expand

Commit Message

Edgecombe, Rick P Sept. 4, 2024, 3:07 a.m. UTC 
Teach EPT violation helper to check shared mask of a GPA to find out
whether the GPA is for private memory.

When EPT violation is triggered after TD accessing a private GPA, KVM will
exit to user space if the corresponding GFN's attribute is not private.
User space will then update GFN's attribute during its memory conversion
process. After that, TD will re-access the private GPA and trigger EPT
violation again. Only with GFN's attribute matches to private, KVM will
fault in private page, map it in mirrored TDP root, and propagate changes
to private EPT to resolve the EPT violation.

Relying on GFN's attribute tracking xarray to determine if a GFN is
private, as for KVM_X86_SW_PROTECTED_VM, may lead to endless EPT
violations.

Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
TDX MMU part 2 v1:
 - Split from "KVM: TDX: handle ept violation/misconfig exit"
---
 arch/x86/kvm/vmx/common.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Paolo Bonzini Sept. 9, 2024, 1:59 p.m. UTC | #1 
On 9/4/24 05:07, Rick Edgecombe wrote:
> Teach EPT violation helper to check shared mask of a GPA to find out
> whether the GPA is for private memory.
> 
> When EPT violation is triggered after TD accessing a private GPA, KVM will
> exit to user space if the corresponding GFN's attribute is not private.
> User space will then update GFN's attribute during its memory conversion
> process. After that, TD will re-access the private GPA and trigger EPT
> violation again. Only with GFN's attribute matches to private, KVM will
> fault in private page, map it in mirrored TDP root, and propagate changes
> to private EPT to resolve the EPT violation.
> 
> Relying on GFN's attribute tracking xarray to determine if a GFN is
> private, as for KVM_X86_SW_PROTECTED_VM, may lead to endless EPT
> violations.
> 
> Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> TDX MMU part 2 v1:
>   - Split from "KVM: TDX: handle ept violation/misconfig exit"
> ---
>   arch/x86/kvm/vmx/common.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 78ae39b6cdcd..10aa12d45097 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -6,6 +6,12 @@
>   
>   #include "mmu.h"
>   
> +static inline bool kvm_is_private_gpa(struct kvm *kvm, gpa_t gpa)
> +{
> +	/* For TDX the direct mask is the shared mask. */
> +	return !kvm_is_addr_direct(kvm, gpa);
> +}
> +
>   static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
>   					     unsigned long exit_qualification)
>   {
> @@ -28,6 +34,13 @@ static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
>   		error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) ?
>   			      PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
>   
> +	/*
> +	 * Don't rely on GFN's attribute tracking xarray to prevent EPT violation
> +	 * loops.
> +	 */
> +	if (kvm_is_private_gpa(vcpu->kvm, gpa))
> +		error_code |= PFERR_PRIVATE_ACCESS;
> +
>   	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
>   }
>   

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Chao Gao Sept. 11, 2024, 8:52 a.m. UTC | #2 
On Tue, Sep 03, 2024 at 08:07:35PM -0700, Rick Edgecombe wrote:
>Teach EPT violation helper to check shared mask of a GPA to find out
>whether the GPA is for private memory.
>
>When EPT violation is triggered after TD accessing a private GPA, KVM will
>exit to user space if the corresponding GFN's attribute is not private.
>User space will then update GFN's attribute during its memory conversion
>process. After that, TD will re-access the private GPA and trigger EPT
>violation again. Only with GFN's attribute matches to private, KVM will
>fault in private page, map it in mirrored TDP root, and propagate changes
>to private EPT to resolve the EPT violation.
>
>Relying on GFN's attribute tracking xarray to determine if a GFN is
>private, as for KVM_X86_SW_PROTECTED_VM, may lead to endless EPT
>violations.
>
>Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
>Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>---
>TDX MMU part 2 v1:
> - Split from "KVM: TDX: handle ept violation/misconfig exit"
>---
> arch/x86/kvm/vmx/common.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
>index 78ae39b6cdcd..10aa12d45097 100644
>--- a/arch/x86/kvm/vmx/common.h
>+++ b/arch/x86/kvm/vmx/common.h
>@@ -6,6 +6,12 @@
> 
> #include "mmu.h"
> 
>+static inline bool kvm_is_private_gpa(struct kvm *kvm, gpa_t gpa)
>+{
>+	/* For TDX the direct mask is the shared mask. */
>+	return !kvm_is_addr_direct(kvm, gpa);
>+}
>+
> static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
> 					     unsigned long exit_qualification)
> {
>@@ -28,6 +34,13 @@ static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
> 		error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) ?
> 			      PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
> 
>+	/*
>+	 * Don't rely on GFN's attribute tracking xarray to prevent EPT violation
>+	 * loops.
>+	 */

The comment seems a bit odd to me. We cannot use the gfn attribute from the
attribute xarray simply because here we need to determine if *this access* is
to private memory, which may not match the gfn attribute. Even if there are
other ways to prevent an infinite EPT violation loop, we still need to check
the shared bit in the faulting GPA.

>+	if (kvm_is_private_gpa(vcpu->kvm, gpa))
>+		error_code |= PFERR_PRIVATE_ACCESS;
>+
> 	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
> }
> 
>-- 
>2.34.1
>
>
Edgecombe, Rick P Sept. 11, 2024, 4:29 p.m. UTC | #3 
On Wed, 2024-09-11 at 16:52 +0800, Chao Gao wrote:
> > +       /*
> > +        * Don't rely on GFN's attribute tracking xarray to prevent EPT
> > violation
> > +        * loops.
> > +        */
> 
> The comment seems a bit odd to me. We cannot use the gfn attribute from the
> attribute xarray simply because here we need to determine if *this access* is
> to private memory, which may not match the gfn attribute. Even if there are
> other ways to prevent an infinite EPT violation loop, we still need to check
> the shared bit in the faulting GPA.

Yea this comment is not super informative. We can probably just drop it.
Huang, Kai Sept. 12, 2024, 12:39 a.m. UTC | #4 
On 4/09/2024 3:07 pm, Rick Edgecombe wrote:
> Teach EPT violation helper to check shared mask of a GPA to find out
> whether the GPA is for private memory.
> 
> When EPT violation is triggered after TD accessing a private GPA, KVM will
> exit to user space if the corresponding GFN's attribute is not private.
> User space will then update GFN's attribute during its memory conversion
> process. After that, TD will re-access the private GPA and trigger EPT
> violation again. Only with GFN's attribute matches to private, KVM will
> fault in private page, map it in mirrored TDP root, and propagate changes
> to private EPT to resolve the EPT violation.
> 
> Relying on GFN's attribute tracking xarray to determine if a GFN is
> private, as for KVM_X86_SW_PROTECTED_VM, may lead to endless EPT
> violations.
> 
> Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> TDX MMU part 2 v1:
>   - Split from "KVM: TDX: handle ept violation/misconfig exit"
> ---
>   arch/x86/kvm/vmx/common.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 78ae39b6cdcd..10aa12d45097 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -6,6 +6,12 @@
>   
>   #include "mmu.h"
>   
> +static inline bool kvm_is_private_gpa(struct kvm *kvm, gpa_t gpa)
> +{
> +	/* For TDX the direct mask is the shared mask. */
> +	return !kvm_is_addr_direct(kvm, gpa);
> +}

Does this get used in any other places?  If no I think we can open code 
this in the __vmx_handle_ept_violation().

The reason is I think the name kvm_is_private_gpa() is too generic and 
this is in the header file.  E.g., one can come up with another 
kvm_is_private_gpa() checking the memory attributes to tell whether a 
GPA is private.

Or we rename it to something like

	__vmx_is_faulting_gpa_private()
?

Which clearly says it is checking the *faulting* GPA.

> +
>   static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
>   					     unsigned long exit_qualification)
>   {
> @@ -28,6 +34,13 @@ static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
>   		error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) ?
>   			      PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
>   
> +	/*
> +	 * Don't rely on GFN's attribute tracking xarray to prevent EPT violation
> +	 * loops.
> +	 */
> +	if (kvm_is_private_gpa(vcpu->kvm, gpa))
> +		error_code |= PFERR_PRIVATE_ACCESS;
> +
>   	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
>   }
>
Huang, Kai Sept. 12, 2024, 1:19 a.m. UTC | #5 
On 4/09/2024 3:07 pm, Rick Edgecombe wrote:
> Teach EPT violation helper to check shared mask of a GPA to find out
> whether the GPA is for private memory.
> 
> When EPT violation is triggered after TD accessing a private GPA, KVM will
> exit to user space if the corresponding GFN's attribute is not private.
> User space will then update GFN's attribute during its memory conversion
> process. After that, TD will re-access the private GPA and trigger EPT
> violation again. Only with GFN's attribute matches to private, KVM will
> fault in private page, map it in mirrored TDP root, and propagate changes
> to private EPT to resolve the EPT violation.
> 
> Relying on GFN's attribute tracking xarray to determine if a GFN is
> private, as for KVM_X86_SW_PROTECTED_VM, may lead to endless EPT
> violations.

Sorry for not finishing in the previous reply:

IMHO in the very beginning of fault handler, we should just use hardware 
as the source to determine whether a *faulting* GPA is private or not. 
It doesn't quite matter whether KVM maintains memory attributes and how 
does it handle based on it -- it just must handle this properly.

E.g., even using memory attributes (to determine private) won't lead to 
endless EPT violations, it is wrong to use it to determine, because at 
the beginning of fault handler, we must know the *hardware* behaviour.

So I think the changelog should be something like this (the title could 
be enhanced too perhaps):

When TDX guests access memory causes EPT violation, TDX determines 
whether the faulting GPA is private or shared by checking whether the 
faulting GPA contains the shared bit (either bit 47 or bit 51 depending 
on the configuration of the guest).

KVM maintains an Xarray to record whether a GPA is private or not, e.g., 
for KVM_X86_SW_PROTECTED_VM guests.  TDX needs to honor this too.  The 
memory attributes (private or shared) for a given GPA that KVM records 
may not match the type of the faulting GPA.  E.g., the TDX guest can 
explicitly convert memory type from private to shared or the opposite. 
In this case KVM will exit to userspace to handle (e.g., change to the 
new memory attributes, issue the memory conversion and go back to 
guest).  After KVM determines the faulting type is legal and can 
proceed, it sets up the actual mapping, using TDX-specific ops for 
private one.

The common KVM fault handler uses the PFERR_PRIVATE_ACCESS bit of the 
error code to tell whether a faulting GPA is private.  Check the 
faulting GPA for TDX and convert it to the PFERR_PRIVATE_ACCESS so the 
common code can handle.

The specific operations to setup private mapping when the faulting GPA 
is private will follow in future patches.
Sean Christopherson Sept. 12, 2024, 1:58 p.m. UTC | #6 
On Thu, Sep 12, 2024, Kai Huang wrote:
> > +static inline bool kvm_is_private_gpa(struct kvm *kvm, gpa_t gpa)
> > +{
> > +	/* For TDX the direct mask is the shared mask. */
> > +	return !kvm_is_addr_direct(kvm, gpa);
> > +}
> 
> Does this get used in any other places?  If no I think we can open code this
> in the __vmx_handle_ept_violation().
> 
> The reason is I think the name kvm_is_private_gpa() is too generic and this
> is in the header file.

+1, kvm_is_private_gpa() is much too generic.  I knew what the code was *supposed*
to do, but had to look at the implementation to verify that's actually what it did.

> E.g., one can come up with another kvm_is_private_gpa() checking the memory
> attributes to tell whether a GPA is private.
> 
> Or we rename it to something like
> 
> 	__vmx_is_faulting_gpa_private()
> ?
> 
> Which clearly says it is checking the *faulting* GPA.

I don't think that necessarily solves the problem either, because the reader has
to know that the KVM looks at the shared bit.

If open coding is undesirable, maybe a very literal name, e.g. vmx_is_shared_bit_set()?
Edgecombe, Rick P Sept. 12, 2024, 2:43 p.m. UTC | #7 
On Thu, 2024-09-12 at 06:58 -0700, Sean Christopherson wrote:
> > Which clearly says it is checking the *faulting* GPA.
> 
> I don't think that necessarily solves the problem either, because the reader
> has
> to know that the KVM looks at the shared bit.
> 
> If open coding is undesirable

Yea, I think it's used in enough places that a helper is worth it.

> , maybe a very literal name, e.g. vmx_is_shared_bit_set()?

Sure, thanks.
Paolo Bonzini Sept. 12, 2024, 2:46 p.m. UTC | #8 
On Thu, Sep 12, 2024 at 4:43 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Thu, 2024-09-12 at 06:58 -0700, Sean Christopherson wrote:
> > > Which clearly says it is checking the *faulting* GPA.
> >
> > I don't think that necessarily solves the problem either, because the reader
> > has
> > to know that the KVM looks at the shared bit.
> >
> > If open coding is undesirable
>
> Yea, I think it's used in enough places that a helper is worth it.
>
> > , maybe a very literal name, e.g. vmx_is_shared_bit_set()?
>
> Sure, thanks.

I didn't see a problem with kvm_is_private_gpa(), but I do prefer
something that has vmx_ or vt_ in the name after seeing it. My
preference would go to something like vt_is_tdx_private_gpa(), but I'm
not going to force one name or another.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
index 78ae39b6cdcd..10aa12d45097 100644
--- a/arch/x86/kvm/vmx/common.h
+++ b/arch/x86/kvm/vmx/common.h
@@ -6,6 +6,12 @@ 
 
 #include "mmu.h"
 
+static inline bool kvm_is_private_gpa(struct kvm *kvm, gpa_t gpa)
+{
+	/* For TDX the direct mask is the shared mask. */
+	return !kvm_is_addr_direct(kvm, gpa);
+}
+
 static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
 					     unsigned long exit_qualification)
 {
@@ -28,6 +34,13 @@  static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
 		error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) ?
 			      PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
 
+	/*
+	 * Don't rely on GFN's attribute tracking xarray to prevent EPT violation
+	 * loops.
+	 */
+	if (kvm_is_private_gpa(vcpu->kvm, gpa))
+		error_code |= PFERR_PRIVATE_ACCESS;
+
 	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
 }