| Message ID | 20230310125718.1442088-4-robert.hu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] KVM: VMX: Rename vmx_umip_emulated() to cpu_has_vmx_desc() | expand |
On Fri, Mar 10, 2023, Robert Hoo wrote: > Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that > we have reg cache layer and defined this wrapper. kvm_read_cr4_bits() predates this code by ~7 years. > Although, effectively for CR4.UMIP, it's the same, at present, as it's not > guest owned, in case of future changes, here better to use the canonical > interface. Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation. I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog. This would also be a good time to opportunistically convert the WARN_ON() to a WARN_ON_ONCE() (when it fires, it fires a _lot). This, with a reworded changelog? /* * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this * and other code needs to be updated if UMIP can be guest owned. */ BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP); WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP)); return kvm_emulate_instruction(vcpu, 0);
On Sat, Mar 11, 2023, Robert Hoo wrote: > Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道： > > Practically speaking, UMIP _can't_ be guest owned without breaking UMIP > emulation. > > Agree. > Can we expect that in the future (not near future) that emulating UMIP > isn't needed at all? I mean, when all x86 CPUs on the market have UMIP in native. Heh, define "all x86s on the market". UMIP didn't come along until Broadwell, and we still get bug reports for Core2. It's going to be a long, long time before KVM can require UMIP support on Intel CPUs.
Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道： > > On Fri, Mar 10, 2023, Robert Hoo wrote: > > Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that > > we have reg cache layer and defined this wrapper. > > kvm_read_cr4_bits() predates this code by ~7 years. > > > Although, effectively for CR4.UMIP, it's the same, at present, as it's not > > guest owned, in case of future changes, here better to use the canonical > > interface. > > Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation. > I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog. > > This would also be a good time to opportunistically convert the WARN_ON() to a > WARN_ON_ONCE() (when it fires, it fires a _lot). > > This, with a reworded changelog? > > /* > * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this > * and other code needs to be updated if UMIP can be guest owned. > */ > BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP); > > WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP)); > return kvm_emulate_instruction(vcpu, 0); Are you going to have this along with your "[PATCH] KVM: VMX: Treat UMIP as emulated if and only if the host doesn't have UMIP"?
On Fri, Mar 31, 2023, Robert Hoo wrote: > Sean Christopherson <seanjc@google.com> 于2023年3月11日周六 00:27写道： > > > > On Fri, Mar 10, 2023, Robert Hoo wrote: > > > Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that > > > we have reg cache layer and defined this wrapper. > > > > kvm_read_cr4_bits() predates this code by ~7 years. > > > > > Although, effectively for CR4.UMIP, it's the same, at present, as it's not > > > guest owned, in case of future changes, here better to use the canonical > > > interface. > > > > Practically speaking, UMIP _can't_ be guest owned without breaking UMIP emulation. > > I do like not open coding vcpu->arch.cr4, but I don't particuarly like the changelog. > > > > This would also be a good time to opportunistically convert the WARN_ON() to a > > WARN_ON_ONCE() (when it fires, it fires a _lot). > > > > This, with a reworded changelog? > > > > /* > > * UMIP emulation relies on intercepting writes to CR4.UMIP, i.e. this > > * and other code needs to be updated if UMIP can be guest owned. > > */ > > BUILD_BUG_ON(KVM_POSSIBLE_CR4_GUEST_BITS & X86_CR4_UMIP); > > > > WARN_ON_ONCE(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP)); > > return kvm_emulate_instruction(vcpu, 0); > > Are you going to have this along with your "[PATCH] KVM: VMX: Treat > UMIP as emulated if and only if the host doesn't have UMIP"? Sure, I'll add a patch for that.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index bec5792acffe..8853853def56 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5435,7 +5435,7 @@ static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val) static int handle_desc(struct kvm_vcpu *vcpu) { - WARN_ON(!(vcpu->arch.cr4 & X86_CR4_UMIP)); + WARN_ON(!kvm_read_cr4_bits(vcpu, X86_CR4_UMIP)); return kvm_emulate_instruction(vcpu, 0); }
Use kvm_read_cr4_bits() rather than directly read vcpu->arch.cr4, now that we have reg cache layer and defined this wrapper. Although, effectively for CR4.UMIP, it's the same, at present, as it's not guest owned, in case of future changes, here better to use the canonical interface. Signed-off-by: Robert Hoo <robert.hu@intel.com> --- arch/x86/kvm/vmx/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)