linux-kernel - Re: [PATCH 3/9] KVM: x86: Initialize guest cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZVaXroTZQi1IcTvm@google.com>
Date:   Thu, 16 Nov 2023 14:29:02 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Weijiang Yang <weijiang.yang@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest CPUID

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()?