Re: [PATCH v2 02/13] xen/bitops: Cleanup ahead of rearrangements

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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