[PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Wei Wang]

Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from callers is
32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
will be converted to 0, which is not expected. Improves the implementation
by using !!(cond) in KVM_BUG and KVM_BUG_ON. Compared to changing 'int' to
'int64_t', this has less LOCs.

Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 include/linux/kvm_host.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f06635b24bd0..d77ddf82c5c8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -881,20 +881,16 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
 
 #define KVM_BUG(cond, kvm, fmt...)				\
 ({								\
-	int __ret = (cond);					\
-								\
-	if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))		\
+	if (WARN_ONCE(!!cond && !(kvm)->vm_bugged, fmt))	\
 		kvm_vm_bugged(kvm);				\
-	unlikely(__ret);					\
+	unlikely(!!cond);					\
 })
 
 #define KVM_BUG_ON(cond, kvm)					\
 ({								\
-	int __ret = (cond);					\
-								\
-	if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))		\
+	if (WARN_ON_ONCE(!!(cond) && !(kvm)->vm_bugged))	\
 		kvm_vm_bugged(kvm);				\
-	unlikely(__ret);					\
+	unlikely(!!(cond));					\
 })
 
 static inline void kvm_vcpu_srcu_read_lock(struct kvm_vcpu *vcpu)
-- 
2.27.0

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Mingwei Zhang]

On Wed, Mar 1, 2023 at 5:39 AM Wei Wang <wei.w.wang@intel.com> wrote:
>
> Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> will be converted to 0, which is not expected. Improves the implementation
> by using !!(cond) in KVM_BUG and KVM_BUG_ON. Compared to changing 'int' to
> 'int64_t', this has less LOCs.
>
> Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  include/linux/kvm_host.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f06635b24bd0..d77ddf82c5c8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -881,20 +881,16 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
>
>  #define KVM_BUG(cond, kvm, fmt...)                             \
>  ({                                                             \
> -       int __ret = (cond);                                     \
> -                                                               \
> -       if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))         \
> +       if (WARN_ONCE(!!cond && !(kvm)->vm_bugged, fmt))        \
>                 kvm_vm_bugged(kvm);                             \
> -       unlikely(__ret);                                        \
> +       unlikely(!!cond);                                       \

Do you want to use brackets for these two places as well, i.e.: !!(cond).

>  })
>
>  #define KVM_BUG_ON(cond, kvm)                                  \
>  ({                                                             \
> -       int __ret = (cond);                                     \
> -                                                               \
> -       if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))           \
> +       if (WARN_ON_ONCE(!!(cond) && !(kvm)->vm_bugged))        \
>                 kvm_vm_bugged(kvm);                             \
> -       unlikely(__ret);                                        \
> +       unlikely(!!(cond));                                     \
>  })
>
>  static inline void kvm_vcpu_srcu_read_lock(struct kvm_vcpu *vcpu)
> --
> 2.27.0
>

Thanks for catching this one.
-Mingwei

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[David Matlack]

On Wed, Mar 1, 2023 at 5:38 AM Wei Wang <wei.w.wang@intel.com> wrote:
>
> Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> will be converted to 0, which is not expected. Improves the implementation
> by using !!(cond) in KVM_BUG and KVM_BUG_ON. Compared to changing 'int' to
> 'int64_t', this has less LOCs.

Less LOC is nice to have, but please preserve the behavior that "cond"
is evaluated only once by KVM_BUG() and KVM_BUG_ON(). i.e.
KVM_BUG_ON(do_something(), kvm) should only result in a single call to
do_something().

>
> Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  include/linux/kvm_host.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f06635b24bd0..d77ddf82c5c8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -881,20 +881,16 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
>
>  #define KVM_BUG(cond, kvm, fmt...)                             \
>  ({                                                             \
> -       int __ret = (cond);                                     \
> -                                                               \
> -       if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))         \
> +       if (WARN_ONCE(!!cond && !(kvm)->vm_bugged, fmt))        \
>                 kvm_vm_bugged(kvm);                             \
> -       unlikely(__ret);                                        \
> +       unlikely(!!cond);                                       \
>  })
>
>  #define KVM_BUG_ON(cond, kvm)                                  \
>  ({                                                             \
> -       int __ret = (cond);                                     \
> -                                                               \
> -       if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))           \
> +       if (WARN_ON_ONCE(!!(cond) && !(kvm)->vm_bugged))        \
>                 kvm_vm_bugged(kvm);                             \
> -       unlikely(__ret);                                        \
> +       unlikely(!!(cond));                                     \
>  })

RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Wang, Wei W]

On Thursday, March 2, 2023 3:47 AM, David Matlack wrote:
> On Wed, Mar 1, 2023 at 5:38 AM Wei Wang <wei.w.wang@intel.com> wrote:
> >
> > Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from
> callers
> > is 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> > provided by a caller is 64-bit, e.g. an error code of
> > 0xc0000d0300000000 will be converted to 0, which is not expected.
> > Improves the implementation by using !!(cond) in KVM_BUG and
> > KVM_BUG_ON. Compared to changing 'int' to 'int64_t', this has less LOCs.
> 
> Less LOC is nice to have, but please preserve the behavior that "cond"
> is evaluated only once by KVM_BUG() and KVM_BUG_ON(). i.e.
> KVM_BUG_ON(do_something(), kvm) should only result in a single call to
> do_something().

Good point, thanks! Using 'typeof(cond)' looks like a better choice.

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Mingwei Zhang]

On Thu, Mar 02, 2023, Wang, Wei W wrote:
> On Thursday, March 2, 2023 3:47 AM, David Matlack wrote:
> > On Wed, Mar 1, 2023 at 5:38 AM Wei Wang <wei.w.wang@intel.com> wrote:
> > >
> > > Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from
> > callers
> > > is 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> > > provided by a caller is 64-bit, e.g. an error code of
> > > 0xc0000d0300000000 will be converted to 0, which is not expected.
> > > Improves the implementation by using !!(cond) in KVM_BUG and
> > > KVM_BUG_ON. Compared to changing 'int' to 'int64_t', this has less LOCs.
> > 
> > Less LOC is nice to have, but please preserve the behavior that "cond"
> > is evaluated only once by KVM_BUG() and KVM_BUG_ON(). i.e.
> > KVM_BUG_ON(do_something(), kvm) should only result in a single call to
> > do_something().
> 
> Good point, thanks! Using 'typeof(cond)' looks like a better choice.

I don't get it. Why bothering the type if we just do this?

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4f26b244f6d0..10455253c6ea 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)

 #define KVM_BUG(cond, kvm, fmt...)				\
 ({								\
-	int __ret = (cond);					\
+	int __ret = !!(cond);					\
 								\
 	if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))		\
 		kvm_vm_bugged(kvm);				\
@@ -857,7 +857,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)

 #define KVM_BUG_ON(cond, kvm)					\
 ({								\
-	int __ret = (cond);					\
+	int __ret = !!(cond);					\
 								\
 	if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))		\
 		kvm_vm_bugged(kvm);				\

RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Wang, Wei W]

