diff mbox series

[RFC,v7,04/64] KVM: x86: Add 'fault_is_private' x86 op

Message ID 20221214194056.161492-5-michael.roth@amd.com (mailing list archive)
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand

Commit Message

Michael Roth Dec. 14, 2022, 7:39 p.m. UTC
This callback is used by the KVM MMU to check whether a #NPF was
or a private GPA or not.

Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/mmu/mmu.c             |  3 +--
 arch/x86/kvm/mmu/mmu_internal.h    | 40 +++++++++++++++++++++++++++---
 4 files changed, 39 insertions(+), 6 deletions(-)

Comments

Borislav Petkov Dec. 29, 2022, 4:14 p.m. UTC | #1
On Wed, Dec 14, 2022 at 01:39:56PM -0600, Michael Roth wrote:
> This callback is used by the KVM MMU to check whether a #NPF was
> or a private GPA or not.

s/or //

> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h |  1 +
>  arch/x86/include/asm/kvm_host.h    |  1 +
>  arch/x86/kvm/mmu/mmu.c             |  3 +--
>  arch/x86/kvm/mmu/mmu_internal.h    | 40 +++++++++++++++++++++++++++---
>  4 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index f530a550c092..efae987cdce0 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -132,6 +132,7 @@ KVM_X86_OP(complete_emulated_msr)
>  KVM_X86_OP(vcpu_deliver_sipi_vector)
>  KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
>  KVM_X86_OP_OPTIONAL_RET0(private_mem_enabled);
> +KVM_X86_OP_OPTIONAL_RET0(fault_is_private);
>  
>  #undef KVM_X86_OP
>  #undef KVM_X86_OP_OPTIONAL
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9317abffbf68..92539708f062 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1636,6 +1636,7 @@ struct kvm_x86_ops {
>  	void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
>  			     int root_level);
>  	int (*private_mem_enabled)(struct kvm *kvm);
> +	int (*fault_is_private)(struct kvm *kvm, gpa_t gpa, u64 error_code, bool *private_fault);

bool

and then you don't need the silly "== 1" at the call site.

>  
>  	bool (*has_wbinvd_exit)(void);

...

> @@ -261,13 +293,13 @@ enum {
>  };
>  
>  static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> -					u32 err, bool prefetch)
> +					u64 err, bool prefetch)

The u32 -> u64 change of err could use a sentence or two of
clarification in the commit message...

>  {
>  	bool is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault);
>  
>  	struct kvm_page_fault fault = {
>  		.addr = cr2_or_gpa,
> -		.error_code = err,
> +		.error_code = lower_32_bits(err),
>  		.exec = err & PFERR_FETCH_MASK,
>  		.write = err & PFERR_WRITE_MASK,
>  		.present = err & PFERR_PRESENT_MASK,
> @@ -281,8 +313,8 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
>  		.req_level = PG_LEVEL_4K,
>  		.goal_level = PG_LEVEL_4K,
> -		.is_private = IS_ENABLED(CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING) && is_tdp &&
> -				kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
> +		.is_private = is_tdp && kvm_mmu_fault_is_private(vcpu->kvm,
> +								 cr2_or_gpa, err),
>  	};
>  	int r;
>  
> -- 
> 2.25.1
>
Michael Roth Jan. 5, 2023, 2:42 a.m. UTC | #2
On Thu, Dec 29, 2022 at 05:14:03PM +0100, Borislav Petkov wrote:
> On Wed, Dec 14, 2022 at 01:39:56PM -0600, Michael Roth wrote:
> > This callback is used by the KVM MMU to check whether a #NPF was
> > or a private GPA or not.
> 
> s/or //
> 
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  arch/x86/include/asm/kvm-x86-ops.h |  1 +
> >  arch/x86/include/asm/kvm_host.h    |  1 +
> >  arch/x86/kvm/mmu/mmu.c             |  3 +--
> >  arch/x86/kvm/mmu/mmu_internal.h    | 40 +++++++++++++++++++++++++++---
> >  4 files changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > index f530a550c092..efae987cdce0 100644
> > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > @@ -132,6 +132,7 @@ KVM_X86_OP(complete_emulated_msr)
> >  KVM_X86_OP(vcpu_deliver_sipi_vector)
> >  KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
> >  KVM_X86_OP_OPTIONAL_RET0(private_mem_enabled);
> > +KVM_X86_OP_OPTIONAL_RET0(fault_is_private);
> >  
> >  #undef KVM_X86_OP
> >  #undef KVM_X86_OP_OPTIONAL
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 9317abffbf68..92539708f062 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1636,6 +1636,7 @@ struct kvm_x86_ops {
> >  	void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
> >  			     int root_level);
> >  	int (*private_mem_enabled)(struct kvm *kvm);
> > +	int (*fault_is_private)(struct kvm *kvm, gpa_t gpa, u64 error_code, bool *private_fault);
> 
> bool
> 
> and then you don't need the silly "== 1" at the call site.

