[PATCH 0/3] x86/alt: Simplify nops handling

[PATCH 0/3] x86/alt: Simplify nops handling

[Andrew Cooper]

Mostly patch 2, with tidyup either side

Andrew Cooper (3):
  x86/alternatives: Factor out access to ideal_nops[]
  x86/alternatives: Rework get_ideal_nops()
  x86/alternatives: Introduce init_or_livepatch_ro_after_init

 xen/arch/x86/alternative.c  | 51 +++++++++++++++----------------------
 xen/include/xen/livepatch.h |  2 ++
 2 files changed, 23 insertions(+), 30 deletions(-)


base-commit: 1f75bd375d0757d6646ca2029a9559d535f6b511
-- 
2.39.5

[PATCH 1/3] x86/alternatives: Factor out access to ideal_nops[]

[Andrew Cooper]

... in order to rework the calculation.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/alternative.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ecc56964bd9c..cc2d0c89aca3 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -86,6 +86,11 @@ static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
 # define toolchain_nops_are_ideal false
 #endif
 
+static const unsigned char *get_ideal_nops(unsigned int noplen)
+{
+    return ideal_nops[noplen];
+}
+
 static void __init arch_init_ideal_nops(void)
 {
     switch ( boot_cpu_data.x86_vendor )
@@ -116,7 +121,7 @@ static void __init arch_init_ideal_nops(void)
     }
 
 #ifdef HAVE_AS_NOPS_DIRECTIVE
-    if ( memcmp(ideal_nops[ASM_NOP_MAX], toolchain_nops, ASM_NOP_MAX) == 0 )
+    if ( memcmp(get_ideal_nops(ASM_NOP_MAX), toolchain_nops, ASM_NOP_MAX) == 0 )
         toolchain_nops_are_ideal = true;
 #endif
 }
@@ -127,9 +132,11 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
     while ( len > 0 )
     {
         unsigned int noplen = len;
+
         if ( noplen > ASM_NOP_MAX )
             noplen = ASM_NOP_MAX;
-        memcpy(insns, ideal_nops[noplen], noplen);
+
+        memcpy(insns, get_ideal_nops(noplen), noplen);
         insns += noplen;
         len -= noplen;
     }
-- 
2.39.5

Re: [PATCH 1/3] x86/alternatives: Factor out access to ideal_nops[]

[Jan Beulich]

On 22.05.2025 17:00, Andrew Cooper wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -86,6 +86,11 @@ static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
>  # define toolchain_nops_are_ideal false
>  #endif
>  
> +static const unsigned char *get_ideal_nops(unsigned int noplen)

Right here this wants to be init_or_livepatch. I didn't go check whether
subsequent patches add a truly non-init caller. If so - fine as is;
otherwise with the attribute added:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH 2/3] x86/alternatives: Rework get_ideal_nops()

[Andrew Cooper]

The {k8,p6}_nops[] arrays are both 80-byte structures indexing 45-byte
structures.  Furthermore, perhaps unusually for C, the source layout is an
obvious hint about the trangular nature of the structure.

Therefore, we can replace the pointer chase with some simple arithmatic.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

The implemenation of get_ideal_nops() changes from:

    mov    0x19bc41(%rip),%rax        # <ideal_nops>
    mov    %edi,%edi
    mov    (%rax,%rdi,8),%rax
    jmp    <__x86_return_thunk>

to:

    lea    -0x1(%rdi),%eax
    imul   %edi,%eax
    shr    %eax
    add    0x67fc1(%rip),%rax        # <ideal_nops>
    jmp    <__x86_return_thunk>

The imul has a latency of 3 cycles on all CPUs back to the K8 and Nehalem.
It's better than an extra deference on all CPUs, even the older ones.
---
 xen/arch/x86/alternative.c | 40 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index cc2d0c89aca3..ff7d83c0ddbd 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -20,7 +20,7 @@
 #define MAX_PATCH_LEN (255-1)
 
 #ifdef K8_NOP1
