Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest CPUID

[Sean Christopherson]

On Thu, Nov 16, 2023, Weijiang Yang wrote:
> On 11/11/2023 7:55 AM, Sean Christopherson wrote:
> 
> [...]
> 
> > -static __always_inline void guest_cpu_cap_check_and_set(struct kvm_vcpu *vcpu,
> > -							unsigned int x86_feature)
> > +static __always_inline void guest_cpu_cap_clear(struct kvm_vcpu *vcpu,
> > +						unsigned int x86_feature)
> >   {
> > -	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
> > +	unsigned int x86_leaf = __feature_leaf(x86_feature);
> > +
> > +	reverse_cpuid_check(x86_leaf);
> > +	vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
> > +}
> > +
> > +static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu,
> > +						 unsigned int x86_feature,
> > +						 bool guest_has_cap)
> > +{
> > +	if (guest_has_cap)
> >   		guest_cpu_cap_set(vcpu, x86_feature);
> > +	else
> > +		guest_cpu_cap_clear(vcpu, x86_feature);
> > +}
> 
> I don't see any necessity to add 3 functions, i.e., guest_cpu_cap_{set, clear, change}, for

I want to have equivalents to the cpuid_entry_*() APIs so that we don't end up
with two different sets of names.  And the clear() API already has a second user.

> guest_cpu_cap update. IMHO one function is enough, e.g,:

Hrm, I open coded the OR/AND logic in cpuid_entry_change() to try to force CMOV
instead of Jcc.  That honestly seems like a pointless optimization.  I would
rather use the helpers, which is less code.

> static __always_inline void guest_cpu_cap_update(struct kvm_vcpu *vcpu,
>                                                  unsigned int x86_feature,
>                                                  bool guest_has_cap)
> {
>         unsigned int x86_leaf = __feature_leaf(x86_feature);
> 
> reverse_cpuid_check(x86_leaf);
>         if (guest_has_cap)
>                 vcpu->arch.cpu_caps[x86_leaf] |= __feature_bit(x86_feature);
> else
>                 vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
> }
> 
> > +
> > +static __always_inline void guest_cpu_cap_restrict(struct kvm_vcpu *vcpu,
> > +						   unsigned int x86_feature)
> > +{
> > +	if (!kvm_cpu_cap_has(x86_feature))
> > +		guest_cpu_cap_clear(vcpu, x86_feature);
> >   }
> 
> _restrict is not clear to me for what the function actually does -- it
> conditionally clears guest cap depending on KVM support of the feature.
> 
> How about renaming it to guest_cpu_cap_sync()?

"sync" isn't correct because it's not synchronizing with KVM's capabilitiy, e.g.
the guest capability will remaing unset if the guest CPUID bit is clear but the
KVM capability is available.

How about constrain()?