Obviously I need to add some proper documentation for this, but a 1
return basically means 'private_fault' pass-by-ref arg has been set
with the appropriate value, whereas 0 means "there's no platform-specific
handling for this, so if you have some generic way to determine this
then use that instead".

This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
just parrots whatever kvm_mem_is_private() returns to support running
KVM selftests without needed hardware/platform support. If we don't
take care to skip this check where the above fault_is_private() hook
returns 1, then it ends up breaking SNP in cases where the kernel has
been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
relies on the page fault flags to make this determination, not
kvm_mem_is_private(), which normally only tracks the memory attributes
set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.

> 
> >  
> >  	bool (*has_wbinvd_exit)(void);
> 
> ...
> 
> > @@ -261,13 +293,13 @@ enum {
> >  };
> >  
> >  static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> > -					u32 err, bool prefetch)
> > +					u64 err, bool prefetch)
> 
> The u32 -> u64 change of err could use a sentence or two of
> clarification in the commit message...

Will do.

-Mike

> 
> >  {
> >  	bool is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault);
> >  
> >  	struct kvm_page_fault fault = {
> >  		.addr = cr2_or_gpa,
> > -		.error_code = err,
> > +		.error_code = lower_32_bits(err),
> >  		.exec = err & PFERR_FETCH_MASK,
> >  		.write = err & PFERR_WRITE_MASK,
> >  		.present = err & PFERR_PRESENT_MASK,
> > @@ -281,8 +313,8 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> >  		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
> >  		.req_level = PG_LEVEL_4K,
> >  		.goal_level = PG_LEVEL_4K,
> > -		.is_private = IS_ENABLED(CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING) && is_tdp &&
> > -				kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
> > +		.is_private = is_tdp && kvm_mmu_fault_is_private(vcpu->kvm,
> > +								 cr2_or_gpa, err),
> >  	};
> >  	int r;
> >  
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov Jan. 13, 2023, 2:34 p.m. UTC | #3
On Wed, Jan 04, 2023 at 08:42:56PM -0600, Michael Roth wrote:
> Obviously I need to add some proper documentation for this, but a 1
> return basically means 'private_fault' pass-by-ref arg has been set
> with the appropriate value, whereas 0 means "there's no platform-specific
> handling for this, so if you have some generic way to determine this
> then use that instead".

Still binary, tho, and can be bool, right?

I.e., you can just as well do:

        if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault))
                goto out;

at the call site.

> This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
> just parrots whatever kvm_mem_is_private() returns to support running
> KVM selftests without needed hardware/platform support. If we don't
> take care to skip this check where the above fault_is_private() hook
> returns 1, then it ends up breaking SNP in cases where the kernel has
> been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
> relies on the page fault flags to make this determination, not
> kvm_mem_is_private(), which normally only tracks the memory attributes
> set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.

Some of that explanation belongs into the commit message, which is a bit
lacking...
Sean Christopherson Jan. 13, 2023, 3:48 p.m. UTC | #4
On Fri, Jan 13, 2023, Borislav Petkov wrote:
> On Wed, Jan 04, 2023 at 08:42:56PM -0600, Michael Roth wrote:
> > Obviously I need to add some proper documentation for this, but a 1
> > return basically means 'private_fault' pass-by-ref arg has been set
> > with the appropriate value, whereas 0 means "there's no platform-specific
> > handling for this, so if you have some generic way to determine this
> > then use that instead".
> 
> Still binary, tho, and can be bool, right?
> 
> I.e., you can just as well do:
> 
>         if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault))
>                 goto out;
> 
> at the call site.

