On 12/7/21 9:14 AM, Marc Orr wrote:
On Tue, Dec 7, 2021 at 6:43 AM Tom Lendacky <thomas.lendacky@xxxxxxx> 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@xxxxxxxxxx>
---
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!
