Hi Michael,
(Cc'ing Lara, Vitaly and Maxim)
On 5/12/23 23:28, Michael Roth wrote:
Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
exposed a long-running bug in current KVM support for SEV-ES where the
kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
kernel, in which case EFER write traps would result in KVM eventually
seeing MSR_EFER_LMA get set and recording it in such a way that it would
be subsequently visible when accessing it via KVM_GET_SREGS/etc.
However, guests kernels currently rely on MSR_EFER_LMA getting set
automatically when MSR_EFER_LME is set and paging is enabled via
CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
MSR_EFER_LMA even though it is set internally, and when QEMU
subsequently tries to pass this EFER value back to KVM via
KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
which is now considered fatal due to the aforementioned QEMU commit.
This can be addressed by inferring the MSR_EFER_LMA bit being set when
paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
the expected bits are all present in subsequent handling on the host
side.
Ultimately, this handling will be implemented in the host kernel, but to
avoid breaking QEMU's SEV-ES support when using older host kernels, the
same handling can be done in QEMU just after fetching the register
values via KVM_GET_SREGS*. Implement that here.
Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
Cc: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
Cc: kvm@xxxxxxxxxxxxxxx
Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
This 'Fixes:' tag is misleading, since as you mentioned this commit
only exposes the issue.
Commit d499f196fe ("target/i386: Added consistency checks for EFER")
or around it seems more appropriate.
Is this feature easily testable on our CI, on a x86 runner with KVM
access?
Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
---
v2:
- Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
target/i386/kvm/kvm.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 11b8177eff..8721c1bf8f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
{
CPUX86State *env = &cpu->env;
struct kvm_sregs sregs;
+ target_ulong cr0_old;
int ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, &sregs);
@@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+ cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+ if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+ if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+ env->efer |= MSR_EFER_LMA;
+ }
+ }
/* changes to apic base and cr8/tpr are read back via kvm_arch_post_run */
x86_update_hflags(env);
@@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
{
CPUX86State *env = &cpu->env;
struct kvm_sregs2 sregs;
+ target_ulong cr0_old;
int i, ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, &sregs);
@@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+ cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+ if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+ if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+ env->efer |= MSR_EFER_LMA;
+ }
+ }
env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
