On Tue, Oct 20, 2020 at 01:38:32PM +0800, Robert Hoo wrote:
> On Mon, 2020-09-28 at 21:56 -0700, Sean Christopherson wrote:
> > > +
> > > + best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > > +
> > > + if (best && boot_cpu_has(X86_FEATURE_XSAVE)) {
> >
> > Braces not needed. We should also check boot_cpu_has() first, it's constant
> > time and maybe even in the cache, whereas finding CPUID entries is linear
> > time and outright slow.
> >
> > Actually, can you add a helper to handle this? With tht boot_cpu_has()
> > check outside of the helper? That'll allow the helper to be used for more
> > features, and will force checking boot_cpu_has() first. Hmm, and not all
> > of the callers will need the boot_cpu_has() check, e.g. toggling PKE from
> > kvm_set_cr4() doesn't need to be guarded because KVM disallows setting PKE
> > if it's not supported by the host.
>
> Do you mean because in kvm_set_cr4(), it has kvm_valid_cr4(vcpu, cr4)
> check first?
Yes.
> Then how about the other 2 callers of kvm_pke_update_cpuid()?
> enter_smm() -- I think it can ommit boot_cpu_has() check as well.
> because it unconditionally cleared all CR4 bit before calls
> kvm_set_cr4().
> __set_sregs() -- looks like it doesn't valid host PKE status before
> call kvm_pke_update_cpuid(). Can I ommit boot_cpu_has() as well?
>
> So, I don't think kvm_pke_update_cpuid() can leverage the helper. Am I
> right?
It can leverage the guest_cpuid_change() helper below, no?
> > static inline void guest_cpuid_change(struct kvm_vcpu *vcpu, u32
> > function,
> > u32 index, unsigned int feature,
> > bool set)
> > {
> > struct kvm_cpuid_entry2 *e = kvm_find_cpuid_entry(vcpu,
> > function, index);
> >
> > if (e)
> > cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> > }
>
> Thanks Sean, I'm going to have this helper in v2 and have your signed-
> off-by.
Eh, no need to give me credit, it's just a snippet in feedback. It's
not even correct :-) E.g. s/X86_FEATURE_OSXSAVE/feature below.
> >
> > > + cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> > > + }
> > > +}