Ya.  Don't spend too much time trying to make this look super pretty though, there
are subtle bugs inherited from the base UPM series that need to be sorted out and
will impact this code.  E.g. invoking kvm_mem_is_private() outside of the protection
of mmu_invalidate_seq means changes to the attributes may not be reflected in the
page tables.

I'm also hoping we can avoid a callback entirely, though that may prove to be
more pain than gain.  I'm poking at the UPM and testing series right now, will
circle back to this and TDX in a few weeks to see if there's a sane way to communicate
shared vs. private without having to resort to a callback, and without having
races between page faults, KVM_SET_MEMORY_ATTRIBUTES, and KVM_SET_USER_MEMORY_REGION2.

> > This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
> > just parrots whatever kvm_mem_is_private() returns to support running
> > KVM selftests without needed hardware/platform support. If we don't
> > take care to skip this check where the above fault_is_private() hook
> > returns 1, then it ends up breaking SNP in cases where the kernel has
> > been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
> > relies on the page fault flags to make this determination, not
> > kvm_mem_is_private(), which normally only tracks the memory attributes
> > set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.
> 
> Some of that explanation belongs into the commit message, which is a bit
> lacking...

I'll circle back to this too when I give this series (and TDX) a proper look,
there's got too be a better way to handle this.
Borislav Petkov Jan. 13, 2023, 6:45 p.m. UTC | #5
On Fri, Jan 13, 2023 at 03:48:59PM +0000, Sean Christopherson wrote:
> Ya.  Don't spend too much time trying to make this look super pretty though, there
> are subtle bugs inherited from the base UPM series that need to be sorted out and
> will impact this code.

Yeah, I'm simply trying to find my way around the code and no better way than
reviewing it. But thanks for the heads up.

> I'll circle back to this too when I give this series (and TDX) a proper look,
> there's got too be a better way to handle this.

Good.

Thx.
Michael Roth Feb. 20, 2023, 4:22 p.m. UTC | #6
On Fri, Jan 13, 2023 at 03:48:59PM +0000, Sean Christopherson wrote:
> On Fri, Jan 13, 2023, Borislav Petkov wrote:
> > On Wed, Jan 04, 2023 at 08:42:56PM -0600, Michael Roth wrote:
> > > Obviously I need to add some proper documentation for this, but a 1
> > > return basically means 'private_fault' pass-by-ref arg has been set
> > > with the appropriate value, whereas 0 means "there's no platform-specific
> > > handling for this, so if you have some generic way to determine this
> > > then use that instead".
> > 
> > Still binary, tho, and can be bool, right?
> > 
> > I.e., you can just as well do:
> > 
> >         if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault))
> >                 goto out;
> > 
> > at the call site.
> 
> Ya.  Don't spend too much time trying to make this look super pretty though, there
> are subtle bugs inherited from the base UPM series that need to be sorted out and
> will impact this code.  E.g. invoking kvm_mem_is_private() outside of the protection
> of mmu_invalidate_seq means changes to the attributes may not be reflected in the
> page tables.
> 
> I'm also hoping we can avoid a callback entirely, though that may prove to be
> more pain than gain.  I'm poking at the UPM and testing series right now, will
> circle back to this and TDX in a few weeks to see if there's a sane way to communicate
> shared vs. private without having to resort to a callback, and without having
> races between page faults, KVM_SET_MEMORY_ATTRIBUTES, and KVM_SET_USER_MEMORY_REGION2.

Can circle back on this, but for v8 at least I've kept the callback, but
simplified SVM implementation of it so that it's only needed for SNP. For
protected-SEV it will fall through to the same generic handling used by UPM
self-tests.

It seems like it's safe to have a callback of that sort here for TDX/SNP (or
whatever we end up replacing the callback with), since the #NPF flags
themselves won't change based on attribute updates, and the subsequent
comparison to kvm_mem_is_private() will happen after mmu_invalidate_seq
is logged.

But for protected-SEV and UPM selftests the initial kvm_mem_is_private()
can become stale vs. the one in __kvm_faultin_pfn(), but it seems like ATM
it would only lead to a spurious KVM_EXIT_MEMORY_FAULT, which SEV at least
should treat at an implicit page-state change and be able to recover from.
But yah, not ideal, and maybe for self-tests that makes it difficult to tell
if things are working as expected or not.

