Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 30, 2023 at 06:02:55PM +0100, Jan Beulich wrote:
> On 28.11.2023 11:03, Roger Pau Monne wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/test-smc.c
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <xen/errno.h>
> > +
> > +#include <asm/alternative.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/test-smc.h>
> > +
> > +static bool cf_check test_insn_replacement(void)
> > +{
> > +#define EXPECTED_VALUE 2
> > +    unsigned int r = ~EXPECTED_VALUE;
> 
> The compiler is permitted to elide the initializer unless ...
> 
> > +    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
> > +                   X86_FEATURE_ALWAYS, "=r"(r));
> 
> ... you use "+r" here.

I see, '=' assumes the operand is always written to, which is not the
case if alternative is not applied.

> Also (nit) there's a blank missing between that
> string and the opening parethesis. Also what's wrong with passing
> EXPECTED_VALUE in as an "i" constraint input operand?

Me not knowing enough inline assembly I think, that's what's wrong.

> > @@ -1261,6 +1269,7 @@ struct xen_sysctl {
> >          struct xen_sysctl_livepatch_op      livepatch;
> >  #if defined(__i386__) || defined(__x86_64__)
> >          struct xen_sysctl_cpu_policy        cpu_policy;
> > +        struct xen_sysctl_test_smc          smc;
> 
> Imo the field name would better be test_smc (leaving aside Stefano's comment).

Right, will see what Stefano thinks about using test_smoc.

Thanks, Roger.