| Message ID | 20200623115816.24132-3-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor handling flow of SET_CPUID* | expand |
On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote: > As handling of bits other leaf 1 added over time, kvm_update_cpuid() > should not return directly if leaf 1 is absent, but should go on > updateing other CPUID leaves. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> This should probably be marked for stable. > --- > arch/x86/kvm/cpuid.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 1d13bad42bf9..0164dac95ef5 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) > struct kvm_lapic *apic = vcpu->arch.apic; > > best = kvm_find_cpuid_entry(vcpu, 1, 0); > - if (!best) > - return 0; Rather than wrap the existing code, what about throwing it in a separate helper? That generates an easier to read diff and also has the nice property of getting 'apic' out of the common code. > - > - /* Update OSXSAVE bit */ > - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1) > - cpuid_entry_change(best, X86_FEATURE_OSXSAVE, > + if (best) { > + /* Update OSXSAVE bit */ > + if (boot_cpu_has(X86_FEATURE_XSAVE)) > + cpuid_entry_change(best, X86_FEATURE_OSXSAVE, > kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)); > > - cpuid_entry_change(best, X86_FEATURE_APIC, > + cpuid_entry_change(best, X86_FEATURE_APIC, > vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE); > > - if (apic) { > - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > - apic->lapic_timer.timer_mode_mask = 3 << 17; > - else > - apic->lapic_timer.timer_mode_mask = 1 << 17; > + if (apic) { > + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > + apic->lapic_timer.timer_mode_mask = 3 << 17; > + else > + apic->lapic_timer.timer_mode_mask = 1 << 17; > + } > } > > best = kvm_find_cpuid_entry(vcpu, 7, 0); > -- > 2.18.2 >
On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote: > On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote: > > As handling of bits other leaf 1 added over time, kvm_update_cpuid() > > should not return directly if leaf 1 is absent, but should go on > > updateing other CPUID leaves. > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > This should probably be marked for stable. > > > --- > > arch/x86/kvm/cpuid.c | 23 +++++++++++------------ > > 1 file changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index 1d13bad42bf9..0164dac95ef5 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) > > struct kvm_lapic *apic = vcpu->arch.apic; > > > > best = kvm_find_cpuid_entry(vcpu, 1, 0); > > - if (!best) > > - return 0; > > Rather than wrap the existing code, what about throwing it in a separate > helper? That generates an easier to read diff and also has the nice > property of getting 'apic' out of the common code. Hrm, that'd be overkill once the apic code is moved in a few patches. What if you keep the cpuid updates wrapped (as in this patch), but then do if (best && apic) { } for the apic path? That'll minimize churn for code that is disappearing, e.g. will make future git archaeologists happy :-). > > - > > - /* Update OSXSAVE bit */ > > - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1) > > - cpuid_entry_change(best, X86_FEATURE_OSXSAVE, > > + if (best) { > > + /* Update OSXSAVE bit */ > > + if (boot_cpu_has(X86_FEATURE_XSAVE)) > > + cpuid_entry_change(best, X86_FEATURE_OSXSAVE, > > kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)); > > > > - cpuid_entry_change(best, X86_FEATURE_APIC, > > + cpuid_entry_change(best, X86_FEATURE_APIC, > > vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE); > > > > - if (apic) { > > - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > > - apic->lapic_timer.timer_mode_mask = 3 << 17; > > - else > > - apic->lapic_timer.timer_mode_mask = 1 << 17; > > + if (apic) { > > + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) > > + apic->lapic_timer.timer_mode_mask = 3 << 17; > > + else > > + apic->lapic_timer.timer_mode_mask = 1 << 17; > > + } > > } > > > > best = kvm_find_cpuid_entry(vcpu, 7, 0); > > -- > > 2.18.2 > >
On 7/3/2020 3:02 AM, Sean Christopherson wrote: > On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote: >> On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote: >>> As handling of bits other leaf 1 added over time, kvm_update_cpuid() >>> should not return directly if leaf 1 is absent, but should go on >>> updateing other CPUID leaves. >>> >>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> >> This should probably be marked for stable. >> >>> --- >>> arch/x86/kvm/cpuid.c | 23 +++++++++++------------ >>> 1 file changed, 11 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index 1d13bad42bf9..0164dac95ef5 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) >>> struct kvm_lapic *apic = vcpu->arch.apic; >>> >>> best = kvm_find_cpuid_entry(vcpu, 1, 0); >>> - if (!best) >>> - return 0; >> >> Rather than wrap the existing code, what about throwing it in a separate >> helper? That generates an easier to read diff and also has the nice >> property of getting 'apic' out of the common code. > > Hrm, that'd be overkill once the apic code is moved in a few patches. > What if you keep the cpuid updates wrapped (as in this patch), but then > do > > if (best && apic) { > } > > for the apic path? That'll minimize churn for code that is disappearing, > e.g. will make future git archaeologists happy :-). Sure. I'll do it in next version. >>> - >>> - /* Update OSXSAVE bit */ >>> - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1) >>> - cpuid_entry_change(best, X86_FEATURE_OSXSAVE, >>> + if (best) { >>> + /* Update OSXSAVE bit */ >>> + if (boot_cpu_has(X86_FEATURE_XSAVE)) >>> + cpuid_entry_change(best, X86_FEATURE_OSXSAVE, >>> kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)); >>> >>> - cpuid_entry_change(best, X86_FEATURE_APIC, >>> + cpuid_entry_change(best, X86_FEATURE_APIC, >>> vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE); >>> >>> - if (apic) { >>> - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) >>> - apic->lapic_timer.timer_mode_mask = 3 << 17; >>> - else >>> - apic->lapic_timer.timer_mode_mask = 1 << 17; >>> + if (apic) { >>> + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) >>> + apic->lapic_timer.timer_mode_mask = 3 << 17; >>> + else >>> + apic->lapic_timer.timer_mode_mask = 1 << 17; >>> + } >>> } >>> >>> best = kvm_find_cpuid_entry(vcpu, 7, 0); >>> -- >>> 2.18.2 >>>
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 1d13bad42bf9..0164dac95ef5 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) struct kvm_lapic *apic = vcpu->arch.apic; best = kvm_find_cpuid_entry(vcpu, 1, 0); - if (!best) - return 0; - - /* Update OSXSAVE bit */ - if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1) - cpuid_entry_change(best, X86_FEATURE_OSXSAVE, + if (best) { + /* Update OSXSAVE bit */ + if (boot_cpu_has(X86_FEATURE_XSAVE)) + cpuid_entry_change(best, X86_FEATURE_OSXSAVE, kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)); - cpuid_entry_change(best, X86_FEATURE_APIC, + cpuid_entry_change(best, X86_FEATURE_APIC, vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE); - if (apic) { - if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) - apic->lapic_timer.timer_mode_mask = 3 << 17; - else - apic->lapic_timer.timer_mode_mask = 1 << 17; + if (apic) { + if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER)) + apic->lapic_timer.timer_mode_mask = 3 << 17; + else + apic->lapic_timer.timer_mode_mask = 1 << 17; + } } best = kvm_find_cpuid_entry(vcpu, 7, 0);
As handling of bits other leaf 1 added over time, kvm_update_cpuid() should not return directly if leaf 1 is absent, but should go on updateing other CPUID leaves. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/kvm/cpuid.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)