On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> I don't get it. Why bothering the type if we just do this?
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> 4f26b244f6d0..10455253c6ea 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
> 
>  #define KVM_BUG(cond, kvm, fmt...)				\
>  ({								\
> -	int __ret = (cond);					\
> +	int __ret = !!(cond);					\

This is essentially "bool __ret". No biggie to change it this way.
But I'm inclined to retain the original intention to have the macro return
the value that was passed in:
typeof(cond) __ret = (cond);

Let's what others vote for.

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Mingwei Zhang]

On Thu, Mar 2, 2023 at 2:26 AM Wang, Wei W <wei.w.wang@intel.com> wrote:
>
> On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > I don't get it. Why bothering the type if we just do this?
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index
> > 4f26b244f6d0..10455253c6ea 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
> >
> >  #define KVM_BUG(cond, kvm, fmt...)                           \
> >  ({                                                           \
> > -     int __ret = (cond);                                     \
> > +     int __ret = !!(cond);                                   \
>
> This is essentially "bool __ret". No biggie to change it this way.

!! will return an int, not a boolean, but it is used as a boolean.
This is consistent with the original code which _is_ returning an
integer.

> But I'm inclined to retain the original intention to have the macro return
> the value that was passed in:
> typeof(cond) __ret = (cond);

hmm, I think it is appropriate to retain the original type of 'cond'
especially since it may also involve other arithmetic operations. But
I doubt it will be very useful. For instance, who is going to write
this code?

......
if (KVM_BUG(cond, true) & some_mask)
  do_something()
......

>
> Let's what others vote for.

Please fix this bug first before introducing nice features.

Thanks.
-Mingwei


-Mingwei

RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Wang, Wei W]

On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > I don't get it. Why bothering the type if we just do this?
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index 4f26b244f6d0..10455253c6ea 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm
> > > *kvm)
> > >
> > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > >  ({                                                           \
> > > -     int __ret = (cond);                                     \
> > > +     int __ret = !!(cond);                                   \
> >
> > This is essentially "bool __ret". No biggie to change it this way.
> 
> !! will return an int, not a boolean, but it is used as a boolean.

What's the point of defining it as an int when actually being used as a Boolean?
Original returning of an 'int' is a bug in this sense. Either returning a Boolean or
the same type (length) as cond is good way to me.

> This is consistent with the original code which _is_ returning an integer.
> 
> > But I'm inclined to retain the original intention to have the macro
> > return the value that was passed in:
> > typeof(cond) __ret = (cond);
> 
> hmm, I think it is appropriate to retain the original type of 'cond'
> especially since it may also involve other arithmetic operations. But I doubt it
> will be very useful. For instance, who is going to write this code?
> 

Maybe there is, maybe not. But it doesn’t hurt anything to leave the
flexibility there using typeof(cond). As said, I'm also fine to use 'bool ret',
but probably not 'int' for no good reason.

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Mingwei Zhang]

On Thu, Mar 2, 2023 at 5:50 PM Wang, Wei W <wei.w.wang@intel.com> wrote:
>
> On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > > I don't get it. Why bothering the type if we just do this?
> > > >
> > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > index 4f26b244f6d0..10455253c6ea 100644
> > > > --- a/include/linux/kvm_host.h
> > > > +++ b/include/linux/kvm_host.h
> > > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm
> > > > *kvm)
> > > >
> > > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > > >  ({                                                           \
> > > > -     int __ret = (cond);                                     \
> > > > +     int __ret = !!(cond);                                   \
> > >
> > > This is essentially "bool __ret". No biggie to change it this way.
> >
> > !! will return an int, not a boolean, but it is used as a boolean.
>
> What's the point of defining it as an int when actually being used as a Boolean?
> Original returning of an 'int' is a bug in this sense. Either returning a Boolean or
> the same type (length) as cond is good way to me.

What's the point of using an integer? I think we need to ask the
original author. But I think one of the reasons might be convenience
as the return value. I am not sure if we can return a boolean in the
function. But it should be fine here since it is a macro.

Anyway, returning an 'int' is not a bug. The bug is the casting from
'cond' to the integer that may lose information and this is what you
have captured.
>
> > This is consistent with the original code which _is_ returning an integer.
> >
> > > But I'm inclined to retain the original intention to have the macro
> > > return the value that was passed in:
> > > typeof(cond) __ret = (cond);
> >
> > hmm, I think it is appropriate to retain the original type of 'cond'
> > especially since it may also involve other arithmetic operations. But I doubt it
> > will be very useful. For instance, who is going to write this code?
> >
>
> Maybe there is, maybe not. But it doesn’t hurt anything to leave the
> flexibility there using typeof(cond). As said, I'm also fine to use 'bool ret',
> but probably not 'int' for no good reason.

Right, maybe or maybe not. But using typeof(cond) for the variable
does not always provide benefits. For instance if the 'cond' is
resolved as the 8-byte type like u64, we are wasting space at runtime.
We could have used a shorter type. In addition, throwing this to the
compiler creates complexity and sometimes bugs since the compiler
could have different behaviors.

So, I prefer not having typeof(cond) for KVM_BUG(). But if you have
strong opinions using typeof, go ahead.

Thanks.
-Mingwei

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[David Matlack]

On Thu, Mar 02, 2023 at 09:53:35PM -0800, Mingwei Zhang wrote:
> On Thu, Mar 2, 2023 at 5:50 PM Wang, Wei W <wei.w.wang@intel.com> wrote:
> >
> > On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > > > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > > > I don't get it. Why bothering the type if we just do this?
> > > > >
> > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > > index 4f26b244f6d0..10455253c6ea 100644
> > > > > --- a/include/linux/kvm_host.h
> > > > > +++ b/include/linux/kvm_host.h
> > > > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm
> > > > > *kvm)
> > > > >
> > > > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > > > >  ({                                                           \
> > > > > -     int __ret = (cond);                                     \
> > > > > +     int __ret = !!(cond);                                   \
> > > >
> > > > This is essentially "bool __ret". No biggie to change it this way.
> > >
> > > !! will return an int, not a boolean, but it is used as a boolean.
> >
> > What's the point of defining it as an int when actually being used as a Boolean?
> > Original returning of an 'int' is a bug in this sense. Either returning a Boolean or
> > the same type (length) as cond is good way to me.
> 
> What's the point of using an integer? I think we need to ask the
> original author. But I think one of the reasons might be convenience
> as the return value. I am not sure if we can return a boolean in the
> function. But it should be fine here since it is a macro.
> 
> Anyway, returning an 'int' is not a bug. The bug is the casting from
> 'cond' to the integer that may lose information and this is what you
> have captured.

typeof() won't work if cond is a bitfield. See commit 8d4fbcfbe0a4 ("Fix
WARN_ON() on bitfield ops") from Linus from back in 2007:

commit 8d4fbcfbe0a4bfc73e7f0297c59ae514e1f1436f
Author: Linus Torvalds <torvalds@woody.linux-foundation.org>
Date:   Tue Jul 31 21:12:07 2007 -0700

    Fix WARN_ON() on bitfield ops

    Alexey Dobriyan noticed that the new WARN_ON() semantics that were
    introduced by commit 684f978347deb42d180373ac4c427f82ef963171 (to also
    return the value to be warned on) didn't compile when given a bitfield,
    because the typeof doesn't work for bitfields.

    So instead of the typeof trick, use an "int" variable together with a
    "!!(x)" expression, as suggested by Al Viro.

    To make matters more interesting, Paul Mackerras points out that that is
    sub-optimal on Power, but the old asm-coded comparison seems to be buggy
    anyway on 32-bit Power if the conditional was 64-bit, so I think there
    are more problems there.

    Regardless, the new WARN_ON() semantics may have been a bad idea.  But
    this at least avoids the more serious complications.

    Cc: Alexey Dobriyan <adobriyan@sw.ru>
    Cc: Herbert Xu <herbert@gondor.apana.org.au>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Al Viro <viro@ftp.linux.org.uk>
    Cc: Ingo Molnar <mingo@elte.hu>
    Cc: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 344e3091af24..d56fedbb457a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -33,7 +33,7 @@ struct bug_entry {

 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) ({                                          \
-       typeof(condition) __ret_warn_on = (condition);                  \
+       int __ret_warn_on = !!(condition);                              \
        if (unlikely(__ret_warn_on)) {                                  \
                printk("WARNING: at %s:%d %s()\n", __FILE__,            \
                        __LINE__, __FUNCTION__);                        \
@
[...]

As for int versus bool, I don't see a strong argument for either. So let's
stick with int since that's what the current code is using and that
aligns with the generic kernel WARN_ON().

If someone wants to propose using a bool instead of an int that should
be a separate commit anyway and needs an actual justification.

RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Wang, Wei W]

On Saturday, March 4, 2023 1:36 AM, David Matlack wrote:
> > > On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > > > > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > > > > I don't get it. Why bothering the type if we just do this?
> > > > > >
> > > > > > diff --git a/include/linux/kvm_host.h
> > > > > > b/include/linux/kvm_host.h index 4f26b244f6d0..10455253c6ea
> > > > > > 100644
> > > > > > --- a/include/linux/kvm_host.h
> > > > > > +++ b/include/linux/kvm_host.h
> > > > > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct
> > > > > > kvm
> > > > > > *kvm)
> > > > > >
> > > > > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > > > > >  ({                                                           \
> > > > > > -     int __ret = (cond);                                     \
> > > > > > +     int __ret = !!(cond);                                   \
> > > > >
> > > > > This is essentially "bool __ret". No biggie to change it this way.
> > > >
> > > > !! will return an int, not a boolean, but it is used as a boolean.
> > >
> > > What's the point of defining it as an int when actually being used as a
> Boolean?
> > > Original returning of an 'int' is a bug in this sense. Either
> > > returning a Boolean or the same type (length) as cond is good way to me.
> >
> > What's the point of using an integer? I think we need to ask the
> > original author. But I think one of the reasons might be convenience
> > as the return value. I am not sure if we can return a boolean in the
> > function. But it should be fine here since it is a macro.
> >
> > Anyway, returning an 'int' is not a bug. The bug is the casting from
> > 'cond' to the integer that may lose information and this is what you
> > have captured.
> 
> typeof() won't work if cond is a bitfield. See commit 8d4fbcfbe0a4 ("Fix
> WARN_ON() on bitfield ops") from Linus from back in 2007:

Yes, this seems to be a good reason for not going for typeof. Thanks for sharing.

> 
> As for int versus bool, I don't see a strong argument for either. So let's stick
> with int since that's what the current code is using and that aligns with the
> generic kernel WARN_ON().
> 
> If someone wants to propose using a bool instead of an int that should be a
> separate commit anyway and needs an actual justification.

Wait a bit. Let me seek for a valid reason from the WARN side first.

CodingStyle already says:
bool function return types and stack variables are always fine to use whenever
appropriate. Use of bool is encouraged to improve readability and is often a
better option than 'int' for storing boolean values.

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Sean Christopherson]

On Sat, Mar 04, 2023, Wang, Wei W wrote:
> On Saturday, March 4, 2023 1:36 AM, David Matlack wrote:
> > > > On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > > > > > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > > > > > I don't get it. Why bothering the type if we just do this?
> > > > > > >
> > > > > > > diff --git a/include/linux/kvm_host.h
> > > > > > > b/include/linux/kvm_host.h index 4f26b244f6d0..10455253c6ea
> > > > > > > 100644
> > > > > > > --- a/include/linux/kvm_host.h
> > > > > > > +++ b/include/linux/kvm_host.h
> > > > > > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct
> > > > > > > kvm
> > > > > > > *kvm)
> > > > > > >
> > > > > > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > > > > > >  ({                                                           \
> > > > > > > -     int __ret = (cond);                                     \
> > > > > > > +     int __ret = !!(cond);                                   \
> > > > > >
> > > > > > This is essentially "bool __ret". No biggie to change it this way.
> > > > >
> > > > > !! will return an int, not a boolean, but it is used as a boolean.
> > > >
> > > > What's the point of defining it as an int when actually being used as a
> > Boolean?
> > > > Original returning of an 'int' is a bug in this sense. Either
> > > > returning a Boolean or the same type (length) as cond is good way to me.
> > >
> > > What's the point of using an integer? I think we need to ask the
> > > original author. But I think one of the reasons might be convenience
> > > as the return value. I am not sure if we can return a boolean in the
> > > function. But it should be fine here since it is a macro.
> > >
> > > Anyway, returning an 'int' is not a bug. The bug is the casting from
> > > 'cond' to the integer that may lose information and this is what you
> > > have captured.
> > 
> > typeof() won't work if cond is a bitfield. See commit 8d4fbcfbe0a4 ("Fix
> > WARN_ON() on bitfield ops") from Linus from back in 2007:
> 
> Yes, this seems to be a good reason for not going for typeof. Thanks for sharing.

Ya, just make __ret a bool.  I'm 99% certain I just loosely copied from WARN_ON(),
but missed the !!.

Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Isaku Yamahata]

On Wed, Mar 01, 2023 at 09:38:41PM +0800,
Wei Wang <wei.w.wang@intel.com> wrote:

> Current KVM_BUG and KVM_BUG_ON assumes that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> will be converted to 0, which is not expected. Improves the implementation
> by using !!(cond) in KVM_BUG and KVM_BUG_ON. Compared to changing 'int' to
> 'int64_t', this has less LOCs.

This changes its semantics. cond is evaluated twice. Also the return value
of KVM_BUG_ON() is changed to bool. typeof?
Perhaps return type of bool is okay, though.

Thanks,


> Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  include/linux/kvm_host.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f06635b24bd0..d77ddf82c5c8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -881,20 +881,16 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
>  
>  #define KVM_BUG(cond, kvm, fmt...)				\
>  ({								\
> -	int __ret = (cond);					\
> -								\
> -	if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))		\
> +	if (WARN_ONCE(!!cond && !(kvm)->vm_bugged, fmt))	\
>  		kvm_vm_bugged(kvm);				\
> -	unlikely(__ret);					\
> +	unlikely(!!cond);					\
>  })
>  
>  #define KVM_BUG_ON(cond, kvm)					\
>  ({								\
> -	int __ret = (cond);					\
> -								\
> -	if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))		\
> +	if (WARN_ON_ONCE(!!(cond) && !(kvm)->vm_bugged))	\
>  		kvm_vm_bugged(kvm);				\
> -	unlikely(__ret);					\
> +	unlikely(!!(cond));					\
>  })
>  
>  static inline void kvm_vcpu_srcu_read_lock(struct kvm_vcpu *vcpu)
> -- 
> 2.27.0
> 

-- 
Isaku Yamahata <isaku.yamahata@gmail.com>