| Message ID | 20200528151927.14346-1-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: X86: Call kvm_x86_ops.cpuid_update() after CPUIDs fully updated | expand |
On 28/05/20 17:19, Xiaoyao Li wrote: > kvm_x86_ops.cpuid_update() is used to update vmx/svm settings based on > updated CPUID settings. So it's supposed to be called after CPUIDs are > fully updated, not in the middle stage. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> Are you seeing anything bad happening from this? Paolo > --- > arch/x86/kvm/cpuid.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index cd708b0b460a..753739bc1bf0 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -208,8 +208,11 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > vcpu->arch.cpuid_nent = cpuid->nent; > cpuid_fix_nx_cap(vcpu); > kvm_apic_set_version(vcpu); > - kvm_x86_ops.cpuid_update(vcpu); > r = kvm_update_cpuid(vcpu); > + if (r) > + goto out; > + > + kvm_x86_ops.cpuid_update(vcpu); > > out: > vfree(cpuid_entries); > @@ -231,8 +234,11 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu, > goto out; > vcpu->arch.cpuid_nent = cpuid->nent; > kvm_apic_set_version(vcpu); > - kvm_x86_ops.cpuid_update(vcpu); > r = kvm_update_cpuid(vcpu); > + if (r) > + goto out; > + > + kvm_x86_ops.cpuid_update(vcpu); > out: > return r; > } >
On 5/28/2020 11:22 PM, Paolo Bonzini wrote: > On 28/05/20 17:19, Xiaoyao Li wrote: >> kvm_x86_ops.cpuid_update() is used to update vmx/svm settings based on >> updated CPUID settings. So it's supposed to be called after CPUIDs are >> fully updated, not in the middle stage. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > Are you seeing anything bad happening from this? Not yet. IMO changing the order is more reasonable and less confusing. > Paolo > >> --- >> arch/x86/kvm/cpuid.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index cd708b0b460a..753739bc1bf0 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -208,8 +208,11 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, >> vcpu->arch.cpuid_nent = cpuid->nent; >> cpuid_fix_nx_cap(vcpu); >> kvm_apic_set_version(vcpu); >> - kvm_x86_ops.cpuid_update(vcpu); >> r = kvm_update_cpuid(vcpu); >> + if (r) >> + goto out; >> + >> + kvm_x86_ops.cpuid_update(vcpu); >> >> out: >> vfree(cpuid_entries); >> @@ -231,8 +234,11 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu, >> goto out; >> vcpu->arch.cpuid_nent = cpuid->nent; >> kvm_apic_set_version(vcpu); >> - kvm_x86_ops.cpuid_update(vcpu); >> r = kvm_update_cpuid(vcpu); >> + if (r) >> + goto out; >> + >> + kvm_x86_ops.cpuid_update(vcpu); >> out: >> return r; >> } >> >
On 28/05/20 17:40, Xiaoyao Li wrote: >> >>> kvm_x86_ops.cpuid_update() is used to update vmx/svm settings based on >>> updated CPUID settings. So it's supposed to be called after CPUIDs are >>> fully updated, not in the middle stage. >>> >>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> >> Are you seeing anything bad happening from this? > > Not yet. > > IMO changing the order is more reasonable and less confusing. Indeed, I just could not decide whether to include it in 5.7 or not. Paolo
On 5/29/2020 12:15 AM, Paolo Bonzini wrote: > On 28/05/20 17:40, Xiaoyao Li wrote: >>> >>>> kvm_x86_ops.cpuid_update() is used to update vmx/svm settings based on >>>> updated CPUID settings. So it's supposed to be called after CPUIDs are >>>> fully updated, not in the middle stage. >>>> >>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >>> >>> Are you seeing anything bad happening from this? >> >> Not yet. >> >> IMO changing the order is more reasonable and less confusing. > > Indeed, I just could not decide whether to include it in 5.7 or not. Maybe for 5.8 I have a new idea to refactor a bit more that I find it does three things in kvm_update_cpuid(): - update cpuid; - update vcpu states, e.g., apic->lapic_timer.timer_mode_mask, guest_supported_xcr0, maxphyaddr, ... etc, - cpuid check, for vaddr_bits I'm going to split it, and make the order as: 1. kvm_check_cpuid(), if invalid value return error; 2. kvm_update_cpuid(); 3. kvm_update_state_based_on_cpuid(); and kvm_x86_ops.kvm_x86_ops.cpuid_update() can be called inside it. If you feel OK, I'll do it tomorrow. -Xiaoyao
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index cd708b0b460a..753739bc1bf0 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -208,8 +208,11 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, vcpu->arch.cpuid_nent = cpuid->nent; cpuid_fix_nx_cap(vcpu); kvm_apic_set_version(vcpu); - kvm_x86_ops.cpuid_update(vcpu); r = kvm_update_cpuid(vcpu); + if (r) + goto out; + + kvm_x86_ops.cpuid_update(vcpu); out: vfree(cpuid_entries); @@ -231,8 +234,11 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu, goto out; vcpu->arch.cpuid_nent = cpuid->nent; kvm_apic_set_version(vcpu); - kvm_x86_ops.cpuid_update(vcpu); r = kvm_update_cpuid(vcpu); + if (r) + goto out; + + kvm_x86_ops.cpuid_update(vcpu); out: return r; }
kvm_x86_ops.cpuid_update() is used to update vmx/svm settings based on updated CPUID settings. So it's supposed to be called after CPUIDs are fully updated, not in the middle stage. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/kvm/cpuid.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)