| Message ID | 20210424004645.3950558-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: vCPU RESET/INIT fixes and consolidation | expand |
> @@ -4504,7 +4505,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > > vmx->msr_ia32_umwait_control = 0; > > - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > + eax = 1; > + if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true)) > + eax = get_rdx_init_val(); > + kvm_rdx_write(vcpu, eax); Reviewed-by: Reiji Watanabe <reijiw@google.com> For RESET, I assume that rdx should be set by userspace when userspace changes CPUID.0x1.EAX. BTW, I would think having a default CPUID for CPUID.(EAX=0x1) would be better for consistency of a vCPU state for RESET. I would think it doesn't matter practically anyway though. Thanks, Reiji
On Tue, May 18, 2021, Reiji Watanabe wrote: > > @@ -4504,7 +4505,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > > > > vmx->msr_ia32_umwait_control = 0; > > > > - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > > + eax = 1; > > + if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true)) > > + eax = get_rdx_init_val(); > > + kvm_rdx_write(vcpu, eax); > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > For RESET, I assume that rdx should be set by userspace > when userspace changes CPUID.0x1.EAX. Ya, although the ideal solution is to add a proper RESET ioctl() so userspace can configure the vCPU model and then pull RESET#. > BTW, I would think having a default CPUID for CPUID.(EAX=0x1) would be better > for consistency of a vCPU state for RESET. I would think it doesn't matter > practically anyway though. Probably, but that would require defining default values for all of CPUID.0x0 and CPUID.0x1, which is a can of worms I'd rather not open. E.g. vendor info, basic feature set, APIC ID, etc... would all need default values. On the other hand, the EDX value stuffing predates CPUID, so using 0x600 isn't provably wrong, just a bit anachronistic. :-)
On Wed, May 19, 2021 at 11:47 AM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, May 18, 2021, Reiji Watanabe wrote: > > > @@ -4504,7 +4505,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > > > > > > vmx->msr_ia32_umwait_control = 0; > > > > > > - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > > > + eax = 1; > > > + if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true)) > > > + eax = get_rdx_init_val(); > > > + kvm_rdx_write(vcpu, eax); > > > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > > > For RESET, I assume that rdx should be set by userspace > > when userspace changes CPUID.0x1.EAX. > > Ya, although the ideal solution is to add a proper RESET ioctl() so userspace can > configure the vCPU model and then pull RESET#. Thank you so much for the answer ! It sounds like a good idea in terms of userspace handling as well as KVM implementation (I assume it would be something similar to KVM_ARM_VCPU_INIT). > > BTW, I would think having a default CPUID for CPUID.(EAX=0x1) would be better > > for consistency of a vCPU state for RESET. I would think it doesn't matter > > practically anyway though. > > Probably, but that would require defining default values for all of CPUID.0x0 and > CPUID.0x1, which is a can of worms I'd rather not open. E.g. vendor info, basic > feature set, APIC ID, etc... would all need default values. On the other hand, > the EDX value stuffing predates CPUID, so using 0x600 isn't provably wrong, just > a bit anachronistic. :-) I see... Then I don't think it's worth doing... Just out of curiosity, can't we simply use a vcpu_id for the APIC ID ? Also, can't we simply use the same values that KVM_GET_SUPPORTED_CPUID provides for other CPUID fields ? Thanks, Reiji
On Fri, May 21, 2021, Reiji Watanabe wrote: > On Wed, May 19, 2021 at 11:47 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Tue, May 18, 2021, Reiji Watanabe wrote: > > > BTW, I would think having a default CPUID for CPUID.(EAX=0x1) would be better > > > for consistency of a vCPU state for RESET. I would think it doesn't matter > > > practically anyway though. > > > > Probably, but that would require defining default values for all of CPUID.0x0 and > > CPUID.0x1, which is a can of worms I'd rather not open. E.g. vendor info, basic > > feature set, APIC ID, etc... would all need default values. On the other hand, > > the EDX value stuffing predates CPUID, so using 0x600 isn't provably wrong, just > > a bit anachronistic. :-) > > I see... Then I don't think it's worth doing... > Just out of curiosity, can't we simply use a vcpu_id for the APIC ID ? That would mostly work, but theoretically we could overflow the 8 bit field because max vCPUs is 288. Thanks Larrabee. commit 682f732ecf7396e9d6fe24d44738966699fae6c0 Author: Radim Krčmář <rkrcmar@redhat.com> Date: Tue Jul 12 22:09:29 2016 +0200 KVM: x86: bump MAX_VCPUS to 288 288 is in high demand because of Knights Landing CPU. We cannot set the limit to 640k, because that would be wasting space. > Also, can't we simply use the same values that KVM_GET_SUPPORTED_CPUID > provides for other CPUID fields ? Yes, that would mostly work. It's certainly possible to have a moderately sane default, but there's essentially zero benefit in doing so since practically speaking all userspace VMMs will override CPUID anyways. KVM could completely default to the host CPUID, but again, it wouldn't provide any meaningful benefit, while doing so would step on userspace's toes since KVM's approach is that KVM is "just" an accelerator, while userspace defines the CPU model, devices, etc... And it would also mean KVM has to start worrying about silly corner cases like the max vCPUs thing. That's why I say it's a can of worms :-)
> > > On Tue, May 18, 2021, Reiji Watanabe wrote: > > > > BTW, I would think having a default CPUID for CPUID.(EAX=0x1) would be better > > > > for consistency of a vCPU state for RESET. I would think it doesn't matter > > > > practically anyway though. > > > > > > Probably, but that would require defining default values for all of CPUID.0x0 and > > > CPUID.0x1, which is a can of worms I'd rather not open. E.g. vendor info, basic > > > feature set, APIC ID, etc... would all need default values. On the other hand, > > > the EDX value stuffing predates CPUID, so using 0x600 isn't provably wrong, just > > > a bit anachronistic. :-) > > > > I see... Then I don't think it's worth doing... > > Just out of curiosity, can't we simply use a vcpu_id for the APIC ID ? > > That would mostly work, but theoretically we could overflow the 8 bit field > because max vCPUs is 288. Thanks Larrabee. > > commit 682f732ecf7396e9d6fe24d44738966699fae6c0 > Author: Radim Krčmář <rkrcmar@redhat.com> > Date: Tue Jul 12 22:09:29 2016 +0200 > > KVM: x86: bump MAX_VCPUS to 288 > > 288 is in high demand because of Knights Landing CPU. > We cannot set the limit to 640k, because that would be wasting space. > > > Also, can't we simply use the same values that KVM_GET_SUPPORTED_CPUID > > provides for other CPUID fields ? > > Yes, that would mostly work. It's certainly possible to have a moderately sane > default, but there's essentially zero benefit in doing so since practically > speaking all userspace VMMs will override CPUID anyways. KVM could completely > default to the host CPUID, but again, it wouldn't provide any meaningful benefit, > while doing so would step on userspace's toes since KVM's approach is that KVM is > "just" an accelerator, while userspace defines the CPU model, devices, etc... > And it would also mean KVM has to start worrying about silly corner cases like > the max vCPUs thing. That's why I say it's a can of worms :-) Ah, I see. Thank you for the answer and the helpful information ! Regards, Reiji
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index a182cae71044..9c00d013af59 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4497,6 +4497,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) { struct vcpu_vmx *vmx = to_vmx(vcpu); struct msr_data apic_base_msr; + u32 eax, dummy; u64 cr0; vmx->rmode.vm86_active = 0; @@ -4504,7 +4505,11 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) vmx->msr_ia32_umwait_control = 0; - vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); + eax = 1; + if (!kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, true)) + eax = get_rdx_init_val(); + kvm_rdx_write(vcpu, eax); + vmx->hv_deadline_tsc = -1; kvm_set_cr8(vcpu, 0);
Set EDX at RESET/INIT based on the userspace-defined CPUID model when possible, i.e. when CPUID.0x1.EAX is defined by userspace. At RESET/INIT, all CPUs that support CPUID set EDX to the FMS enumerated in CPUID.0x1.EAX. If no CPUID match is found, fall back to KVM's default of 0x600 (Family '6'), which is the least awful approximation of KVM's virtual CPU model. Fixes: 6aa8b732ca01 ("[PATCH] kvm: userspace interface") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)