All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] KVM: x86: Always set kvm_run->if_flag
@ 2021-12-09 15:52 Marc Orr
  2021-12-09 16:21 ` Maxim Levitsky
  2021-12-09 17:48 ` Paolo Bonzini
  0 siblings, 2 replies; 8+ messages in thread
From: Marc Orr @ 2021-12-09 15:52 UTC (permalink / raw)
  To: pbonzini, seanjc, vkuznets, wanpengli, jmattson, joro, tglx,
	mingo, bp, dave.hansen, x86, hpa, thomas.lendacky, mlevitsk, kvm,
	linux-kernel
  Cc: Marc Orr

The kvm_run struct's if_flag is a part of the userspace/kernel API. The
SEV-ES patches failed to set this flag because it's no longer needed by
QEMU (according to the comment in the source code). However, other
hypervisors may make use of this flag. Therefore, set the flag for
guests with encrypted registers (i.e., with guest_state_protected set).

Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
Signed-off-by: Marc Orr <marcorr@google.com>
---
v1 -> v2
- fix typos in commit message
- fix `svm_get_if_flag()` to work for non-SEV
- remove !! in return for both [svm|vmx]_get_if_flag()
- refactor `svm_interrupt_blocked()` to use `svm_get_if_flag()` arch/x86/include/asm/kvm-x86-ops.h |  1 +

 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/svm/svm.c             | 21 ++++++++++++---------
 arch/x86/kvm/vmx/vmx.c             |  6 ++++++
 arch/x86/kvm/x86.c                 |  9 +--------
 5 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index cefe1d81e2e8..9e50da3ed01a 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -47,6 +47,7 @@ KVM_X86_OP(set_dr7)
 KVM_X86_OP(cache_reg)
 KVM_X86_OP(get_rflags)
 KVM_X86_OP(set_rflags)
+KVM_X86_OP(get_if_flag)
 KVM_X86_OP(tlb_flush_all)
 KVM_X86_OP(tlb_flush_current)
 KVM_X86_OP_NULL(tlb_remote_flush)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 860ed500580c..a7f868ff23e7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1349,6 +1349,7 @@ struct kvm_x86_ops {
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
 	void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
+	bool (*get_if_flag)(struct kvm_vcpu *vcpu);
 
 	void (*tlb_flush_all)(struct kvm_vcpu *vcpu);
 	void (*tlb_flush_current)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d0f68d11ec70..5151efa424ac 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1585,6 +1585,15 @@ static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	to_svm(vcpu)->vmcb->save.rflags = rflags;
 }
 
