Re: [PATCH v5 2/5] [Trivial]KVM: x86: Explicitly cast ulong to bool in kvm_set_cr3()

[Sean Christopherson <seanjc@google.com>]

On Mon, Mar 20, 2023, Binbin Wu wrote:
> 
> On 3/20/2023 8:05 PM, Binbin Wu wrote:
> > 
> > On 3/11/2023 4:22 AM, Sean Christopherson wrote:
> > > As Chao pointed out, this does not belong in the LAM series.� And
> > > FWIW, I highly
> > > recommend NOT tagging things as Trivial.� If you're wrong and the
> > > patch _isn't_
> > > trivial, it only slows things down.� And if you're right, then
> > > expediting the
> > > patch can't possibly be necessary.
> > > 
> > > On Fri, Mar 03, 2023, Robert Hoo wrote:
> > > > On Thu, 2023-03-02 at 15:24 +0800, Chao Gao wrote:
> > > > > > -��� bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> > > > > > +��� bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> > > > > > 
> > > > > > ����if (pcid_enabled) {
> > > > > > ������� skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH;
> > > > > pcid_enabled is used only once. You can drop it, i.e.,
> > > > > 
> > > > > ����if (kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) {
> > > > > 
> > > > Emm, that's actually another point.
> > > > Though I won't object so, wouldn't this be compiler optimized?
> > > > 
> > > > And my point was: honor bool type, though in C implemention it's 0 and
> > > > !0, it has its own type value: true, false.
> > > > Implicit type casting always isn't good habit.
> > > I don't disagree, but I also don't particularly want to "fix" one
> > > case while
> > > ignoring the many others, e.g. kvm_handle_invpcid() has the exact
> > > same "buggy"
> > > pattern.
> > > 
> > > I would be supportive of a patch that adds helpers and then converts
> > > all of the
> > > relevant CR0/CR4 checks though...
> > 
> > Hi Sean, I can cook a patch by your suggesion and sent out the patch
> > seperately.
> 
> Sean, besides the call of kvm_read_cr0_bits() and kvm_read_cr4_bits(), there
> are also a lot checks in if statement like
> if ( cr4 & X86_CR4_XXX )
> or
> if ( cr0 & X86_CR0_XXX )
> I suppose these usages are OK, right?

Generally speaking, yes.  Most flows of that nature use a local variable for very
good reasons.  The only one I would probably convert is this code in
svm_can_emulate_instruction().  

	cr4 = kvm_read_cr4(vcpu);
	smep = cr4 & X86_CR4_SMEP;
	smap = cr4 & X86_CR4_SMAP;