Ensure at build time the feature(s) used for the alternative blocks are in
range of the featureset.

No functional change intended, as all current usages are correct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/include/asm/alternative-asm.h | 3 +++
 xen/arch/x86/include/asm/alternative.h     | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/xen/arch/x86/include/asm/alternative-asm.h b/xen/arch/x86/include/asm/alternative-asm.h
index 4092f5ba70a6..83e8594f0eaf 100644
--- a/xen/arch/x86/include/asm/alternative-asm.h
+++ b/xen/arch/x86/include/asm/alternative-asm.h
@@ -12,6 +12,9 @@
  * instruction. See apply_alternatives().
  */
 .macro altinstruction_entry orig, repl, feature, orig_len, repl_len, pad_len
+    .if \feature >= NCAPINTS * 32
+        .error "alternative feature outside of featureset range"
+    .endif
     .long \orig - .
     .long \repl - .
     .word \feature
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index 69555d781ef9..b7f155994b2c 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -7,6 +7,7 @@
 #include <xen/lib.h>
 #include <xen/stringify.h>
 #include <asm/asm-macros.h>
+#include <asm/cpufeatureset.h>
 
 struct __packed alt_instr {
     int32_t  orig_offset;   /* original instruction */
@@ -59,6 +60,9 @@ extern void alternative_branches(void);
                     alt_repl_len(n2)) "-" alt_orig_len)
 
 #define ALTINSTR_ENTRY(feature, num)                                    \
+        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\
+        " .error \"alternative feature outside of featureset range\"\n" \
+        " .endif\n"                                                     \
         " .long .LXEN%=_orig_s - .\n"             /* label           */ \
         " .long " alt_repl_s(num)" - .\n"         /* new instruction */ \
         " .word " __stringify(feature) "\n"       /* feature bit     */ \
-- 
2.46.0

On 25.09.2024 10:42, Roger Pau Monne wrote:
> Ensure at build time the feature(s) used for the alternative blocks are in
> range of the featureset.
> 
> No functional change intended, as all current usages are correct.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I'm struggling with what this is meant to guard against. All validly usable
constants are within range. Any unsuitable constant can of course have any
value, yet you'd then refuse only those which are out of bounds.

Jan

On Wed, Sep 25, 2024 at 11:04:45AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, Roger Pau Monne wrote:
> > Ensure at build time the feature(s) used for the alternative blocks are in
> > range of the featureset.
> > 
> > No functional change intended, as all current usages are correct.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'm struggling with what this is meant to guard against. All validly usable
> constants are within range. Any unsuitable constant can of course have any
> value, yet you'd then refuse only those which are out of bounds.

It's IMO better than nothing, and it's a build-time check.

Thanks, Roger.

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
> index 69555d781ef9..b7f155994b2c 100644
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -7,6 +7,7 @@
>  #include <xen/lib.h>
>  #include <xen/stringify.h>
>  #include <asm/asm-macros.h>
> +#include <asm/cpufeatureset.h>
>  
>  struct __packed alt_instr {
>      int32_t  orig_offset;   /* original instruction */
> @@ -59,6 +60,9 @@ extern void alternative_branches(void);
>                      alt_repl_len(n2)) "-" alt_orig_len)
>  
>  #define ALTINSTR_ENTRY(feature, num)                                    \
> +        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\

Please use STR() from macros.h.  It's shorter, and we're trying to
retire the use of __stringify().

Happy to fix on commit.  Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

On Wed, Sep 25, 2024 at 09:51:36AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
> > index 69555d781ef9..b7f155994b2c 100644
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -7,6 +7,7 @@
> >  #include <xen/lib.h>
> >  #include <xen/stringify.h>
> >  #include <asm/asm-macros.h>
> > +#include <asm/cpufeatureset.h>
> >  
> >  struct __packed alt_instr {
> >      int32_t  orig_offset;   /* original instruction */
> > @@ -59,6 +60,9 @@ extern void alternative_branches(void);
> >                      alt_repl_len(n2)) "-" alt_orig_len)
> >  
> >  #define ALTINSTR_ENTRY(feature, num)                                    \
> > +        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\
> 
> Please use STR() from macros.h.  It's shorter, and we're trying to
> retire the use of __stringify().

Oh, yes, indeed.  I just copied from the surrounding context and
forgot about STR().

> Happy to fix on commit.  Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Sure, thanks.

Roger.