+static bool svm_get_if_flag(struct kvm_vcpu *vcpu)
+{
+	struct vmcb *vmcb = to_svm(vcpu)->vmcb;
+
+	return sev_es_guest(vcpu->kvm)
+		? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
+		: kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
+}
+
 static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 {
 	switch (reg) {
@@ -3568,14 +3577,7 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
 	if (!gif_set(svm))
 		return true;
 
-	if (sev_es_guest(vcpu->kvm)) {
-		/*
-		 * SEV-ES guests to not expose RFLAGS. Use the VMCB interrupt mask
-		 * bit to determine the state of the IF flag.
-		 */
-		if (!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK))
-			return true;
-	} else if (is_guest_mode(vcpu)) {
+	if (is_guest_mode(vcpu)) {
 		/* As long as interrupts are being delivered...  */
 		if ((svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK)
 		    ? !(svm->vmcb01.ptr->save.rflags & X86_EFLAGS_IF)
@@ -3586,7 +3588,7 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
 		if (nested_exit_on_intr(svm))
 			return false;
 	} else {
-		if (!(kvm_get_rflags(vcpu) & X86_EFLAGS_IF))
+		if (!svm_get_if_flag(vcpu))
 			return true;
 	}
 
@@ -4621,6 +4623,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
 	.set_rflags = svm_set_rflags,
+	.get_if_flag = svm_get_if_flag,
 
 	.tlb_flush_all = svm_flush_tlb,
 	.tlb_flush_current = svm_flush_tlb,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9453743ce0c4..269de5bb98d7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1363,6 +1363,11 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 		vmx->emulation_required = vmx_emulation_required(vcpu);
 }
 
+static bool vmx_get_if_flag(struct kvm_vcpu *vcpu)
+{
+	return vmx_get_rflags(vcpu) & X86_EFLAGS_IF;
+}
+
 u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
 	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
@@ -7575,6 +7580,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
 	.set_rflags = vmx_set_rflags,
+	.get_if_flag = vmx_get_if_flag,
 
 	.tlb_flush_all = vmx_flush_tlb_all,
 	.tlb_flush_current = vmx_flush_tlb_current,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e0aa4dd53c7f..45e836db5bcd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8995,14 +8995,7 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *kvm_run = vcpu->run;
 
-	/*
-	 * if_flag is obsolete and useless, so do not bother
-	 * setting it for SEV-ES guests.  Userspace can just
-	 * use kvm_run->ready_for_interrupt_injection.
-	 */
-	kvm_run->if_flag = !vcpu->arch.guest_state_protected
-		&& (kvm_get_rflags(vcpu) & X86_EFLAGS_IF) != 0;
-
+	kvm_run->if_flag = static_call(kvm_x86_get_if_flag)(vcpu);
 	kvm_run->cr8 = kvm_get_cr8(vcpu);
 	kvm_run->apic_base = kvm_get_apic_base(vcpu);
 
-- 
2.34.1.173.g76aa8bc2d0-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 15:52 [PATCH v2] KVM: x86: Always set kvm_run->if_flag Marc Orr
@ 2021-12-09 16:21 ` Maxim Levitsky
  2021-12-09 17:48 ` Paolo Bonzini
  1 sibling, 0 replies; 8+ messages in thread
From: Maxim Levitsky @ 2021-12-09 16:21 UTC (permalink / raw)
  To: Marc Orr, pbonzini, seanjc, vkuznets, wanpengli, jmattson, joro,
	tglx, mingo, bp, dave.hansen, x86, hpa, thomas.lendacky, kvm,
	linux-kernel

On Thu, 2021-12-09 at 07:52 -0800, Marc Orr wrote:
> The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> SEV-ES patches failed to set this flag because it's no longer needed by
> QEMU (according to the comment in the source code). However, other
> hypervisors may make use of this flag. Therefore, set the flag for
> guests with encrypted registers (i.e., with guest_state_protected set).
> 
> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> Signed-off-by: Marc Orr <marcorr@google.com>
> ---
> v1 -> v2
> - fix typos in commit message
> - fix `svm_get_if_flag()` to work for non-SEV
> - remove !! in return for both [svm|vmx]_get_if_flag()
> - refactor `svm_interrupt_blocked()` to use `svm_get_if_flag()` arch/x86/include/asm/kvm-x86-ops.h |  1 +
> 
>  arch/x86/include/asm/kvm_host.h    |  1 +
>  arch/x86/kvm/svm/svm.c             | 21 ++++++++++++---------
>  arch/x86/kvm/vmx/vmx.c             |  6 ++++++
>  arch/x86/kvm/x86.c                 |  9 +--------
>  5 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index cefe1d81e2e8..9e50da3ed01a 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -47,6 +47,7 @@ KVM_X86_OP(set_dr7)
>  KVM_X86_OP(cache_reg)
>  KVM_X86_OP(get_rflags)
>  KVM_X86_OP(set_rflags)
> +KVM_X86_OP(get_if_flag)
>  KVM_X86_OP(tlb_flush_all)
>  KVM_X86_OP(tlb_flush_current)
>  KVM_X86_OP_NULL(tlb_remote_flush)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 860ed500580c..a7f868ff23e7 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1349,6 +1349,7 @@ struct kvm_x86_ops {
>  	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>  	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
>  	void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
> +	bool (*get_if_flag)(struct kvm_vcpu *vcpu);
>  
>  	void (*tlb_flush_all)(struct kvm_vcpu *vcpu);
>  	void (*tlb_flush_current)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index d0f68d11ec70..5151efa424ac 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1585,6 +1585,15 @@ static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>  	to_svm(vcpu)->vmcb->save.rflags = rflags;
>  }
>  
> +static bool svm_get_if_flag(struct kvm_vcpu *vcpu)
> +{
> +	struct vmcb *vmcb = to_svm(vcpu)->vmcb;
> +
> +	return sev_es_guest(vcpu->kvm)
> +		? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
> +		: kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
> +}
> +
>  static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
>  {
>  	switch (reg) {
> @@ -3568,14 +3577,7 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
>  	if (!gif_set(svm))
>  		return true;
>  
> -	if (sev_es_guest(vcpu->kvm)) {
> -		/*
> -		 * SEV-ES guests to not expose RFLAGS. Use the VMCB interrupt mask
> -		 * bit to determine the state of the IF flag.
> -		 */
> -		if (!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK))
> -			return true;
> -	} else if (is_guest_mode(vcpu)) {
> +	if (is_guest_mode(vcpu)) {
>  		/* As long as interrupts are being delivered...  */
>  		if ((svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK)
>  		    ? !(svm->vmcb01.ptr->save.rflags & X86_EFLAGS_IF)
> @@ -3586,7 +3588,7 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
>  		if (nested_exit_on_intr(svm))
>  			return false;
>  	} else {
> -		if (!(kvm_get_rflags(vcpu) & X86_EFLAGS_IF))
> +		if (!svm_get_if_flag(vcpu))
>  			return true;
>  	}
>  
> @@ -4621,6 +4623,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.cache_reg = svm_cache_reg,
>  	.get_rflags = svm_get_rflags,
>  	.set_rflags = svm_set_rflags,
> +	.get_if_flag = svm_get_if_flag,
>  
>  	.tlb_flush_all = svm_flush_tlb,
>  	.tlb_flush_current = svm_flush_tlb,
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9453743ce0c4..269de5bb98d7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1363,6 +1363,11 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>  		vmx->emulation_required = vmx_emulation_required(vcpu);
>  }
>  
> +static bool vmx_get_if_flag(struct kvm_vcpu *vcpu)
> +{
> +	return vmx_get_rflags(vcpu) & X86_EFLAGS_IF;
> +}
> +
>  u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
>  {
>  	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
> @@ -7575,6 +7580,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>  	.cache_reg = vmx_cache_reg,
>  	.get_rflags = vmx_get_rflags,
>  	.set_rflags = vmx_set_rflags,
> +	.get_if_flag = vmx_get_if_flag,
>  
>  	.tlb_flush_all = vmx_flush_tlb_all,
>  	.tlb_flush_current = vmx_flush_tlb_current,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0aa4dd53c7f..45e836db5bcd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8995,14 +8995,7 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *kvm_run = vcpu->run;
>  
> -	/*
> -	 * if_flag is obsolete and useless, so do not bother
> -	 * setting it for SEV-ES guests.  Userspace can just
> -	 * use kvm_run->ready_for_interrupt_injection.
> -	 */
> -	kvm_run->if_flag = !vcpu->arch.guest_state_protected
> -		&& (kvm_get_rflags(vcpu) & X86_EFLAGS_IF) != 0;
> -
> +	kvm_run->if_flag = static_call(kvm_x86_get_if_flag)(vcpu);
>  	kvm_run->cr8 = kvm_get_cr8(vcpu);
>  	kvm_run->apic_base = kvm_get_apic_base(vcpu);
>  

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 15:52 [PATCH v2] KVM: x86: Always set kvm_run->if_flag Marc Orr
  2021-12-09 16:21 ` Maxim Levitsky
@ 2021-12-09 17:48 ` Paolo Bonzini
  2021-12-09 18:20   ` Jim Mattson
  1 sibling, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2021-12-09 17:48 UTC (permalink / raw)
  To: Marc Orr, seanjc, vkuznets, wanpengli, jmattson, joro, tglx,
	mingo, bp, dave.hansen, x86, hpa, thomas.lendacky, mlevitsk, kvm,
	linux-kernel

On 12/9/21 16:52, Marc Orr wrote:
> The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> SEV-ES patches failed to set this flag because it's no longer needed by
> QEMU (according to the comment in the source code). However, other
> hypervisors may make use of this flag. Therefore, set the flag for
> guests with encrypted registers (i.e., with guest_state_protected set).
> 
> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> Signed-off-by: Marc Orr<marcorr@google.com>

Applied, though I wonder if it is really needed by those other VMMs 
(which? gVisor is the only one that comes to mind that is interested in 
userspace APIC).

It shouldn't be necessary for in-kernel APIC (where userspace can inject 
interrupts at any time), and ready_for_interrupt_injection is superior 
for userspace APIC.

Paolo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 17:48 ` Paolo Bonzini
@ 2021-12-09 18:20   ` Jim Mattson
  2021-12-09 18:22     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2021-12-09 18:20 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Marc Orr, seanjc, vkuznets, wanpengli, joro, tglx, mingo, bp,
	dave.hansen, x86, hpa, thomas.lendacky, mlevitsk, kvm,
	linux-kernel

On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 12/9/21 16:52, Marc Orr wrote:
> > The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> > SEV-ES patches failed to set this flag because it's no longer needed by
> > QEMU (according to the comment in the source code). However, other
> > hypervisors may make use of this flag. Therefore, set the flag for
> > guests with encrypted registers (i.e., with guest_state_protected set).
> >
> > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> > Signed-off-by: Marc Orr<marcorr@google.com>
>
> Applied, though I wonder if it is really needed by those other VMMs
> (which? gVisor is the only one that comes to mind that is interested in
> userspace APIC).

Vanadium appears to have one use of it.

> It shouldn't be necessary for in-kernel APIC (where userspace can inject
> interrupts at any time), and ready_for_interrupt_injection is superior
> for userspace APIC.

LOL. Here's that one use...

if (vcpu_run_state_->request_interrupt_window &&
vcpu_run_state_->ready_for_interrupt_injection &&
vcpu_run_state_->if_flag) {
...
}

So, maybe this is much ado about nothing?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 18:20   ` Jim Mattson
@ 2021-12-09 18:22     ` Sean Christopherson
  2021-12-09 18:27       ` Jim Mattson
  2021-12-09 18:29       ` Marc Orr
  0 siblings, 2 replies; 8+ messages in thread
From: Sean Christopherson @ 2021-12-09 18:22 UTC (permalink / raw)
  To: Jim Mattson
  Cc: Paolo Bonzini, Marc Orr, vkuznets, wanpengli, joro, tglx, mingo,
	bp, dave.hansen, x86, hpa, thomas.lendacky, mlevitsk, kvm,
	linux-kernel

On Thu, Dec 09, 2021, Jim Mattson wrote:
> On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 12/9/21 16:52, Marc Orr wrote:
> > > The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> > > SEV-ES patches failed to set this flag because it's no longer needed by
> > > QEMU (according to the comment in the source code). However, other
> > > hypervisors may make use of this flag. Therefore, set the flag for
> > > guests with encrypted registers (i.e., with guest_state_protected set).
> > >
> > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> > > Signed-off-by: Marc Orr<marcorr@google.com>
> >
> > Applied, though I wonder if it is really needed by those other VMMs
> > (which? gVisor is the only one that comes to mind that is interested in
> > userspace APIC).
> 
> Vanadium appears to have one use of it.
> 
> > It shouldn't be necessary for in-kernel APIC (where userspace can inject
> > interrupts at any time), and ready_for_interrupt_injection is superior
> > for userspace APIC.
> 
> LOL. Here's that one use...
> 
> if (vcpu_run_state_->request_interrupt_window &&
> vcpu_run_state_->ready_for_interrupt_injection &&
> vcpu_run_state_->if_flag) {
> ...
> }
> 
> So, maybe this is much ado about nothing?

I assume the issue is that SEV-ES always squishes if_flag, so that above statement
can never evaluate true.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 18:22     ` Sean Christopherson
@ 2021-12-09 18:27       ` Jim Mattson
  2021-12-09 18:29       ` Marc Orr
  1 sibling, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2021-12-09 18:27 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Marc Orr, vkuznets, wanpengli, joro, tglx, mingo,
	bp, dave.hansen, x86, hpa, thomas.lendacky, mlevitsk, kvm,
	linux-kernel

On Thu, Dec 9, 2021 at 10:22 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Dec 09, 2021, Jim Mattson wrote:
> > On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 12/9/21 16:52, Marc Orr wrote:
> > > > The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> > > > SEV-ES patches failed to set this flag because it's no longer needed by
> > > > QEMU (according to the comment in the source code). However, other
> > > > hypervisors may make use of this flag. Therefore, set the flag for
> > > > guests with encrypted registers (i.e., with guest_state_protected set).
> > > >
> > > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> > > > Signed-off-by: Marc Orr<marcorr@google.com>
> > >
> > > Applied, though I wonder if it is really needed by those other VMMs
> > > (which? gVisor is the only one that comes to mind that is interested in
> > > userspace APIC).
> >
> > Vanadium appears to have one use of it.
> >
> > > It shouldn't be necessary for in-kernel APIC (where userspace can inject
> > > interrupts at any time), and ready_for_interrupt_injection is superior
> > > for userspace APIC.
> >
> > LOL. Here's that one use...
> >
> > if (vcpu_run_state_->request_interrupt_window &&
> > vcpu_run_state_->ready_for_interrupt_injection &&
> > vcpu_run_state_->if_flag) {
> > ...
> > }
> >
> > So, maybe this is much ado about nothing?
>
> I assume the issue is that SEV-ES always squishes if_flag, so that above statement
> can never evaluate true.

If that's the only use, though, it's pretty easy to just remove that conjunct.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 18:22     ` Sean Christopherson
  2021-12-09 18:27       ` Jim Mattson
@ 2021-12-09 18:29       ` Marc Orr
  2021-12-09 18:55         ` Paolo Bonzini
  1 sibling, 1 reply; 8+ messages in thread
From: Marc Orr @ 2021-12-09 18:29 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Jim Mattson, Paolo Bonzini, vkuznets, wanpengli, joro, tglx,
	mingo, bp, dave.hansen, x86, hpa, Thomas.Lendacky, mlevitsk, kvm,
	linux-kernel

On Thu, Dec 9, 2021 at 10:22 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Dec 09, 2021, Jim Mattson wrote:
> > On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 12/9/21 16:52, Marc Orr wrote:
> > > > The kvm_run struct's if_flag is a part of the userspace/kernel API. The
> > > > SEV-ES patches failed to set this flag because it's no longer needed by
> > > > QEMU (according to the comment in the source code). However, other
> > > > hypervisors may make use of this flag. Therefore, set the flag for
> > > > guests with encrypted registers (i.e., with guest_state_protected set).
> > > >
> > > > Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> > > > Signed-off-by: Marc Orr<marcorr@google.com>
> > >
> > > Applied, though I wonder if it is really needed by those other VMMs
> > > (which? gVisor is the only one that comes to mind that is interested in
> > > userspace APIC).
> >
> > Vanadium appears to have one use of it.
> >
> > > It shouldn't be necessary for in-kernel APIC (where userspace can inject
> > > interrupts at any time), and ready_for_interrupt_injection is superior
> > > for userspace APIC.
> >
> > LOL. Here's that one use...
> >
> > if (vcpu_run_state_->request_interrupt_window &&
> > vcpu_run_state_->ready_for_interrupt_injection &&
> > vcpu_run_state_->if_flag) {
> > ...
> > }
> >
> > So, maybe this is much ado about nothing?
>
> I assume the issue is that SEV-ES always squishes if_flag, so that above statement
> can never evaluate true.

Correct. And the Vanadium code snippet above is what motivated this
patch. Technically Vanadium uses the if_flag in one other place, but I
also think that place can be replaced with the
`ready_for_interrupt_injection` flag. In fact, I had an internal patch
prepped to do this because my reading of the KVM code was identical to
Paolo's that the ready_for_interrupt_injection is superior to the
if_flag. But then after some internal discussion, we felt that the
userspace/kernel API shouldn't have been changed.

All that being said, after Jim added his Ack to this patch (which I
forgot to attach to the v2), we realized that technically the ES
patches were within their right to redefine if_flag since it's
previous semantics are maintained for non-ES VMs and ES requires
userspace changes anyway (PSP commands, guest memory pinning, etc.).

I'm OK either way here. But I assume that if this flag is giving us
pains it will give others pains. And this patch seems reasonable to
me. So all things being equal, I'd prefer to proceed with it.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] KVM: x86: Always set kvm_run->if_flag
  2021-12-09 18:29       ` Marc Orr
@ 2021-12-09 18:55         ` Paolo Bonzini
  0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2021-12-09 18:55 UTC (permalink / raw)
  To: Marc Orr, Sean Christopherson
  Cc: Jim Mattson, vkuznets, wanpengli, joro, tglx, mingo, bp,
	dave.hansen, x86, hpa, Thomas.Lendacky, mlevitsk, kvm,
	linux-kernel

On 12/9/21 19:29, Marc Orr wrote:
> All that being said, after Jim added his Ack to this patch (which I
> forgot to attach to the v2), we realized that technically the ES
> patches were within their right to redefine if_flag since it's
> previous semantics are maintained for non-ES VMs and ES requires
> userspace changes anyway (PSP commands, guest memory pinning, etc.).

Correct, but it's a bit ugly to redefine the semantics and that is why I 
am going to apply the patch anyway.

Paolo

> I'm OK either way here. But I assume that if this flag is giving us
> pains it will give others pains. And this patch seems reasonable to
> me. So all things being equal, I'd prefer to proceed with it.
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-12-09 18:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-09 15:52 [PATCH v2] KVM: x86: Always set kvm_run->if_flag Marc Orr
2021-12-09 16:21 ` Maxim Levitsky
2021-12-09 17:48 ` Paolo Bonzini
2021-12-09 18:20   ` Jim Mattson
2021-12-09 18:22     ` Sean Christopherson
2021-12-09 18:27       ` Jim Mattson
2021-12-09 18:29       ` Marc Orr
2021-12-09 18:55         ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.