-static const unsigned char k8nops[] init_or_livepatch_const = {
+static const unsigned char k8_nops[] init_or_livepatch_const = {
     K8_NOP1,
     K8_NOP2,
     K8_NOP3,
@@ -31,22 +31,10 @@ static const unsigned char k8nops[] init_or_livepatch_const = {
     K8_NOP8,
     K8_NOP9,
 };
-static const unsigned char * const k8_nops[ASM_NOP_MAX+1] init_or_livepatch_constrel = {
-    NULL,
-    k8nops,
-    k8nops + 1,
-    k8nops + 1 + 2,
-    k8nops + 1 + 2 + 3,
-    k8nops + 1 + 2 + 3 + 4,
-    k8nops + 1 + 2 + 3 + 4 + 5,
-    k8nops + 1 + 2 + 3 + 4 + 5 + 6,
-    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7,
-    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8,
-};
 #endif
 
 #ifdef P6_NOP1
-static const unsigned char p6nops[] init_or_livepatch_const = {
+static const unsigned char p6_nops[] init_or_livepatch_const = {
     P6_NOP1,
     P6_NOP2,
     P6_NOP3,
@@ -57,21 +45,9 @@ static const unsigned char p6nops[] init_or_livepatch_const = {
     P6_NOP8,
     P6_NOP9,
 };
-static const unsigned char * const p6_nops[ASM_NOP_MAX+1] init_or_livepatch_constrel = {
-    NULL,
-    p6nops,
-    p6nops + 1,
-    p6nops + 1 + 2,
-    p6nops + 1 + 2 + 3,
-    p6nops + 1 + 2 + 3 + 4,
-    p6nops + 1 + 2 + 3 + 4 + 5,
-    p6nops + 1 + 2 + 3 + 4 + 5 + 6,
-    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7,
-    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8,
-};
 #endif
 
-static const unsigned char * const *ideal_nops init_or_livepatch_data = p6_nops;
+static const unsigned char *ideal_nops init_or_livepatch_data = p6_nops;
 
 #ifdef HAVE_AS_NOPS_DIRECTIVE
 
@@ -86,9 +62,17 @@ static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
 # define toolchain_nops_are_ideal false
 #endif
 
+/*
+ * Both k8_nops[] and p6_nops[] are flattened triangular data structures,
+ * making the offsets easy to calculate.
+ *
+ * To get the start of NOP $N, we want to calculate T($N - 1)
+ */
 static const unsigned char *get_ideal_nops(unsigned int noplen)
 {
-    return ideal_nops[noplen];
+    unsigned int offset = ((noplen - 1) * noplen) / 2;
+
+    return &ideal_nops[offset];
 }
 
 static void __init arch_init_ideal_nops(void)
-- 
2.39.5

Re: [PATCH 2/3] x86/alternatives: Rework get_ideal_nops()

[Jan Beulich]

On 22.05.2025 17:00, Andrew Cooper wrote:
> The {k8,p6}_nops[] arrays are both 80-byte structures indexing 45-byte
> structures.  Furthermore, perhaps unusually for C, the source layout is an
> obvious hint about the trangular nature of the structure.
> 
> Therefore, we can replace the pointer chase with some simple arithmatic.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The implemenation of get_ideal_nops() changes from:
> 
>     mov    0x19bc41(%rip),%rax        # <ideal_nops>
>     mov    %edi,%edi
>     mov    (%rax,%rdi,8),%rax
>     jmp    <__x86_return_thunk>
> 
> to:
> 
>     lea    -0x1(%rdi),%eax
>     imul   %edi,%eax
>     shr    %eax
>     add    0x67fc1(%rip),%rax        # <ideal_nops>
>     jmp    <__x86_return_thunk>
> 
> The imul has a latency of 3 cycles on all CPUs back to the K8 and Nehalem.
> It's better than an extra deference on all CPUs, even the older ones.

While this is all good, what we're losing is ...

> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -20,7 +20,7 @@
>  #define MAX_PATCH_LEN (255-1)
>  
>  #ifdef K8_NOP1
> -static const unsigned char k8nops[] init_or_livepatch_const = {
> +static const unsigned char k8_nops[] init_or_livepatch_const = {
>      K8_NOP1,
>      K8_NOP2,
>      K8_NOP3,
> @@ -31,22 +31,10 @@ static const unsigned char k8nops[] init_or_livepatch_const = {
>      K8_NOP8,
>      K8_NOP9,
>  };
> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] init_or_livepatch_constrel = {

... the (at least visual) connection to ASM_NOP_MAX. Could I talk you into
adding build time array-size checks for both arrays, to restore the
connection?

> -    NULL,
> -    k8nops,
> -    k8nops + 1,
> -    k8nops + 1 + 2,
> -    k8nops + 1 + 2 + 3,
> -    k8nops + 1 + 2 + 3 + 4,
> -    k8nops + 1 + 2 + 3 + 4 + 5,
> -    k8nops + 1 + 2 + 3 + 4 + 5 + 6,
> -    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7,
> -    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8,
> -};
>  #endif
>  
>  #ifdef P6_NOP1
> -static const unsigned char p6nops[] init_or_livepatch_const = {
> +static const unsigned char p6_nops[] init_or_livepatch_const = {
>      P6_NOP1,
>      P6_NOP2,
>      P6_NOP3,
> @@ -57,21 +45,9 @@ static const unsigned char p6nops[] init_or_livepatch_const = {
>      P6_NOP8,
>      P6_NOP9,
>  };
> -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] init_or_livepatch_constrel = {
> -    NULL,
> -    p6nops,
> -    p6nops + 1,
> -    p6nops + 1 + 2,
> -    p6nops + 1 + 2 + 3,
> -    p6nops + 1 + 2 + 3 + 4,
> -    p6nops + 1 + 2 + 3 + 4 + 5,
> -    p6nops + 1 + 2 + 3 + 4 + 5 + 6,
> -    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7,
> -    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8,
> -};
>  #endif
>  
> -static const unsigned char * const *ideal_nops init_or_livepatch_data = p6_nops;
> +static const unsigned char *ideal_nops init_or_livepatch_data = p6_nops;
>  
>  #ifdef HAVE_AS_NOPS_DIRECTIVE
>  
> @@ -86,9 +62,17 @@ static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
>  # define toolchain_nops_are_ideal false
>  #endif
>  
> +/*
> + * Both k8_nops[] and p6_nops[] are flattened triangular data structures,
> + * making the offsets easy to calculate.
> + *
> + * To get the start of NOP $N, we want to calculate T($N - 1)
> + */
>  static const unsigned char *get_ideal_nops(unsigned int noplen)
>  {
> -    return ideal_nops[noplen];
> +    unsigned int offset = ((noplen - 1) * noplen) / 2;
> +
> +    return &ideal_nops[offset];
>  }
>  
>  static void __init arch_init_ideal_nops(void)

[PATCH 3/3] x86/alternatives: Introduce init_or_livepatch_ro_after_init

[Andrew Cooper]

... and use it for ideal_nops and toolchain_nops_are_ideal; both of which are
invariant after arch_init_ideal_nops() has run.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/alternative.c  | 4 ++--
 xen/include/xen/livepatch.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ff7d83c0ddbd..058a8b22d41f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -47,7 +47,7 @@ static const unsigned char p6_nops[] init_or_livepatch_const = {
 };
 #endif
 
-static const unsigned char *ideal_nops init_or_livepatch_data = p6_nops;
+static const unsigned char *ideal_nops init_or_livepatch_ro_after_init = p6_nops;
 
 #ifdef HAVE_AS_NOPS_DIRECTIVE
 
@@ -56,7 +56,7 @@ asm ( ".pushsection .init.rodata, \"a\", @progbits\n\t"
       "toolchain_nops: .nops " __stringify(ASM_NOP_MAX) "\n\t"
       ".popsection\n\t");
 extern char toolchain_nops[ASM_NOP_MAX];
-static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
+static bool init_or_livepatch_ro_after_init toolchain_nops_are_ideal;
 
 #else
 # define toolchain_nops_are_ideal false
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index d074a5bebecc..62f8db2b55b4 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -29,6 +29,7 @@ struct xen_sysctl_livepatch_op;
 #define init_or_livepatch_constrel
 #define init_or_livepatch_data
 #define init_or_livepatch_read_mostly __read_mostly
+#define init_or_livepatch_ro_after_init __ro_after_init
 #define init_or_livepatch
 
 /* Convenience define for printk. */
@@ -153,6 +154,7 @@ void revert_payload_tail(struct payload *data);
 #define init_or_livepatch_constrel    __initconstrel
 #define init_or_livepatch_data        __initdata
 #define init_or_livepatch_read_mostly __initdata
+#define init_or_livepatch_ro_after_init __initdata
 #define init_or_livepatch             __init
 
 static inline int livepatch_op(struct xen_sysctl_livepatch_op *op)
-- 
2.39.5

Re: [PATCH 3/3] x86/alternatives: Introduce init_or_livepatch_ro_after_init

[Jan Beulich]

On 22.05.2025 17:00, Andrew Cooper wrote:
> ... and use it for ideal_nops and toolchain_nops_are_ideal; both of which are
> invariant after arch_init_ideal_nops() has run.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
preferably with ...

> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -47,7 +47,7 @@ static const unsigned char p6_nops[] init_or_livepatch_const = {
>  };
>  #endif
>  
> -static const unsigned char *ideal_nops init_or_livepatch_data = p6_nops;
> +static const unsigned char *ideal_nops init_or_livepatch_ro_after_init = p6_nops;

... the attribute moved to between type and identifier, as we generally have it
elsewhere.

Jan