[PATCH] KVM: x86: Always set kvm_run->if_flag

[PATCH] KVM: x86: Always set kvm_run->if_flag

[Marc Orr]

The kvm_run struct's if_flag is apart 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 regiesters (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>
---
 arch/x86/include/asm/kvm-x86-ops.h | 1 +
 arch/x86/include/asm/kvm_host.h    | 1 +
 arch/x86/kvm/svm/svm.c             | 8 ++++++++
 arch/x86/kvm/vmx/vmx.c             | 6 ++++++
 arch/x86/kvm/x86.c                 | 9 +--------
 5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
+}
+
 static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 {
 	switch (reg) {
@@ -4621,6 +4628,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..6056baa13977 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.400.ga245620fadb-goog

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Jim Mattson]

On Mon, Dec 6, 2021 at 8:31 PM Marc Orr <marcorr@google.com> wrote:
>
> The kvm_run struct's if_flag is apart of the userspace/kernel API. The

Typo: 'a part'.

> 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 regiesters (i.e., with guest_state_protected set).

Typo: 'registers'.

> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> Signed-off-by: Marc Orr <marcorr@google.com>
> ---

> -       /*
> -        * 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);

I'm sorry that I missed that change when it first went by. Maintaining
backwards compatibility with existing userspace code is a fundamental
tenet of Linux kernel development.

Acked-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Tom Lendacky]

On 12/6/21 10:31 PM, Marc Orr wrote:
> The kvm_run struct's if_flag is apart 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 regiesters (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>
> ---
>   arch/x86/include/asm/kvm-x86-ops.h | 1 +
>   arch/x86/include/asm/kvm_host.h    | 1 +
>   arch/x86/kvm/svm/svm.c             | 8 ++++++++
>   arch/x86/kvm/vmx/vmx.c             | 6 ++++++
>   arch/x86/kvm/x86.c                 | 9 +--------
>   5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);

I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
the better thing would be:

	return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
				       : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;

(Since this function returns a bool, I don't think you need the !!)

Thanks,
Tom

> +}
> +
>   static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
>   {
>   	switch (reg) {
> @@ -4621,6 +4628,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..6056baa13977 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);
>   
> 

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Marc Orr]

On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 12/6/21 10:31 PM, Marc Orr wrote:
> > The kvm_run struct's if_flag is apart 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 regiesters (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>
> > ---
> >   arch/x86/include/asm/kvm-x86-ops.h | 1 +
> >   arch/x86/include/asm/kvm_host.h    | 1 +
> >   arch/x86/kvm/svm/svm.c             | 8 ++++++++
> >   arch/x86/kvm/vmx/vmx.c             | 6 ++++++
> >   arch/x86/kvm/x86.c                 | 9 +--------
> >   5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
>
> I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
> the better thing would be:
>
>         return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
>                                        : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
>
> (Since this function returns a bool, I don't think you need the !!)

I had the same reservations when writing the patch. (Why fix what's
not broken.) The reason I wrote the patch this way is based on what I
read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
Value of the RFLAGS.IF bit for the guest."

Also, I had _thought_ that `svm_interrupt_allowed()` -- the
AMD-specific function used to populate `ready_for_interrupt_injection`
-- was relying on `GUEST_INTERRUPT_MASK`. But now I'm reading the code
again, and I realized I was overly focused on the SEV-ES handling.
That code is actually extracting the IF bit from the RFLAGS register
in the same way you've proposed here.

Changing the patch as you've suggested SGTM. I can send out a v2. I'll
wait a day or two to see if there are any other comments first. I
guess the alternative would be to change `svm_interrupt_blocked()` to
solely use the `SVM_GUEST_INTERRUPT_MASK`. If we were confident that
it was sufficient, it would be a nice little cleanup. But regardless,
I think we should keep the code introduced by this patch consistent
with `svm_interrupt_blocked()`.

Also, noted on the `!!` not being needed when returning from a bool
function. I'll keep this in mind in the future. Thanks!

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Maxim Levitsky]

On Tue, 2021-12-07 at 07:14 -0800, Marc Orr wrote:
> On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > On 12/6/21 10:31 PM, Marc Orr wrote:
> > > The kvm_run struct's if_flag is apart 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 regiesters (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>
> > > ---
> > >   arch/x86/include/asm/kvm-x86-ops.h | 1 +
> > >   arch/x86/include/asm/kvm_host.h    | 1 +
> > >   arch/x86/kvm/svm/svm.c             | 8 ++++++++
> > >   arch/x86/kvm/vmx/vmx.c             | 6 ++++++
> > >   arch/x86/kvm/x86.c                 | 9 +--------
> > >   5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
> > 
> > I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
> > the better thing would be:

I also noticed long ago that SVM_GUEST_INTERRUPT_MASK seems to duplicate the EFLAGS.IF value,
it seems to have no other purpose.

Best regards,
	Maxim Levitsky

> > 
> >         return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
> >                                        : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
> > 
> > (Since this function returns a bool, I don't think you need the !!)
> 
> I had the same reservations when writing the patch. (Why fix what's
> not broken.) The reason I wrote the patch this way is based on what I
> read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
> Value of the RFLAGS.IF bit for the guest."
> 
> Also, I had _thought_ that `svm_interrupt_allowed()` -- the
> AMD-specific function used to populate `ready_for_interrupt_injection`
> -- was relying on `GUEST_INTERRUPT_MASK`. But now I'm reading the code
> again, and I realized I was overly focused on the SEV-ES handling.
> That code is actually extracting the IF bit from the RFLAGS register
> in the same way you've proposed here.
> 
> Changing the patch as you've suggested SGTM. I can send out a v2. I'll
> wait a day or two to see if there are any other comments first. I
> guess the alternative would be to change `svm_interrupt_blocked()` to
> solely use the `SVM_GUEST_INTERRUPT_MASK`. If we were confident that
> it was sufficient, it would be a nice little cleanup. But regardless,
> I think we should keep the code introduced by this patch consistent
> with `svm_interrupt_blocked()`.
> 
> Also, noted on the `!!` not being needed when returning from a bool
> function. I'll keep this in mind in the future. Thanks!
> 

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Tom Lendacky]

On 12/7/21 9:14 AM, Marc Orr wrote:
> On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 12/6/21 10:31 PM, Marc Orr wrote:
>>> The kvm_run struct's if_flag is apart 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 regiesters (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>
>>> ---
>>>    arch/x86/include/asm/kvm-x86-ops.h | 1 +
>>>    arch/x86/include/asm/kvm_host.h    | 1 +
>>>    arch/x86/kvm/svm/svm.c             | 8 ++++++++
>>>    arch/x86/kvm/vmx/vmx.c             | 6 ++++++
>>>    arch/x86/kvm/x86.c                 | 9 +--------
>>>    5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
>>
>> I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
>> the better thing would be:
>>
>>          return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
>>                                         : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
>>
>> (Since this function returns a bool, I don't think you need the !!)
> 
> I had the same reservations when writing the patch. (Why fix what's
> not broken.) The reason I wrote the patch this way is based on what I
> read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
> Value of the RFLAGS.IF bit for the guest."

I just verified with the hardware team that this flag is indeed only set 
for a guest with protected state (SEV-ES / SEV-SNP). An update to the APM 
will be made.

Thanks,
Tom

> 
> Also, I had _thought_ that `svm_interrupt_allowed()` -- the
> AMD-specific function used to populate `ready_for_interrupt_injection`
> -- was relying on `GUEST_INTERRUPT_MASK`. But now I'm reading the code
> again, and I realized I was overly focused on the SEV-ES handling.
> That code is actually extracting the IF bit from the RFLAGS register
> in the same way you've proposed here.
> 
> Changing the patch as you've suggested SGTM. I can send out a v2. I'll
> wait a day or two to see if there are any other comments first. I
> guess the alternative would be to change `svm_interrupt_blocked()` to
> solely use the `SVM_GUEST_INTERRUPT_MASK`. If we were confident that
> it was sufficient, it would be a nice little cleanup. But regardless,
> I think we should keep the code introduced by this patch consistent
> with `svm_interrupt_blocked()`.
> 
> Also, noted on the `!!` not being needed when returning from a bool
> function. I'll keep this in mind in the future. Thanks!
> 

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Sean Christopherson]

On Tue, Dec 07, 2021, Tom Lendacky wrote:
> On 12/7/21 9:14 AM, Marc Orr wrote:
> > On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > > +static bool svm_get_if_flag(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +     struct vmcb *vmcb = to_svm(vcpu)->vmcb;
> > > > +
> > > > +     return !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
> > > 
> > > I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
> > > the better thing would be:
> > > 
> > >          return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
> > >                                         : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
> > > 
> > > (Since this function returns a bool, I don't think you need the !!)
> > 
> > I had the same reservations when writing the patch. (Why fix what's
> > not broken.) The reason I wrote the patch this way is based on what I
> > read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
> > Value of the RFLAGS.IF bit for the guest."
> 
> I just verified with the hardware team that this flag is indeed only set for
> a guest with protected state (SEV-ES / SEV-SNP). An update to the APM will
> be made.

svm_interrupt_blocked() should be modified to use the new svm_get_if_flag()
helper so that the SEV-{ES,SN} behavior is contained in a single location, e.g.

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 208566f63bce..fef04e9fa9c9 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3583,14 +3583,10 @@ 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))
+       if (!is_guest_mode(vcpu)) {
+               if (!svm_get_if_flag(vcpu))
                        return true;
-       } else if (is_guest_mode(vcpu)) {
+       } else {
                /* 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)
@@ -3600,9 +3596,6 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
                /* ... vmexits aren't blocked by the interrupt shadow  */
                if (nested_exit_on_intr(svm))
                        return false;
-       } else {
-               if (!(kvm_get_rflags(vcpu) & X86_EFLAGS_IF))
-                       return true;
        }

        return (vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK);

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Marc Orr]

On Tue, Dec 7, 2021 at 8:34 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Dec 07, 2021, Tom Lendacky wrote:
> > On 12/7/21 9:14 AM, Marc Orr wrote:
> > > On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > > > +static bool svm_get_if_flag(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +     struct vmcb *vmcb = to_svm(vcpu)->vmcb;
> > > > > +
> > > > > +     return !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
> > > >
> > > > I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
> > > > the better thing would be:
> > > >
> > > >          return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
> > > >                                         : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
> > > >
> > > > (Since this function returns a bool, I don't think you need the !!)
> > >
> > > I had the same reservations when writing the patch. (Why fix what's
> > > not broken.) The reason I wrote the patch this way is based on what I
> > > read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
> > > Value of the RFLAGS.IF bit for the guest."
> >
> > I just verified with the hardware team that this flag is indeed only set for
> > a guest with protected state (SEV-ES / SEV-SNP). An update to the APM will
> > be made.
>
> svm_interrupt_blocked() should be modified to use the new svm_get_if_flag()
> helper so that the SEV-{ES,SN} behavior is contained in a single location, e.g.
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 208566f63bce..fef04e9fa9c9 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3583,14 +3583,10 @@ 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))
> +       if (!is_guest_mode(vcpu)) {
> +               if (!svm_get_if_flag(vcpu))
>                         return true;
> -       } else if (is_guest_mode(vcpu)) {
> +       } else {
>                 /* 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)
> @@ -3600,9 +3596,6 @@ bool svm_interrupt_blocked(struct kvm_vcpu *vcpu)
>                 /* ... vmexits aren't blocked by the interrupt shadow  */
>                 if (nested_exit_on_intr(svm))
>                         return false;
> -       } else {
> -               if (!(kvm_get_rflags(vcpu) & X86_EFLAGS_IF))
> -                       return true;
>         }
>
>         return (vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK);

