I encounter the following issue when compiling 5.15-rc4 with clang-14:
In file included from arch/x86/kvm/mmu/mmu.c:27:
arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
return __is_bad_mt_xwr(rsvd_check, spte) |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||
arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
(no issue with gcc-11)
Tor
On 04/10/21 11:30, [email protected] wrote:
>
>> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
>>
>>
>> On 04/10/21 11:08, [email protected] wrote:
>>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
>>>
>>> In file included from arch/x86/kvm/mmu/mmu.c:27:
>>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
>>> return __is_bad_mt_xwr(rsvd_check, spte) |
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ||
>>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
>>
>> The warning is wrong, as mentioned in the line right above:
>
> So it's an issue with clang-14 then?
> (I add Nick and Nathan)
My clang here doesn't have the option, so I'm going to ask---are you
using W=1? I can see why clang is warning for KVM's code, but in my
opinion such a check should only be in -Wextra.
Paolo
>>
>> /*
>> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
>> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
>> * (this is extremely unlikely to be short-circuited as true).
>> */
>>
>> Paolo
>
On 04/10/21 11:08, [email protected] wrote:
> I encounter the following issue when compiling 5.15-rc4 with clang-14:
>
> In file included from arch/x86/kvm/mmu/mmu.c:27:
> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> return __is_bad_mt_xwr(rsvd_check, spte) |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ||
> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
The warning is wrong, as mentioned in the line right above:
/*
* Use a bitwise-OR instead of a logical-OR to aggregate the reserved
* bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
* (this is extremely unlikely to be short-circuited as true).
*/
Paolo
> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
>
>
> On 04/10/21 11:08, [email protected] wrote:
> > I encounter the following issue when compiling 5.15-rc4 with clang-14:
> >
> > In file included from arch/x86/kvm/mmu/mmu.c:27:
> > arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > return __is_bad_mt_xwr(rsvd_check, spte) |
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ||
> > arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
>
> The warning is wrong, as mentioned in the line right above:
So it's an issue with clang-14 then?
(I add Nick and Nathan)
>
> /*
> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> * (this is extremely unlikely to be short-circuited as true).
> */
>
> Paolo
> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:49 geschrieben:
>
>
> On 04/10/21 11:30, [email protected] wrote:
> >
> >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> >>
> >>
> >> On 04/10/21 11:08, [email protected] wrote:
> >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> >>>
> >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> ||
> >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> >>
> >> The warning is wrong, as mentioned in the line right above:
> >
> > So it's an issue with clang-14 then?
> > (I add Nick and Nathan)
>
> My clang here doesn't have the option, so I'm going to ask---are you
> using W=1? I can see why clang is warning for KVM's code, but in my
> opinion such a check should only be in -Wextra.
I don't use any options (not that I'm aware of).
Clang version 14.0.0 5f7a5353301b776ffb0e5fb048992898507bf7ee
>
> Paolo
>
> >>
> >> /*
> >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> >> * (this is extremely unlikely to be short-circuited as true).
> >> */
> >>
> >> Paolo
> >
> [email protected] hat am 04.10.2021 12:10 geschrieben:
>
>
> > Paolo Bonzini <[email protected]> hat am 04.10.2021 11:49 geschrieben:
> >
> >
> > On 04/10/21 11:30, [email protected] wrote:
> > >
> > >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> > >>
> > >>
> > >> On 04/10/21 11:08, [email protected] wrote:
> > >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> > >>>
> > >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> > >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>> ||
> > >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> > >>
> > >> The warning is wrong, as mentioned in the line right above:
> > >
> > > So it's an issue with clang-14 then?
> > > (I add Nick and Nathan)
> >
> > My clang here doesn't have the option, so I'm going to ask---are you
> > using W=1? I can see why clang is warning for KVM's code, but in my
> > opinion such a check should only be in -Wextra.
>
> I don't use any options (not that I'm aware of).
> Clang version 14.0.0 5f7a5353301b776ffb0e5fb048992898507bf7ee
Probably the cause for this bug is this recent llvm commit:
https://github.com/llvm/llvm-project/commit/f59cc9542bfb461d16ad12b2cc4be4abbfd9d96e
>
> >
> > Paolo
> >
> > >>
> > >> /*
> > >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> > >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> > >> * (this is extremely unlikely to be short-circuited as true).
> > >> */
> > >>
> > >> Paolo
> > >
On Mon, Oct 4, 2021 at 2:49 AM Paolo Bonzini <[email protected]> wrote:
>
> On 04/10/21 11:30, [email protected] wrote:
> >
> >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> >>
> >>
> >> On 04/10/21 11:08, [email protected] wrote:
> >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> >>>
> >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> ||
> >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> >>
> >> The warning is wrong, as mentioned in the line right above:
> >
> > So it's an issue with clang-14 then?
> > (I add Nick and Nathan)
>
> My clang here doesn't have the option, so I'm going to ask---are you
> using W=1? I can see why clang is warning for KVM's code, but in my
> opinion such a check should only be in -Wextra.
This is a newly added warning in top of tree clang.
>
> Paolo
>
> >>
> >> /*
> >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> >> * (this is extremely unlikely to be short-circuited as true).
> >> */
> >>
> >> Paolo
> >
>
--
Thanks,
~Nick Desaulniers
On Mon, Oct 4, 2021 at 9:13 AM Nick Desaulniers <[email protected]> wrote:
>
> On Mon, Oct 4, 2021 at 2:49 AM Paolo Bonzini <[email protected]> wrote:
> >
> > On 04/10/21 11:30, [email protected] wrote:
> > >
> > >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> > >>
> > >>
> > >> On 04/10/21 11:08, [email protected] wrote:
> > >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> > >>>
> > >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> > >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>> ||
> > >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> > >>
> > >> The warning is wrong, as mentioned in the line right above:
Casting the bool to an int doesn't seem that onerous.
> > > So it's an issue with clang-14 then?
> > > (I add Nick and Nathan)
> >
> > My clang here doesn't have the option, so I'm going to ask---are you
> > using W=1? I can see why clang is warning for KVM's code, but in my
> > opinion such a check should only be in -Wextra.
>
> This is a newly added warning in top of tree clang.
>
> >
> > Paolo
> >
> > >>
> > >> /*
> > >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> > >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> > >> * (this is extremely unlikely to be short-circuited as true).
> > >> */
> > >>
> > >> Paolo
> > >
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
On Mon, Oct 04, 2021 at 10:12:33AM -0700, Jim Mattson wrote:
> On Mon, Oct 4, 2021 at 9:13 AM Nick Desaulniers <[email protected]> wrote:
> >
> > On Mon, Oct 4, 2021 at 2:49 AM Paolo Bonzini <[email protected]> wrote:
> > >
> > > On 04/10/21 11:30, [email protected] wrote:
> > > >
> > > >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> > > >>
> > > >>
> > > >> On 04/10/21 11:08, [email protected] wrote:
> > > >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> > > >>>
> > > >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> > > >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > > >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >>> ||
> > > >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> > > >>
> > > >> The warning is wrong, as mentioned in the line right above:
>
> Casting the bool to an int doesn't seem that onerous.
Alternatively, could we just change both of the functions to return u64?
I understand that they are being used in boolean contexts only but it
seems like this would make it clear that a boolean or bitwise operator
on them is acceptable.
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index eb7b227fc6cf..0ca215bfe3a3 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -295,14 +295,14 @@ static inline u64 get_rsvd_bits(struct rsvd_bits_validate *rsvd_check, u64 pte,
return rsvd_check->rsvd_bits_mask[bit7][level-1];
}
-static inline bool __is_rsvd_bits_set(struct rsvd_bits_validate *rsvd_check,
- u64 pte, int level)
+static inline u64 __is_rsvd_bits_set(struct rsvd_bits_validate *rsvd_check,
+ u64 pte, int level)
{
return pte & get_rsvd_bits(rsvd_check, pte, level);
}
-static inline bool __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check,
- u64 pte)
+static inline u64 __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check,
+ u64 pte)
{
return rsvd_check->bad_mt_xwr & BIT_ULL(pte & 0x3f);
}
> > > > So it's an issue with clang-14 then?
> > > > (I add Nick and Nathan)
> > >
> > > My clang here doesn't have the option, so I'm going to ask---are you
> > > using W=1? I can see why clang is warning for KVM's code, but in my
> > > opinion such a check should only be in -Wextra.
> >
> > This is a newly added warning in top of tree clang.
> >
> > >
> > > Paolo
> > >
> > > >>
> > > >> /*
> > > >> * Use a bitwise-OR instead of a logical-OR to aggregate the reserved
> > > >> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> > > >> * (this is extremely unlikely to be short-circuited as true).
> > > >> */
> > > >>
> > > >> Paolo
> > > >
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
On Thu, Oct 14, 2021, Nathan Chancellor wrote:
> On Mon, Oct 04, 2021 at 10:12:33AM -0700, Jim Mattson wrote:
> > On Mon, Oct 4, 2021 at 9:13 AM Nick Desaulniers <[email protected]> wrote:
> > >
> > > On Mon, Oct 4, 2021 at 2:49 AM Paolo Bonzini <[email protected]> wrote:
> > > >
> > > > On 04/10/21 11:30, [email protected] wrote:
> > > > >
> > > > >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> > > > >>
> > > > >>
> > > > >> On 04/10/21 11:08, [email protected] wrote:
> > > > >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> > > > >>>
> > > > >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> > > > >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > > > >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >>> ||
> > > > >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> > > > >>
> > > > >> The warning is wrong, as mentioned in the line right above:
> >
> > Casting the bool to an int doesn't seem that onerous.
>
> Alternatively, could we just change both of the functions to return u64?
> I understand that they are being used in boolean contexts only but it
> seems like this would make it clear that a boolean or bitwise operator
> on them is acceptable.
If we want to fix this, my vote is for casting to an int and updating the comment
in is_rsvd_spte(). I think I'd vote to fix this? IIRC KVM has had bitwise goofs
in the past that manifested as real bugs, it would be nice to turn this on.
Or maybe add a macro to handle this? E.g.
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 7c0b09461349..38aeb4b21925 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -307,6 +307,12 @@ static inline bool __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check,
return rsvd_check->bad_mt_xwr & BIT_ULL(pte & 0x3f);
}
+/*
+ * Macro for intentional bitwise-OR of two booleans, which requires casting at
+ * least one of the results to an int to suppress -Wbitwise-instead-of-logical.
+ */
+#define BITWISE_BOOLEAN_OR(a, b) (!!((int)(a) | (int)(b)))
+
static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
u64 spte, int level)
{
@@ -315,8 +321,8 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
* bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
* (this is extremely unlikely to be short-circuited as true).
*/
- return __is_bad_mt_xwr(rsvd_check, spte) |
- __is_rsvd_bits_set(rsvd_check, spte, level);
+ return BITWISE_BOOLEAN_OR(__is_bad_mt_xwr(rsvd_check, spte),
+ __is_rsvd_bits_set(rsvd_check, spte, level));
}
static inline bool spte_can_locklessly_be_made_writable(u64 spte)
On Thu, Oct 14, 2021 at 11:31 AM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Oct 14, 2021, Nathan Chancellor wrote:
> > On Mon, Oct 04, 2021 at 10:12:33AM -0700, Jim Mattson wrote:
> > > On Mon, Oct 4, 2021 at 9:13 AM Nick Desaulniers <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 4, 2021 at 2:49 AM Paolo Bonzini <[email protected]> wrote:
> > > > >
> > > > > On 04/10/21 11:30, [email protected] wrote:
> > > > > >
> > > > > >> Paolo Bonzini <[email protected]> hat am 04.10.2021 11:26 geschrieben:
> > > > > >>
> > > > > >>
> > > > > >> On 04/10/21 11:08, [email protected] wrote:
> > > > > >>> I encounter the following issue when compiling 5.15-rc4 with clang-14:
> > > > > >>>
> > > > > >>> In file included from arch/x86/kvm/mmu/mmu.c:27:
> > > > > >>> arch/x86/kvm/mmu/spte.h:318:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
> > > > > >>> return __is_bad_mt_xwr(rsvd_check, spte) |
> > > > > >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > >>> ||
> > > > > >>> arch/x86/kvm/mmu/spte.h:318:9: note: cast one or both operands to int to silence this warning
> > > > > >>
> > > > > >> The warning is wrong, as mentioned in the line right above:
> > >
> > > Casting the bool to an int doesn't seem that onerous.
> >
> > Alternatively, could we just change both of the functions to return u64?
> > I understand that they are being used in boolean contexts only but it
> > seems like this would make it clear that a boolean or bitwise operator
> > on them is acceptable.
>
> If we want to fix this, my vote is for casting to an int and updating the comment
At the least, I think bitwise operations should only be performed on
unsigned types.
> in is_rsvd_spte(). I think I'd vote to fix this? IIRC KVM has had bitwise goofs
> in the past that manifested as real bugs, it would be nice to turn this on.
>
> Or maybe add a macro to handle this? E.g.
I think Nathan's suggestion was much cleaner. If explicit casts are
enough to silence the warning, then I think Jim's suggestion is even
better (though unsigned, not signed int).
>
> diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> index 7c0b09461349..38aeb4b21925 100644
> --- a/arch/x86/kvm/mmu/spte.h
> +++ b/arch/x86/kvm/mmu/spte.h
> @@ -307,6 +307,12 @@ static inline bool __is_bad_mt_xwr(struct rsvd_bits_validate *rsvd_check,
> return rsvd_check->bad_mt_xwr & BIT_ULL(pte & 0x3f);
> }
>
> +/*
> + * Macro for intentional bitwise-OR of two booleans, which requires casting at
> + * least one of the results to an int to suppress -Wbitwise-instead-of-logical.
> + */
> +#define BITWISE_BOOLEAN_OR(a, b) (!!((int)(a) | (int)(b)))
> +
> static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
> u64 spte, int level)
> {
> @@ -315,8 +321,8 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
> * bits and EPT's invalid memtype/XWR checks to avoid an extra Jcc
> * (this is extremely unlikely to be short-circuited as true).
> */
> - return __is_bad_mt_xwr(rsvd_check, spte) |
> - __is_rsvd_bits_set(rsvd_check, spte, level);
> + return BITWISE_BOOLEAN_OR(__is_bad_mt_xwr(rsvd_check, spte),
> + __is_rsvd_bits_set(rsvd_check, spte, level));
> }
>
> static inline bool spte_can_locklessly_be_made_writable(u64 spte)
--
Thanks,
~Nick Desaulniers
On 14/10/21 21:06, Nick Desaulniers wrote:
>> If we want to fix this, my vote is for casting to an int and updating the comment
>
> At the least, I think bitwise operations should only be performed on
> unsigned types.
This is not a bitwise operation, it's a non-short-circuiting boolean
operation. I'll apply Jim's suggestion.
Paolo
> On 14 Oct 2021, at 22:50, Paolo Bonzini <[email protected]> wrote:
>
> On 14/10/21 21:06, Nick Desaulniers wrote:
>>> If we want to fix this, my vote is for casting to an int and updating the comment
>> At the least, I think bitwise operations should only be performed on
>> unsigned types.
>
> This is not a bitwise operation, it's a non-short-circuiting boolean operation. I'll apply Jim's suggestion.
What about making it an inline function, which would require evaluation of arguments:
static __always_inline bool BITWISE_BOOLEAN_OR(bool a, bool b)
{
return a || b; // Safe here, because arguments have been evaluated
}
Suggesting that because I'm always nervous about casts in macros hiding something that the type system would otherwise catch.
Christophe
>
> Paolo
>