[PATCH] KVM: x86: fix cocci warnings

[PATCH] KVM: x86: fix cocci warnings

[Vihas Mak]

change 0 to false and 1 to true to fix following cocci warnings:

        arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
        arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool

Signed-off-by: Vihas Mak <makvihas@gmail.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Joerg Roedel <joro@8bytes.org>
---
 arch/x86/kvm/mmu/mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 337943799..2fcea4a78 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1454,7 +1454,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
 {
 	u64 *sptep;
 	struct rmap_iterator iter;
-	int need_flush = 0;
+	bool need_flush = false;
 	u64 new_spte;
 	kvm_pfn_t new_pfn;
 
@@ -1466,7 +1466,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
 		rmap_printk("spte %p %llx gfn %llx (%d)\n",
 			    sptep, *sptep, gfn, level);
 
-		need_flush = 1;
+		need_flush = true;
 
 		if (pte_write(pte)) {
 			pte_list_remove(kvm, rmap_head, sptep);
@@ -1482,7 +1482,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
 
 	if (need_flush && kvm_available_flush_tlb_with_range()) {
 		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
-		return 0;
+		return false;
 	}
 
 	return need_flush;
@@ -1623,8 +1623,8 @@ static bool kvm_test_age_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
 
 	for_each_rmap_spte(rmap_head, &iter, sptep)
 		if (is_accessed_spte(*sptep))
-			return 1;
-	return 0;
+			return true;
+	return false;
 }
 
 #define RMAP_RECYCLE_THRESHOLD 1000
-- 
2.25.1

Re: [PATCH] KVM: x86: fix cocci warnings

[Vitaly Kuznetsov]

Vihas Mak <makvihas@gmail.com> writes:

> change 0 to false and 1 to true to fix following cocci warnings:
>
>         arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
>         arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool
>
> Signed-off-by: Vihas Mak <makvihas@gmail.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> ---
>  arch/x86/kvm/mmu/mmu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 337943799..2fcea4a78 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1454,7 +1454,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  {
>  	u64 *sptep;
>  	struct rmap_iterator iter;
> -	int need_flush = 0;
> +	bool need_flush = false;
>  	u64 new_spte;
>  	kvm_pfn_t new_pfn;
>  
> @@ -1466,7 +1466,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  		rmap_printk("spte %p %llx gfn %llx (%d)\n",
>  			    sptep, *sptep, gfn, level);
>  
> -		need_flush = 1;
> +		need_flush = true;
>  
>  		if (pte_write(pte)) {
>  			pte_list_remove(kvm, rmap_head, sptep);
> @@ -1482,7 +1482,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  
>  	if (need_flush && kvm_available_flush_tlb_with_range()) {
>  		kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
> -		return 0;
> +		return false;
>  	}
>  
>  	return need_flush;
> @@ -1623,8 +1623,8 @@ static bool kvm_test_age_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  
>  	for_each_rmap_spte(rmap_head, &iter, sptep)
>  		if (is_accessed_spte(*sptep))
> -			return 1;
> -	return 0;
> +			return true;
> +	return false;
>  }
>  
>  #define RMAP_RECYCLE_THRESHOLD 1000

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

One minor remark: 'kvm_set_pte_rmapp()' handler is passed to
'kvm_handle_gfn_range()' which does

        bool ret = false;

        for_each_slot_rmap_range(...)
                ret |= handler(...);

and I find '|=' to not be very natural with booleans. I'm not sure it's
worth changing though.

-- 
Vitaly

Re: [PATCH] KVM: x86: fix cocci warnings

[Vihas Mak]

> and I find '|=' to not be very natural with booleans. I'm not sure it's
> worth changing though.

I see. But there are many functions in which '|=' is used on booleans.
get_mmio_spte(), __rmap_write_protect(), kvm_handle_gfn_range and many more.
That's why I thought it would be better if the code follows the same convention.

Thanks,
Vihas


On Mon, Nov 15, 2021 at 3:29 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Vihas Mak <makvihas@gmail.com> writes:
>
> > change 0 to false and 1 to true to fix following cocci warnings:
> >
> >         arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
> >         arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool
> >
> > Signed-off-by: Vihas Mak <makvihas@gmail.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Cc: Wanpeng Li <wanpengli@tencent.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > ---
> >  arch/x86/kvm/mmu/mmu.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 337943799..2fcea4a78 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1454,7 +1454,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> >  {
> >       u64 *sptep;
> >       struct rmap_iterator iter;
> > -     int need_flush = 0;
> > +     bool need_flush = false;
> >       u64 new_spte;
> >       kvm_pfn_t new_pfn;
> >
> > @@ -1466,7 +1466,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> >               rmap_printk("spte %p %llx gfn %llx (%d)\n",
> >                           sptep, *sptep, gfn, level);
> >
> > -             need_flush = 1;
> > +             need_flush = true;
> >
> >               if (pte_write(pte)) {
> >                       pte_list_remove(kvm, rmap_head, sptep);
> > @@ -1482,7 +1482,7 @@ static bool kvm_set_pte_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> >
> >       if (need_flush && kvm_available_flush_tlb_with_range()) {
> >               kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
> > -             return 0;
> > +             return false;
> >       }
> >
> >       return need_flush;
> > @@ -1623,8 +1623,8 @@ static bool kvm_test_age_rmapp(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> >
> >       for_each_rmap_spte(rmap_head, &iter, sptep)
> >               if (is_accessed_spte(*sptep))
> > -                     return 1;
> > -     return 0;
> > +                     return true;
> > +     return false;
> >  }
> >
> >  #define RMAP_RECYCLE_THRESHOLD 1000
>
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> One minor remark: 'kvm_set_pte_rmapp()' handler is passed to
> 'kvm_handle_gfn_range()' which does
>
>         bool ret = false;
>
>         for_each_slot_rmap_range(...)
>                 ret |= handler(...);
>
> and I find '|=' to not be very natural with booleans. I'm not sure it's
> worth changing though.
>
> --
> Vitaly
>

Re: [PATCH] KVM: x86: fix cocci warnings

[Paolo Bonzini]

On 11/15/21 10:59, Vitaly Kuznetsov wrote:
> One minor remark: 'kvm_set_pte_rmapp()' handler is passed to
> 'kvm_handle_gfn_range()' which does
> 
>          bool ret = false;
> 
>          for_each_slot_rmap_range(...)
>                  ret |= handler(...);
> 
> and I find '|=' to not be very natural with booleans. I'm not sure it's
> worth changing though.

Changing that would be "harder" than it seems because "ret = ret || 
handler(...)" is wrong, and "|" is even more unnatural than "|=" (so 
much that clang warns about it).

In fact I wonder if "|=" with a bool might end up warning with clang, 
which we should check before applying this patch.  It doesn't seem to be 
in the original commit[1], but better safe than sorry: Nick, does clang 
intend to warn also about "ret |= fn()" and "ret &= fn()"?  Technically, 
it is a bitwise operation with side-effects in the RHS.

Paolo

[1] https://github.com/llvm/llvm-project/commit/f59cc9542bfb461

Re: [PATCH] KVM: x86: fix cocci warnings

[Nick Desaulniers]

On Tue, Nov 16, 2021 at 1:50 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/15/21 10:59, Vitaly Kuznetsov wrote:
> > One minor remark: 'kvm_set_pte_rmapp()' handler is passed to
> > 'kvm_handle_gfn_range()' which does
> >
> >          bool ret = false;
> >
> >          for_each_slot_rmap_range(...)
> >                  ret |= handler(...);
> >
> > and I find '|=' to not be very natural with booleans. I'm not sure it's
> > worth changing though.
>
> Changing that would be "harder" than it seems because "ret = ret ||
> handler(...)" is wrong, and "|" is even more unnatural than "|=" (so
> much that clang warns about it).
>
> In fact I wonder if "|=" with a bool might end up warning with clang,
> which we should check before applying this patch.  It doesn't seem to be
> in the original commit[1], but better safe than sorry: Nick, does clang
> intend to warn also about "ret |= fn()" and "ret &= fn()"?  Technically,
> it is a bitwise operation with side-effects in the RHS.

I think that warning had more to due with typo's where `||` or `&&`
was meant (to short circuit the side effects) but `|` or `&` was typed
by accident, keeping both side effects.  I'm not sure what the typo
would be in `ret |= fn();`.

>
> Paolo
>
> [1] https://github.com/llvm/llvm-project/commit/f59cc9542bfb461
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] KVM: x86: fix cocci warnings

[Peter Zijlstra]

On Sun, Nov 14, 2021 at 10:13:12PM +0530, Vihas Mak wrote:
> change 0 to false and 1 to true to fix following cocci warnings:
> 
>         arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
>         arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool

That script should be deleted, it's absolute garbage.

Re: [PATCH] KVM: x86: fix cocci warnings

[Paolo Bonzini]

On 11/15/21 12:43, Peter Zijlstra wrote:
> On Sun, Nov 14, 2021 at 10:13:12PM +0530, Vihas Mak wrote:
>> change 0 to false and 1 to true to fix following cocci warnings:
>>
>>          arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
>>          arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool
> 
> That script should be deleted, it's absolute garbage.
> 

Only a Sith deals in absolutes.

Paolo

Re: [PATCH] KVM: x86: fix cocci warnings

[Peter Zijlstra]

On Mon, Nov 15, 2021 at 06:06:08PM +0100, Paolo Bonzini wrote:
> On 11/15/21 12:43, Peter Zijlstra wrote:
> > On Sun, Nov 14, 2021 at 10:13:12PM +0530, Vihas Mak wrote:
> > > change 0 to false and 1 to true to fix following cocci warnings:
> > > 
> > >          arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
> > >          arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool
> > 
> > That script should be deleted, it's absolute garbage.
> > 
> 
> Only a Sith deals in absolutes.

Is that a star-wars thingy?

In C 0 is a valid way to spell false, equally, any non-0 value is a
valid way to spell true. Why would this rate a warn?

In fact, when casting _Bool to integer, you get 0 and 1. When looking at
the memory content of the _Bool variable, you'll get 0 and 1. But we're
not allowed to write 0 and 1?

Re: [PATCH] KVM: x86: fix cocci warnings

[Paolo Bonzini]

On 11/15/21 21:49, Peter Zijlstra wrote:
> On Mon, Nov 15, 2021 at 06:06:08PM +0100, Paolo Bonzini wrote:
>> On 11/15/21 12:43, Peter Zijlstra wrote:
>>> On Sun, Nov 14, 2021 at 10:13:12PM +0530, Vihas Mak wrote:
>>>> change 0 to false and 1 to true to fix following cocci warnings:
>>>>
>>>>           arch/x86/kvm/mmu/mmu.c:1485:9-10: WARNING: return of 0/1 in function 'kvm_set_pte_rmapp' with return type bool
>>>>           arch/x86/kvm/mmu/mmu.c:1636:10-11: WARNING: return of 0/1 in function 'kvm_test_age_rmapp' with return type bool
>>>
>>> That script should be deleted, it's absolute garbage.
>>
>> Only a Sith deals in absolutes.
> 
> Is that a star-wars thingy?

Yes, it is.  "If you're not with me, then you're my enemy!" "Only a Sith 
deals in absolutes". :)

> In C 0 is a valid way to spell false, equally, any non-0 value is a
> valid way to spell true. Why would this rate a warn?

Because often 0 means success (if -errno means failure).  So if you 
write false/true consistently for bool and 0 only for int, it's one less 
thing that one can get wrong.  At least that's the rationale.

Paolo

> In fact, when casting _Bool to integer, you get 0 and 1. When looking at
> the memory content of the _Bool variable, you'll get 0 and 1. But we're
> not allowed to write 0 and 1?
>