| Message ID | 20180501144954.26086-1-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/05/2018 16:49, Wei Huang wrote: > The CPUID bits of OSXSAVE (function=0x1) and OSPKE (func=0x7, leaf=0x0) > allows user apps to detect if OS has set CR4.OSXSAVE or CR4.PKE. KVM is > supposed to update these CPUID bits when CR4 is updated. Current KVM > code doesn't handle some special cases when updates come from emulator. > Here is one example: > > Step 1: guest boots > Step 2: guest OS enables XSAVE ==> CR4.OSXSAVE=1 and CPUID.OSXSAVE=1 > Step 3: guest hot reboot ==> QEMU reset CR4 to 0, but CPUID.OSXAVE==1 > Step 4: guest os checks CPUID.OSXAVE, detects 1, then executes xgetbv > > Step 4 above will cause an #UD and guest crash because guest OS hasn't > turned on OSXAVE yet. This patch solves the problem by comparing the the > old_cr4 with cr4. If the related bits have been changed, > kvm_update_cpuid() needs to be called. It seems possible to write a testcase for this. You could use S3 instead of hot reboot, or you could use the new framework in tools/testing/selftests (execute mov to CR4 in guest, trigger vmexit, do KVM_SET_SREGS in host, execute CPUID in guest). Would you like to give it a shot? Paolo > Signed-off-by: Wei Huang <wei@redhat.com> > --- > arch/x86/kvm/x86.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 51ecd381793b..daeb5e2176c6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7979,6 +7979,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > { > struct msr_data apic_base_msr; > int mmu_reset_needed = 0; > + int cpuid_update_needed = 0; > int pending_vec, max_bits, idx; > struct desc_ptr dt; > int ret = -EINVAL; > @@ -8017,8 +8018,10 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > vcpu->arch.cr0 = sregs->cr0; > > mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4; > + cpuid_update_needed |= ((kvm_read_cr4(vcpu) ^ sregs->cr4) & > + (X86_CR4_OSXSAVE | X86_CR4_PKE)); > kvm_x86_ops->set_cr4(vcpu, sregs->cr4); > - if (sregs->cr4 & (X86_CR4_OSXSAVE | X86_CR4_PKE)) > + if (cpuid_update_needed) > kvm_update_cpuid(vcpu); > > idx = srcu_read_lock(&vcpu->kvm->srcu); >
On 05/02/2018 03:59 AM, Paolo Bonzini wrote: > On 01/05/2018 16:49, Wei Huang wrote: >> The CPUID bits of OSXSAVE (function=0x1) and OSPKE (func=0x7, leaf=0x0) >> allows user apps to detect if OS has set CR4.OSXSAVE or CR4.PKE. KVM is >> supposed to update these CPUID bits when CR4 is updated. Current KVM >> code doesn't handle some special cases when updates come from emulator. >> Here is one example: >> >> Step 1: guest boots >> Step 2: guest OS enables XSAVE ==> CR4.OSXSAVE=1 and CPUID.OSXSAVE=1 >> Step 3: guest hot reboot ==> QEMU reset CR4 to 0, but CPUID.OSXAVE==1 >> Step 4: guest os checks CPUID.OSXAVE, detects 1, then executes xgetbv >> >> Step 4 above will cause an #UD and guest crash because guest OS hasn't >> turned on OSXAVE yet. This patch solves the problem by comparing the the >> old_cr4 with cr4. If the related bits have been changed, >> kvm_update_cpuid() needs to be called. > > It seems possible to write a testcase for this. You could use S3 > instead of hot reboot, or you could use the new framework in > tools/testing/selftests (execute mov to CR4 in guest, trigger vmexit, do > KVM_SET_SREGS in host, execute CPUID in guest). > > Would you like to give it a shot? Yes, I can take a look at it. > > Paolo > >> Signed-off-by: Wei Huang <wei@redhat.com> >> --- >> arch/x86/kvm/x86.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 51ecd381793b..daeb5e2176c6 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -7979,6 +7979,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) >> { >> struct msr_data apic_base_msr; >> int mmu_reset_needed = 0; >> + int cpuid_update_needed = 0; >> int pending_vec, max_bits, idx; >> struct desc_ptr dt; >> int ret = -EINVAL; >> @@ -8017,8 +8018,10 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) >> vcpu->arch.cr0 = sregs->cr0; >> >> mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4; >> + cpuid_update_needed |= ((kvm_read_cr4(vcpu) ^ sregs->cr4) & >> + (X86_CR4_OSXSAVE | X86_CR4_PKE)); >> kvm_x86_ops->set_cr4(vcpu, sregs->cr4); >> - if (sregs->cr4 & (X86_CR4_OSXSAVE | X86_CR4_PKE)) >> + if (cpuid_update_needed) >> kvm_update_cpuid(vcpu); >> >> idx = srcu_read_lock(&vcpu->kvm->srcu); >> >
Wei Huang <wei@redhat.com> writes: > The CPUID bits of OSXSAVE (function=0x1) and OSPKE (func=0x7, leaf=0x0) > allows user apps to detect if OS has set CR4.OSXSAVE or CR4.PKE. KVM is > supposed to update these CPUID bits when CR4 is updated. Current KVM > code doesn't handle some special cases when updates come from emulator. > Here is one example: > > Step 1: guest boots > Step 2: guest OS enables XSAVE ==> CR4.OSXSAVE=1 and CPUID.OSXSAVE=1 > Step 3: guest hot reboot ==> QEMU reset CR4 to 0, but CPUID.OSXAVE==1 > Step 4: guest os checks CPUID.OSXAVE, detects 1, then executes xgetbv > > Step 4 above will cause an #UD and guest crash because guest OS hasn't > turned on OSXAVE yet. This patch solves the problem by comparing the the > old_cr4 with cr4. If the related bits have been changed, > kvm_update_cpuid() needs to be called. > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > arch/x86/kvm/x86.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 51ecd381793b..daeb5e2176c6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7979,6 +7979,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > { > struct msr_data apic_base_msr; > int mmu_reset_needed = 0; > + int cpuid_update_needed = 0; > int pending_vec, max_bits, idx; > struct desc_ptr dt; > int ret = -EINVAL; > @@ -8017,8 +8018,10 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > vcpu->arch.cr0 = sregs->cr0; > > mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4; > + cpuid_update_needed |= ((kvm_read_cr4(vcpu) ^ sregs->cr4) & > + (X86_CR4_OSXSAVE | X86_CR4_PKE)); > kvm_x86_ops->set_cr4(vcpu, sregs->cr4); > - if (sregs->cr4 & (X86_CR4_OSXSAVE | X86_CR4_PKE)) > + if (cpuid_update_needed) > kvm_update_cpuid(vcpu); > > idx = srcu_read_lock(&vcpu->kvm->srcu); Reviewed-by: Bandan Das <bsd@redhat.com>
2018-05-01 09:49-0500, Wei Huang: > The CPUID bits of OSXSAVE (function=0x1) and OSPKE (func=0x7, leaf=0x0) > allows user apps to detect if OS has set CR4.OSXSAVE or CR4.PKE. KVM is > supposed to update these CPUID bits when CR4 is updated. Current KVM > code doesn't handle some special cases when updates come from emulator. > Here is one example: > > Step 1: guest boots > Step 2: guest OS enables XSAVE ==> CR4.OSXSAVE=1 and CPUID.OSXSAVE=1 > Step 3: guest hot reboot ==> QEMU reset CR4 to 0, but CPUID.OSXAVE==1 > Step 4: guest os checks CPUID.OSXAVE, detects 1, then executes xgetbv > > Step 4 above will cause an #UD and guest crash because guest OS hasn't > turned on OSXAVE yet. This patch solves the problem by comparing the the > old_cr4 with cr4. If the related bits have been changed, > kvm_update_cpuid() needs to be called. > > Signed-off-by: Wei Huang <wei@redhat.com> > --- Applied to kvm/master with Cc stable, thanks.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 51ecd381793b..daeb5e2176c6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7979,6 +7979,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) { struct msr_data apic_base_msr; int mmu_reset_needed = 0; + int cpuid_update_needed = 0; int pending_vec, max_bits, idx; struct desc_ptr dt; int ret = -EINVAL; @@ -8017,8 +8018,10 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) vcpu->arch.cr0 = sregs->cr0; mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4; + cpuid_update_needed |= ((kvm_read_cr4(vcpu) ^ sregs->cr4) & + (X86_CR4_OSXSAVE | X86_CR4_PKE)); kvm_x86_ops->set_cr4(vcpu, sregs->cr4); - if (sregs->cr4 & (X86_CR4_OSXSAVE | X86_CR4_PKE)) + if (cpuid_update_needed) kvm_update_cpuid(vcpu); idx = srcu_read_lock(&vcpu->kvm->srcu);
The CPUID bits of OSXSAVE (function=0x1) and OSPKE (func=0x7, leaf=0x0) allows user apps to detect if OS has set CR4.OSXSAVE or CR4.PKE. KVM is supposed to update these CPUID bits when CR4 is updated. Current KVM code doesn't handle some special cases when updates come from emulator. Here is one example: Step 1: guest boots Step 2: guest OS enables XSAVE ==> CR4.OSXSAVE=1 and CPUID.OSXSAVE=1 Step 3: guest hot reboot ==> QEMU reset CR4 to 0, but CPUID.OSXAVE==1 Step 4: guest os checks CPUID.OSXAVE, detects 1, then executes xgetbv Step 4 above will cause an #UD and guest crash because guest OS hasn't turned on OSXAVE yet. This patch solves the problem by comparing the the old_cr4 with cr4. If the related bits have been changed, kvm_update_cpuid() needs to be called. Signed-off-by: Wei Huang <wei@redhat.com> --- arch/x86/kvm/x86.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)