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

[Sean Christopherson <seanjc@google.com>]

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...

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 4c91f626c058..6e3cb958afdd 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -157,6 +157,14 @@ static inline ulong kvm_read_cr0_bits(struct kvm_vcpu *vcpu, ulong mask)
        return vcpu->arch.cr0 & mask;
 }
 
+static __always_inline bool kvm_is_cr0_bit_set(struct kvm_vcpu *vcpu,
+                                              unsigned long cr0_bit)
+{
+       BUILD_BUG_ON(!is_power_of_2(cr0_bit));
+
+       return !!kvm_read_cr0_bits(vcpu, cr0_bit);
+}
+
 static inline ulong kvm_read_cr0(struct kvm_vcpu *vcpu)
 {
        return kvm_read_cr0_bits(vcpu, ~0UL);
@@ -178,6 +186,14 @@ static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu)
        return vcpu->arch.cr3;
 }
 
+static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu,
+                                              unsigned long cr4_bit)
+{
+       BUILD_BUG_ON(!is_power_of_2(cr4_bit));
+
+       return !!kvm_read_cr4_bits(vcpu, cr4_bit);
+}
+