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

[Binbin Wu <binbin.wu@linux.intel.com>]

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