Re: [PATCH v2 4/5] x86/livepatch: introduce a basic live patch test to gitlab CI

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2023 11:03, Roger Pau Monne wrote:
> Introduce a basic livepatch test using the interface to run self modifying
> tests.  The introduced test relies on changing a function from returning false
> to returning true.
> 
> To simplify the burden of keeping a patch that can be provided to
> livepatch-build-tools, introduce two new files: one containing the unpatched
> test functions, and another one that contains the patched forms of such
> functions.  Note that only the former is linked into the Xen image, the latter
> is built but the object file is not consumed afterwards.  Do this to assert
> that the file containing the patched functions continues to build.
> 
> Since livepatch testing will ensure that the functions are not patched 
> previous
> the applying the livepatch, allow the livepatch related tests to fail without
> tainting the hypervisor.
> 
> Note the livepatch tests are not run as part of the self modifying checks
> executed during boot, as they would obviously fail.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - New interface & test.
> ---
>  tools/misc/xen-livepatch.c          | 29 +++++++++++++++++++++++++++++
>  xen/arch/x86/Makefile               |  2 ++
>  xen/arch/x86/include/asm/test-smc.h |  2 ++
>  xen/arch/x86/setup.c                |  2 +-
>  xen/arch/x86/test-smc-lp-alt.c      | 23 +++++++++++++++++++++++
>  xen/arch/x86/test-smc-lp.c          | 23 +++++++++++++++++++++++
>  xen/arch/x86/test-smc.c             | 11 ++++++++++-
>  xen/include/public/sysctl.h         |  6 +++++-
>  8 files changed, 95 insertions(+), 3 deletions(-)
>  create mode 100644 xen/arch/x86/test-smc-lp-alt.c
>  create mode 100644 xen/arch/x86/test-smc-lp.c

Can these (and perhaps also the one file introduced earlier in the series)
perhaps become xen/arch/x86/test/smc*.c?

> --- /dev/null
> +++ b/xen/arch/x86/test-smc-lp-alt.c
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <asm/test-smc.h>
> +
> +/*
> + * Interesting case because `return false` can be encoded as an xor
> + * instruction, which is shorter than `return true` which is a mov 
> instruction,
> + * and also shorter than a jmp instruction.
> + */

I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like
"xor %eax, %eax" is.

> +bool cf_check test_lp_insn_replacement(void)

What's the purpose of the cf_check here?

Jan