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

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