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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.12.2023 14:08, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote:
>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>> --- /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?

I think so, yes. I.e. ...

>> "xor %eax, %eax" is.
> 
> GCC 13.2 (from godbolt) generates at -O2:
> 
> mov    $0x1,%eax
> ret
> 
> Which is 5 bytes long mov insn.

... at -Os I'd kind of expect the compiler to use the shorter (albeit
slower to execute) "mov $1,%al" (unless of course I'm overlooking a
specific rule in psABI).

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

Oh, of course it does. I managed to overlook that use (misguided by one
of the two files being built without being actually used).

Jan