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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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?





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);
+}
+



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux