Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Tue, 28 Nov 2023 13:13:46 +0800
From:   Xu Yilun <yilun.xu@linux.intel.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Maxim Levitsky <mlevitsk@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/9] KVM: x86: Update guest cpu_caps at runtime for
 dynamic CPUID-based features
Message-ID: <ZWV3CoZg7VGBa7za@yilunxu-OptiPlex-7050>
References: <20231110235528.1561679-1-seanjc@google.com>
 <20231110235528.1561679-7-seanjc@google.com>
 <4484647425e2dbf92c76a173b7b14e346f60362d.camel@redhat.com>
 <ZWBDsOJpdi7hWaYV@yilunxu-OptiPlex-7050>
 <ZWU3wTElmiEOUg-I@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZWU3wTElmiEOUg-I@google.com>
Precedence: bulk

On Mon, Nov 27, 2023 at 04:43:45PM -0800, Sean Christopherson wrote:
> On Fri, Nov 24, 2023, Xu Yilun wrote:
> > On Sun, Nov 19, 2023 at 07:35:30PM +0200, Maxim Levitsky wrote:
> > > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > > >  static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
> > > >  				       int nent)
> > > >  {
> > > >  	struct kvm_cpuid_entry2 *best;
> > > > +	struct kvm_vcpu *caps = vcpu;
> > > > +
> > > > +	/*
> > > > +	 * Don't update vCPU capabilities if KVM is updating CPUID entries that
> > > > +	 * are coming in from userspace!
> > > > +	 */
> > > > +	if (entries != vcpu->arch.cpuid_entries)
> > > > +		caps = NULL;
> > > 
> > > I think that this should be decided by the caller. Just a boolean will suffice.
> 
> I strongly disagree.  The _only_ time the caps should be updated is if
> entries == vcpu->arch.cpuid_entries, and if entries == cpuid_entires than the caps
> should _always_ be updated.
> 
> > kvm_set_cpuid() calls this function only to validate/adjust the temporary
> > "entries" variable. While kvm_update_cpuid_runtime() calls it to do system
> > level changes.
> > 
> > So I kind of agree to make the caller fully awared, how about adding a
> > newly named wrapper for kvm_set_cpuid(), like:
> > 
> > 
> >   static void kvm_adjust_cpuid_entry(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
> > 				     int nent)
> > 
> >   {
> > 	WARN_ON(entries == vcpu->arch.cpuid_entries);
> > 	__kvm_update_cpuid_runtime(vcpu, entries, nent);
> 
> But taking it a step further, we end up with
> 
> 	WARN_ON_ONCE(update_caps != (entries == vcpu->arch.cpuid_entries));
> 
> which is silly since any bugs that would result in the WARN firing can be avoided
> by doing:
> 
> 	update_caps = entries == vcpu->arch.cpuid_entries;
> 
> which eventually distils down to the code I posted.

OK, I agree with you.

My initial idea is to make developers easier to recognize what is
happening by name, without looking into __kvm_update_cpuid_runtime().
But it seems causing other subtle confuse and wastes cycles. Maybe the
comment is already good enough for developers.

Thanks,
Yilun