[PATCH 0/3] KVM: x86: Revert dirty tracking for GPRs

[PATCH 0/3] KVM: x86: Revert dirty tracking for GPRs

[Sean Christopherson]

This is effectively belated feedback on the SEV-ES series.  My primary
interest is to revert the GPR dirty/available tracking, as it's pure
overhead for non-SEV-ES VMs, and even for SEV-ES I suspect the dirty
tracking is at best lost in the noise, and possibly even a net negative.

My original plan was to submit patches 1+3 as patch 1, taking a few
creative liberties with the GHCB spec to justify writing the GHCB GPRs
after every VMGEXIT.  But, since KVM is effectively writing the GHCB GPRs
on every VMRUN, I feel confident in saying that my interpretation of the
spec has already been proven correct.

The SEV-ES changes are effectively compile tested only, but unless I've
overlooked a code path, patch 1 is a nop.  Patch 3 definitely needs
testing.

Paolo, I'd really like to get patches 1 and 2 into 5.11, the code cost of
the dirty/available tracking is not trivial.

Sean Christopherson (3):
  KVM: SVM: Unconditionally sync GPRs to GHCB on VMRUN of SEV-ES guest
  KVM: x86: Revert "KVM: x86: Mark GPRs dirty when written"
  KVM: SVM: Sync GPRs to the GHCB only after VMGEXIT

 arch/x86/kvm/kvm_cache_regs.h | 51 +++++++++++++++++------------------
 arch/x86/kvm/svm/sev.c        | 14 +++++-----
 arch/x86/kvm/svm/svm.h        |  1 +
 3 files changed, 34 insertions(+), 32 deletions(-)

-- 
2.30.0.280.ga3ce27912f-goog

[PATCH 1/3] KVM: SVM: Unconditionally sync GPRs to GHCB on VMRUN of SEV-ES guest

[Sean Christopherson]

Drop the per-GPR dirty checks when synchronizing GPRs to the GHCB, the
GRPs' dirty bits are set from time zero and never cleared, i.e. will
always be seen as dirty.  The obvious alternative would be to clear
the dirty bits when appropriate, but removing the dirty checks is
desirable as it allows reverting GPR dirty+available tracking, which
adds overhead to all flavors of x86 VMs.

Note, unconditionally writing the GPRs in the GHCB is tacitly allowed
by the GHCB spec, which allows the hypervisor (or guest) to provide
unnecessary info; it's the guest's responsibility to consume only what
it needs (the hypervisor is untrusted after all).

  The guest and hypervisor can supply additional state if desired but
  must not rely on that additional state being provided.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c8ffdbc81709..ac652bc476ae 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1415,16 +1415,13 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
 	 * to be returned:
 	 *   GPRs RAX, RBX, RCX, RDX
 	 *
-	 * Copy their values to the GHCB if they are dirty.
+	 * Copy their values, even if they may not have been written during the
+	 * VM-Exit.  It's the guest's responsibility to not consume random data.
 	 */
-	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RAX))
-		ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
-	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RBX))
-		ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
-	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RCX))
-		ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
-	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RDX))
-		ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
+	ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
+	ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
+	ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
+	ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
 }
 
 static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
-- 
2.30.0.280.ga3ce27912f-goog

Re: [PATCH 1/3] KVM: SVM: Unconditionally sync GPRs to GHCB on VMRUN of SEV-ES guest

[Tom Lendacky]

On 1/22/21 5:50 PM, Sean Christopherson wrote:
> Drop the per-GPR dirty checks when synchronizing GPRs to the GHCB, the
> GRPs' dirty bits are set from time zero and never cleared, i.e. will

Ah, missed that, bad assumption on my part.

> always be seen as dirty.  The obvious alternative would be to clear
> the dirty bits when appropriate, but removing the dirty checks is
> desirable as it allows reverting GPR dirty+available tracking, which
> adds overhead to all flavors of x86 VMs.
> 
> Note, unconditionally writing the GPRs in the GHCB is tacitly allowed
> by the GHCB spec, which allows the hypervisor (or guest) to provide
> unnecessary info; it's the guest's responsibility to consume only what
> it needs (the hypervisor is untrusted after all).
> 
>   The guest and hypervisor can supply additional state if desired but
>   must not rely on that additional state being provided.

Yes, that's true.

I'm ok with removing the tracking if that's desired. Otherwise, we can add
a vcpu->arch.regs_dirty = 0 in sev_es_sync_from_ghcb().

Thanks,
Tom

> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/sev.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c8ffdbc81709..ac652bc476ae 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1415,16 +1415,13 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
>  	 * to be returned:
>  	 *   GPRs RAX, RBX, RCX, RDX
>  	 *
> -	 * Copy their values to the GHCB if they are dirty.
> +	 * Copy their values, even if they may not have been written during the
> +	 * VM-Exit.  It's the guest's responsibility to not consume random data.
>  	 */
> -	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RAX))
> -		ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
> -	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RBX))
> -		ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> -	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RCX))
> -		ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> -	if (kvm_register_is_dirty(vcpu, VCPU_REGS_RDX))
> -		ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
> +	ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
> +	ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> +	ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> +	ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
>  }
>  
>  static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> 

