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 <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>
Cc: Wanpeng Li <[email protected]>
Cc: Jim Mattson <[email protected]>
Cc: Joerg Roedel <[email protected]>
---
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
Vihas Mak <[email protected]> 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 <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Cc: Vitaly Kuznetsov <[email protected]>
> Cc: Wanpeng Li <[email protected]>
> Cc: Jim Mattson <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> ---
> 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 <[email protected]>
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
> 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 <[email protected]> wrote:
>
> Vihas Mak <[email protected]> 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 <[email protected]>
> > Cc: Sean Christopherson <[email protected]>
> > Cc: Vitaly Kuznetsov <[email protected]>
> > Cc: Wanpeng Li <[email protected]>
> > Cc: Jim Mattson <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > ---
> > 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 <[email protected]>
>
> 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
>
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.
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
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?
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?
>
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
On Tue, Nov 16, 2021 at 1:50 AM Paolo Bonzini <[email protected]> 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