Maybe we should just skip setting fault->is_private here in the
non-TDX/non-SNP cases, and just have some other indicator so it's
initialized/ignored in kvm_mem_is_private() later. I think some iterations
of UPM did it this way prior to 'is_private' becoming const.

> 
> > > This is mainly to handle CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, which
> > > just parrots whatever kvm_mem_is_private() returns to support running
> > > KVM selftests without needed hardware/platform support. If we don't
> > > take care to skip this check where the above fault_is_private() hook
> > > returns 1, then it ends up breaking SNP in cases where the kernel has
> > > been compiled with CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING, since SNP
> > > relies on the page fault flags to make this determination, not
> > > kvm_mem_is_private(), which normally only tracks the memory attributes
> > > set by userspace via KVM_SET_MEMORY_ATTRIBUTES ioctl.
> > 
> > Some of that explanation belongs into the commit message, which is a bit
> > lacking...
> 
> I'll circle back to this too when I give this series (and TDX) a proper look,
> there's got too be a better way to handle this.
> 

It seems like for SNP/TDX we just need to register the shared/encrypted
bits with KVM MMU and let it handle checking the #NPF flags, but can
iterate on that for the next spin when we have a better idea what it
should look like.

-Mike
diff mbox series

Patch

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index f530a550c092..efae987cdce0 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -132,6 +132,7 @@  KVM_X86_OP(complete_emulated_msr)
 KVM_X86_OP(vcpu_deliver_sipi_vector)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
 KVM_X86_OP_OPTIONAL_RET0(private_mem_enabled);
+KVM_X86_OP_OPTIONAL_RET0(fault_is_private);
 
 #undef KVM_X86_OP
 #undef KVM_X86_OP_OPTIONAL
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9317abffbf68..92539708f062 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1636,6 +1636,7 @@  struct kvm_x86_ops {
 	void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
 			     int root_level);
 	int (*private_mem_enabled)(struct kvm *kvm);
+	int (*fault_is_private)(struct kvm *kvm, gpa_t gpa, u64 error_code, bool *private_fault);
 
 	bool (*has_wbinvd_exit)(void);
 
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index b3ffc61c668c..61a7c221b966 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5646,8 +5646,7 @@  int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	}
 
 	if (r == RET_PF_INVALID) {
-		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
-					  lower_32_bits(error_code), false);
+		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa, error_code, false);
 		if (KVM_BUG_ON(r == RET_PF_INVALID, vcpu->kvm))
 			return -EIO;
 	}
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
index e2f508db0b6e..04ea8da86510 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -230,6 +230,38 @@  struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (!kvm_slot_can_be_private(slot)) {
+		pr_debug("%s: slot is not private, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (static_call(kvm_x86_fault_is_private)(kvm, gpa, err, &private_fault) == 1)
+		goto out;
+
+	/*
+	 * Handling below is for UPM self-tests and guests that use
+	 * slot->shared_bitmap for encrypted access tracking.
+	 */
+	if (IS_ENABLED(CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING))
+		private_fault = kvm_mem_is_private(kvm, gpa >> PAGE_SHIFT);
+
+out:
+	pr_debug("%s: GFN: 0x%llx, private: %d\n", __func__, gfn, private_fault);
+	return private_fault;
+}
+
 /*
  * Return values of handle_mmio_page_fault(), mmu.page_fault(), fast_page_fault(),
  * and of course kvm_mmu_do_page_fault().
@@ -261,13 +293,13 @@  enum {
 };
 
 static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
-					u32 err, bool prefetch)
+					u64 err, bool prefetch)
 {
 	bool is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault);
 
 	struct kvm_page_fault fault = {
 		.addr = cr2_or_gpa,
-		.error_code = err,
+		.error_code = lower_32_bits(err),
 		.exec = err & PFERR_FETCH_MASK,
 		.write = err & PFERR_WRITE_MASK,
 		.present = err & PFERR_PRESENT_MASK,
@@ -281,8 +313,8 @@  static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
 		.req_level = PG_LEVEL_4K,
 		.goal_level = PG_LEVEL_4K,
-		.is_private = IS_ENABLED(CONFIG_HAVE_KVM_PRIVATE_MEM_TESTING) && is_tdp &&
-				kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
+		.is_private = is_tdp && kvm_mmu_fault_is_private(vcpu->kvm,
+								 cr2_or_gpa, err),
 	};
 	int r;