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