Agreed. This is a nice change. I'll incorporate it into v2. Thanks!

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Marc Orr]

On Tue, Dec 7, 2021 at 8:00 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 12/7/21 9:14 AM, Marc Orr wrote:
> > On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 12/6/21 10:31 PM, Marc Orr wrote:
> >>> The kvm_run struct's if_flag is apart 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 regiesters (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>
> >>> ---
> >>>    arch/x86/include/asm/kvm-x86-ops.h | 1 +
> >>>    arch/x86/include/asm/kvm_host.h    | 1 +
> >>>    arch/x86/kvm/svm/svm.c             | 8 ++++++++
> >>>    arch/x86/kvm/vmx/vmx.c             | 6 ++++++
> >>>    arch/x86/kvm/x86.c                 | 9 +--------
> >>>    5 files changed, 17 insertions(+), 8 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..91608f8c0cde 100644
> >>> --- a/arch/x86/kvm/svm/svm.c
> >>> +++ b/arch/x86/kvm/svm/svm.c
> >>> @@ -1585,6 +1585,13 @@ 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 !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
> >>
> >> I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
> >> the better thing would be:
> >>
> >>          return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
> >>                                         : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
> >>
> >> (Since this function returns a bool, I don't think you need the !!)
> >
> > I had the same reservations when writing the patch. (Why fix what's
> > not broken.) The reason I wrote the patch this way is based on what I
> > read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
> > Value of the RFLAGS.IF bit for the guest."
>
> I just verified with the hardware team that this flag is indeed only set
> for a guest with protected state (SEV-ES / SEV-SNP). An update to the APM
> will be made.

Got it now. Then the change you suggested is a must! Thanks, Tom.

Re: [PATCH] KVM: x86: Always set kvm_run->if_flag

[Paolo Bonzini]

On 12/7/21 18:28, Marc Orr wrote:
>>>>> +static bool svm_get_if_flag(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +     struct vmcb *vmcb = to_svm(vcpu)->vmcb;
>>>>> +
>>>>> +     return !!(vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK);
>>>> I'm not sure if this is always valid to use for non SEV-ES guests. Maybe
>>>> the better thing would be:
>>>>
>>>>           return sev_es_guest(vcpu->kvm) ? vmcb->control.int_state & SVM_GUEST_INTERRUPT_MASK
>>>>                                          : kvm_get_rflags(vcpu) & X86_EFLAGS_IF;
>>>>
>>>> (Since this function returns a bool, I don't think you need the !!)
>>>
>>> I had the same reservations when writing the patch. (Why fix what's
>>> not broken.) The reason I wrote the patch this way is based on what I
>>> read in APM vol2: Appendix B Layout of VMCB: "GUEST_INTERRUPT_MASK -
>>> Value of the RFLAGS.IF bit for the guest."
>>
>> I just verified with the hardware team that this flag is indeed only set
>> for a guest with protected state (SEV-ES / SEV-SNP). An update to the APM
>> will be made.
>
> Got it now. Then the change you suggested is a must! Thanks, Tom.

Besides, the bit wouldn't have existed on old (pre-SEV-ES) processors.

Paolo