[PATCH 2/3] KVM: x86: Revert "KVM: x86: Mark GPRs dirty when written"

[Sean Christopherson]

Revert the dirty/available tracking of GPRs now that KVM copies the GPRs
to the GHCB on any post-VMGEXIT VMRUN, even if a GPR is not dirty.  Per
commit de3cd117ed2f ("KVM: x86: Omit caching logic for always-available
GPRs"), tracking for GPRs noticeably impacts KVM's code footprint.

This reverts commit 1c04d8c986567c27c56c05205dceadc92efb14ff.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/kvm_cache_regs.h | 51 +++++++++++++++++------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index f15bc16de07c..a889563ad02d 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -9,31 +9,6 @@
 	(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE | X86_CR4_OSFXSR  \
 	 | X86_CR4_OSXMMEXCPT | X86_CR4_PGE | X86_CR4_TSD | X86_CR4_FSGSBASE)
 
-static inline bool kvm_register_is_available(struct kvm_vcpu *vcpu,
-					     enum kvm_reg reg)
-{
-	return test_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
-}
-
-static inline bool kvm_register_is_dirty(struct kvm_vcpu *vcpu,
-					 enum kvm_reg reg)
-{
-	return test_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
-}
-
-static inline void kvm_register_mark_available(struct kvm_vcpu *vcpu,
-					       enum kvm_reg reg)
-{
-	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
-}
-
-static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu,
-					   enum kvm_reg reg)
-{
-	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
-	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
-}
-
 #define BUILD_KVM_GPR_ACCESSORS(lname, uname)				      \
 static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu *vcpu)\
 {									      \
@@ -43,7 +18,6 @@ static __always_inline void kvm_##lname##_write(struct kvm_vcpu *vcpu,	      \
 						unsigned long val)	      \
 {									      \
 	vcpu->arch.regs[VCPU_REGS_##uname] = val;			      \
-	kvm_register_mark_dirty(vcpu, VCPU_REGS_##uname);		      \
 }
 BUILD_KVM_GPR_ACCESSORS(rax, RAX)
 BUILD_KVM_GPR_ACCESSORS(rbx, RBX)
@@ -63,6 +37,31 @@ BUILD_KVM_GPR_ACCESSORS(r14, R14)
 BUILD_KVM_GPR_ACCESSORS(r15, R15)
 #endif
 
+static inline bool kvm_register_is_available(struct kvm_vcpu *vcpu,
+					     enum kvm_reg reg)
+{
+	return test_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
+}
+
+static inline bool kvm_register_is_dirty(struct kvm_vcpu *vcpu,
+					 enum kvm_reg reg)
+{
+	return test_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
+}
+
+static inline void kvm_register_mark_available(struct kvm_vcpu *vcpu,
+					       enum kvm_reg reg)
+{
+	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
+}
+
+static inline void kvm_register_mark_dirty(struct kvm_vcpu *vcpu,
+					   enum kvm_reg reg)
+{
+	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_avail);
+	__set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
+}
+
 static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu, int reg)
 {
 	if (WARN_ON_ONCE((unsigned int)reg >= NR_VCPU_REGS))
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH 3/3] KVM: SVM: Sync GPRs to the GHCB only after VMGEXIT

[Sean Christopherson]

Sync GPRs to the GHCB on VMRUN only if a sync is needed, i.e. if the
previous exit was a VMGEXIT and the guest is expecting some data back.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 15 ++++++++++-----
 arch/x86/kvm/svm/svm.h |  1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ac652bc476ae..9bd1e1650eb3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1418,10 +1418,13 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
 	 * Copy their values, even if they may not have been written during the
 	 * VM-Exit.  It's the guest's responsibility to not consume random data.
 	 */
-	ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
-	ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
-	ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
-	ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
+	if (svm->need_sync_to_ghcb) {
+		ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
+		ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
+		ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
+		ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
+		svm->need_sync_to_ghcb = false;
+	}
 }
 
 static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
@@ -1441,8 +1444,10 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 	 * VMMCALL allows the guest to provide extra registers. KVM also
 	 * expects RSI for hypercalls, so include that, too.
 	 *
-	 * Copy their values to the appropriate location if supplied.
+	 * Copy their values to the appropriate location if supplied, and
+	 * flag that a sync back to the GHCB is needed on the next VMRUN.
 	 */
