[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

[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

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

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)

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

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

[Andrew Cooper]

On 02/06/2025 10:57 am, Jan Beulich wrote:
> On 22.05.2025 17:00, Andrew Cooper wrote:
>> 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?

Sorry, but I have no idea what you're asking for here.

The use of ASM_NOP_MAX was latently buggy before; it was easy to create
a NULL deference if the initialiser wasn't filled in when ASM_NOP_MAX
changed.

~Andrew

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

[Jan Beulich]

On 04.08.2026 19:13, Andrew Cooper wrote:
> On 02/06/2025 10:57 am, Jan Beulich wrote:
>> On 22.05.2025 17:00, Andrew Cooper wrote:
>>> --- 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?
> 
> Sorry, but I have no idea what you're asking for here.

    BUILD_BUG_ON(ARRAY_SIZE(k8_nops) != ASM_NOP_MAX);
    BUILD_BUG_ON(ARRAY_SIZE(p6_nops) != ASM_NOP_MAX);

> The use of ASM_NOP_MAX was latently buggy before; it was easy to create
> a NULL deference if the initialiser wasn't filled in when ASM_NOP_MAX
> changed.

Partly, yes. But why make it worse when it can be made at least somewhat
better? Omitted inner entries are reasonably easy to spot. Omitted trailing
entries aren't, hence why even in the original code omitting the array
dimension in the definitions and instead having such BUILD_BUG_ON()s would
have been more robust.

Also note how I said "(at least visual)" - by adding the BUILD_BUG_ON()s,
grep-ing for ASM_NOP_MAX will hit here, providing links to the controlled
arrays. Personally I consider it entirely plausible to possibly bump
ASM_NOP_MAX, as technically we could go up to 15. (Whether going that far
is efficient is a separate question.)

Jan

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

[Andrew Cooper]

On 05/08/2026 7:04 am, Jan Beulich wrote:
> On 04.08.2026 19:13, Andrew Cooper wrote:
>> On 02/06/2025 10:57 am, Jan Beulich wrote:
>>> On 22.05.2025 17:00, Andrew Cooper wrote:
>>>> --- 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?
>> Sorry, but I have no idea what you're asking for here.
>     BUILD_BUG_ON(ARRAY_SIZE(k8_nops) != ASM_NOP_MAX);
>     BUILD_BUG_ON(ARRAY_SIZE(p6_nops) != ASM_NOP_MAX);

The arrays are 45 bytes (and elements) long.  ASM_NOP_MAX is 9.

>
>> The use of ASM_NOP_MAX was latently buggy before; it was easy to create
>> a NULL deference if the initialiser wasn't filled in when ASM_NOP_MAX
>> changed.
> Partly, yes. But why make it worse when it can be made at least somewhat
> better?

On the contrary, I've removed an incorrect (and ineffective) attempt to
tie to ASM_NOP_MAX, and consider this form better than what was there
before.

~Andrew

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

[Jan Beulich]

On 05.08.2026 08:34, Andrew Cooper wrote:
> On 05/08/2026 7:04 am, Jan Beulich wrote:
>> On 04.08.2026 19:13, Andrew Cooper wrote:
>>> On 02/06/2025 10:57 am, Jan Beulich wrote:
>>>> On 22.05.2025 17:00, Andrew Cooper wrote:
>>>>> --- 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?
>>> Sorry, but I have no idea what you're asking for here.
>>     BUILD_BUG_ON(ARRAY_SIZE(k8_nops) != ASM_NOP_MAX);
>>     BUILD_BUG_ON(ARRAY_SIZE(p6_nops) != ASM_NOP_MAX);
> 
> The arrays are 45 bytes (and elements) long.  ASM_NOP_MAX is 9.

Oh, right, sorry:

     BUILD_BUG_ON(ARRAY_SIZE(k8_nops) != (ASM_NOP_MAX * (ASM_NOP_MAX + 1)) / 2);
     BUILD_BUG_ON(ARRAY_SIZE(p6_nops) != (ASM_NOP_MAX * (ASM_NOP_MAX + 1)) / 2);

Jan

[PATCH v2 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 triangular nature of the structure.

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

No functional change.

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

v2:
 * Add build assertion.
---
 xen/arch/x86/alternative.c | 48 ++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index f0644055d3d3..30c5ccaa8b16 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,19 @@ static bool init_or_livepatch_read_mostly toolchain_nops_are_ideal;
 # define toolchain_nops_are_ideal false
 #endif
 
+#define TRIANGLE(x) (((x) * ((x) + 1)) / 2)
+
+/*
+ * 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 TRIANGLE($N - 1)
+ */
 static const unsigned char *init_or_livepatch get_ideal_nops(unsigned int noplen)
 {
-    return ideal_nops[noplen];
+    unsigned int offset = TRIANGLE(noplen - 1);
+
+    return &ideal_nops[offset];
 }
 
 static void __init arch_init_ideal_nops(void)
@@ -601,3 +587,9 @@ void __init boot_apply_alt_calls(void)
     _alternative_instructions(ALT_CALLS);
     local_irq_enable();
 }
+
+static void __init __maybe_unused build_assertions(void)
+{
+    BUILD_BUG_ON(ARRAY_SIZE(k8_nops) != TRIANGLE(ASM_NOP_MAX));
+    BUILD_BUG_ON(ARRAY_SIZE(p6_nops) != TRIANGLE(ASM_NOP_MAX));
+}
-- 
2.34.1

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

[Jan Beulich]

On 05.08.2026 09:53, 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 triangular nature of the structure.
> 
> Therefore, we can replace the pointer chase with some simple arithmetic.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>