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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote:
> 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@citrix.com>
> > ---
> > 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?

Yes, sure, I don't see why not.

> > --- /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

Don't we need to zero the high part of the register also?  Or since
the return type is a bool the compiler could assume it's truncated by
the caller?

> "xor %eax, %eax" is.

GCC 13.2 (from godbolt) generates at -O2:

mov    $0x1,%eax
ret

Which is 5 bytes long mov insn.

The return false case is:

xor    %eax,%eax
ret

I can adjust to mention this specific behavior.

> > +bool cf_check test_lp_insn_replacement(void)
> 
> What's the purpose of the cf_check here?

Because it's added to the array of test functions to call in
test_smc().  Doesn't it need cf_check in that case?

Thanks, Roger.