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
guest_cpu_cap update. IMHO one function is enough, e.g,:
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()?
>
> static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8a99a73b6ee5..5827328e30f1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4315,14 +4315,14 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * XSS on VM-Enter/VM-Exit. Failure to do so would effectively give
> * the guest read/write access to the host's XSS.
> */
> - if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> - boot_cpu_has(X86_FEATURE_XSAVES) &&
> - guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> - guest_cpu_cap_set(vcpu, X86_FEATURE_XSAVES);
> + guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> + boot_cpu_has(X86_FEATURE_XSAVE) &&
> + boot_cpu_has(X86_FEATURE_XSAVES) &&
> + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE));
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_NRIPS);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_TSCRATEMSR);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_LBRV);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_NRIPS);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_TSCRATEMSR);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_LBRV);
>
> /*
> * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that
> @@ -4330,12 +4330,12 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * SVM on Intel is bonkers and extremely unlikely to work).
> */
> if (!guest_cpuid_is_intel(vcpu))
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PAUSEFILTER);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_PFTHRESHOLD);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VGIF);
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VNMI);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI);
>
> svm_recalc_instruction_intercepts(vcpu, svm);
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6328f0d47c64..5a056ad1ae55 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7757,9 +7757,11 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> */
> if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_XSAVES);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_XSAVES);
> + else
> + guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES);
>
> - guest_cpu_cap_check_and_set(vcpu, X86_FEATURE_VMX);
> + guest_cpu_cap_restrict(vcpu, X86_FEATURE_VMX);
>
> vmx_setup_uret_msrs(vmx);
>
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()?
On 11/17/2023 6:29 AM, Sean Christopherson wrote:
> 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()?
I don't know, just feel we already have guest_cpu_cap_{set, clear, change}, here the name cannot exactly match the behavior of the function, maybe guest_cpu_cap_filter()? But just ignore the nit, up to you to decide the name :-)
On Fri, Nov 17, 2023 at 04:33:27PM +0800, Yang, Weijiang wrote:
> On 11/17/2023 6:29 AM, Sean Christopherson wrote:
> > 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()?
> I don't know, just feel we already have guest_cpu_cap_{set, clear, change}, here the name cannot exactly match the behavior of the function, maybe guest_cpu_cap_filter()? But just ignore the nit, up to you to decide the name :-)
How about guest_cpu_cap_kvm_restrict or guest_cpu_cap_kvm_constrain ?
>
>