On 27/05/2024 9:24 am, Jan Beulich wrote:
> On 24.05.2024 22:03, Andrew Cooper wrote:
>> * Rename __attribute_pure__ to just __pure before it gains users.
>> * Introduce __constructor which is going to be used in lib/, and is
>> unconditionally cf_check.
>> * Identify the areas of xen/bitops.h which are a mess.
>> * Introduce xen/boot-check.h as helpers for compile and boot time testing.
>> This provides a statement of the ABI, and a confirmation that
>> arch-specific
>> implementations behave as expected.
>>
>> Sadly Clang 7 and older isn't happy with the compile time checks. Skip them,
>> and just rely on the runtime checks.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
Thanks.
>
> Further remarks, though:
>
>> ---
>> xen/include/xen/bitops.h | 13 ++++++--
>> xen/include/xen/boot-check.h | 60 ++++++++++++++++++++++++++++++++++++
>> xen/include/xen/compiler.h | 3 +-
>> 3 files changed, 72 insertions(+), 4 deletions(-)
>> create mode 100644 xen/include/xen/boot-check.h
> The bulk of the changes isn't about bitops; it's just that you're intending
> to first use it for testing there. The subject prefix therefore is somewhat
> misleading.
I'll change to "Cleanup and infrastructure ahead ..." but the bitops
aspect is still reasonably important.
>> --- /dev/null
>> +++ b/xen/include/xen/boot-check.h
>> @@ -0,0 +1,60 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +/*
>> + * Helpers for boot-time checks of basic logic, including confirming that
>> + * examples which should be calculated by the compiler are.
>> + */
>> +#ifndef XEN_BOOT_CHECK_H
>> +#define XEN_BOOT_CHECK_H
Given that CONFIG_SELF_TESTS was subsequently approved, I've renamed
this file to match.
>> +
>> +#include <xen/lib.h>
>> +
>> +/* Hide a value from the optimiser. */
>> +#define HIDE(x) \
>> + ({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; })
> In principle this is a macro that could be of use elsewhere. That's also
> reflected in its entirely generic name. It therefore feels mis-placed in
> this header.
I'd forgotten that we several variations of this already. compiler.h
has both OPTIMIZER_HIDE_VAR() and RELOC_HIDE().
> Otoh though the use of "+r" is more restricting than truly
> necessary: While I'm not sure if "+g" would work, i.e. if that wouldn't
> cause issues with literals,
OPTIMIZER_HIDE_VAR() is indeed buggy using "+g", and RELOC_HIDE() even
explains how "g" tickles a bug in a compiler we probably don't care
about any more.
[Slightly out of order] the use of OPTIMIZER_HIDE_VAR() in gsi_vioapic()
is bogus AFAICT, and is actively creating the problem the commit message
says it was trying to avoid.
> pretty surely "+rm" ought to work, removing
> the strict requirement for the compiler to put a certain value in a
> register.
"+rm" would be ideal in theory, we can't use it in practice because
Clang will (still!) interpret it as "+m" and force a spill.
While that's not necessarily a problem for the SELF_TESTS, it really is
a problem in array_index_mask_nospec(), which is latently buggy even now.
If the compiler really uses the flexibility offered by
OPTIMIZER_HIDE_VAR() to spill the value, array_index_mask_nospec() has
entirely failed at its purpose.
> Assuming you may have reservations against "+g" / "+rm" (and hence the
> construct wants keeping here), maybe rename to e.g. BOOT_CHECK_HIDE()?
> Alternatively, if generalized, moving to xen/macros.h would seem
> appropriate to me.
I've moved it to macros.h (because we should consolidate around it), but
kept as "+r" for both Clang and array_index_mask_nospec() reasons.
I don't expect HIDE() is ever actually going to be used in a case where
letting the value stay in memory is a useful thing overall. But if you
still feel strongly about it, we can debate further when consolidating
the other users.
~Andrew
