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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Maxim Levitsky
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<[email protected]>
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
On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <[email protected]> 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<[email protected]>
>
> 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?
On Thu, Dec 09, 2021, Jim Mattson wrote:
> On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <[email protected]> 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<[email protected]>
> >
> > 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.
On Thu, Dec 9, 2021 at 10:22 AM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Dec 09, 2021, Jim Mattson wrote:
> > On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <[email protected]> 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<[email protected]>
> > >
> > > 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.
On Thu, Dec 9, 2021 at 10:22 AM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Dec 09, 2021, Jim Mattson wrote:
> > On Thu, Dec 9, 2021 at 9:48 AM Paolo Bonzini <[email protected]> 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<[email protected]>
> > >
> > > 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.
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.
>