+	svm->need_sync_to_ghcb = true;
 	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
 
 	vcpu->arch.regs[VCPU_REGS_RAX] = ghcb_get_rax_if_valid(ghcb);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 0fe874ae5498..4e2e5f9fbfc2 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -192,6 +192,7 @@ struct vcpu_svm {
 	u64 ghcb_sa_len;
 	bool ghcb_sa_sync;
 	bool ghcb_sa_free;
+	bool need_sync_to_ghcb;
 };
 
 struct svm_cpu_data {
-- 
2.30.0.280.ga3ce27912f-goog

Re: [PATCH 3/3] KVM: SVM: Sync GPRs to the GHCB only after VMGEXIT

[Tom Lendacky]

On 1/22/21 5:50 PM, Sean Christopherson wrote:
> Sync GPRs to the GHCB on VMRUN only if a sync is needed, i.e. if the
> previous exit was a VMGEXIT and the guest is expecting some data back.
> 

The start of sev_es_sync_to_ghcb() checks if the GHCB has been mapped, 
which only occurs on VMGEXIT, and exits early if not. And 
sev_es_sync_from_ghcb() is only called if the GHCB has been successfully 
mapped. The only thing in between is sev_es_validate_vmgexit(), which will 
terminate the VM on error. So I don't think this patch is needed.

Thanks,
Tom

> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/sev.c | 15 ++++++++++-----
>   arch/x86/kvm/svm/svm.h |  1 +
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index ac652bc476ae..9bd1e1650eb3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1418,10 +1418,13 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
>   	 * Copy their values, even if they may not have been written during the
>   	 * VM-Exit.  It's the guest's responsibility to not consume random data.
>   	 */
> -	ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
> -	ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> -	ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> -	ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
> +	if (svm->need_sync_to_ghcb) {
> +		ghcb_set_rax(ghcb, vcpu->arch.regs[VCPU_REGS_RAX]);
> +		ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> +		ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> +		ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
> +		svm->need_sync_to_ghcb = false;
> +	}
>   }
>   
>   static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> @@ -1441,8 +1444,10 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>   	 * VMMCALL allows the guest to provide extra registers. KVM also
>   	 * expects RSI for hypercalls, so include that, too.
>   	 *
> -	 * Copy their values to the appropriate location if supplied.
> +	 * Copy their values to the appropriate location if supplied, and
> +	 * flag that a sync back to the GHCB is needed on the next VMRUN.
>   	 */
> +	svm->need_sync_to_ghcb = true;
>   	memset(vcpu->arch.regs, 0, sizeof(vcpu->arch.regs));
>   
>   	vcpu->arch.regs[VCPU_REGS_RAX] = ghcb_get_rax_if_valid(ghcb);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 0fe874ae5498..4e2e5f9fbfc2 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -192,6 +192,7 @@ struct vcpu_svm {
>   	u64 ghcb_sa_len;
>   	bool ghcb_sa_sync;
>   	bool ghcb_sa_free;
> +	bool need_sync_to_ghcb;
>   };
>   
>   struct svm_cpu_data {
> 

Re: [PATCH 3/3] KVM: SVM: Sync GPRs to the GHCB only after VMGEXIT

[Sean Christopherson]

On Fri, Jan 22, 2021, Tom Lendacky wrote:
> On 1/22/21 5:50 PM, Sean Christopherson wrote:
> > Sync GPRs to the GHCB on VMRUN only if a sync is needed, i.e. if the
> > previous exit was a VMGEXIT and the guest is expecting some data back.
> > 
> 
> The start of sev_es_sync_to_ghcb() checks if the GHCB has been mapped, which
> only occurs on VMGEXIT, and exits early if not. And sev_es_sync_from_ghcb()
> is only called if the GHCB has been successfully mapped. The only thing in
> between is sev_es_validate_vmgexit(), which will terminate the VM on error.
> So I don't think this patch is needed.

Ah, nice!  Yep, this can be dropped.  Thanks!

Re: [PATCH 0/3] KVM: x86: Revert dirty tracking for GPRs

[Paolo Bonzini]

On 23/01/21 00:50, Sean Christopherson wrote:
> This is effectively belated feedback on the SEV-ES series.  My primary
> interest is to revert the GPR dirty/available tracking, as it's pure
> overhead for non-SEV-ES VMs, and even for SEV-ES I suspect the dirty
> tracking is at best lost in the noise, and possibly even a net negative.
> 
> My original plan was to submit patches 1+3 as patch 1, taking a few
> creative liberties with the GHCB spec to justify writing the GHCB GPRs
> after every VMGEXIT.  But, since KVM is effectively writing the GHCB GPRs
> on every VMRUN, I feel confident in saying that my interpretation of the
> spec has already been proven correct.
> 
> The SEV-ES changes are effectively compile tested only, but unless I've
> overlooked a code path, patch 1 is a nop.  Patch 3 definitely needs
> testing.
> 
> Paolo, I'd really like to get patches 1 and 2 into 5.11, the code cost of
> the dirty/available tracking is not trivial.
> 
> Sean Christopherson (3):
>    KVM: SVM: Unconditionally sync GPRs to GHCB on VMRUN of SEV-ES guest
>    KVM: x86: Revert "KVM: x86: Mark GPRs dirty when written"
>    KVM: SVM: Sync GPRs to the GHCB only after VMGEXIT
> 
>   arch/x86/kvm/kvm_cache_regs.h | 51 +++++++++++++++++------------------
>   arch/x86/kvm/svm/sev.c        | 14 +++++-----
>   arch/x86/kvm/svm/svm.h        |  1 +
>   3 files changed, 34 insertions(+), 32 deletions(-)
> 

Queued 1-2, thanks!  Yes, these should be in 5.11.

Paolo