Re: [RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Sean Christopherson <sean.j.christopherson@intel.com>]

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);
> > > +	}
> > > +}