Alternatives auditing for livepatches is currently broken.  To fix it, the
livepatch code needs to inspect more fields of alt_instr.

Rename ARM's fields to match x86's, because:

 * ARM already exposes alt_offset under the repl name via ALT_REPL_PTR()
 * "alt" is somewhat ambiguous in a structure entirely about alternatives
   already.
 * "repl", being the same number of character as orig leads to slightly neater
   code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

The other option is to make alt_instr an entirely common structure, but it's
already different between ARM and x86 and I'm not sure the result of doing
this would result in nicer code.
---
 xen/arch/arm/alternative.c             |  6 +++---
 xen/arch/arm/include/asm/alternative.h | 12 ++++++------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index f00e3b9b3c11..7366af4ea646 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -44,7 +44,7 @@ static bool branch_insn_requires_update(const struct alt_instr *alt,
         return true;
 
     replptr = (unsigned long)ALT_REPL_PTR(alt);
-    if ( pc >= replptr && pc <= (replptr + alt->alt_len) )
+    if ( pc >= replptr && pc <= (replptr + alt->repl_len) )
         return false;
 
     /*
@@ -128,9 +128,9 @@ static int __apply_alternatives(const struct alt_region *region,
             continue;
 
         if ( alt->cpufeature == ARM_CB_PATCH )
-            BUG_ON(alt->alt_len != 0);
+            BUG_ON(alt->repl_len != 0);
         else
-            BUG_ON(alt->alt_len != alt->orig_len);
+            BUG_ON(alt->repl_len != alt->orig_len);
 
         origptr = ALT_ORIG_PTR(alt);
         updptr = (void *)origptr + update_offset;
diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h
index 1eb4b60fbb3e..d3210e82f9e5 100644
--- a/xen/arch/arm/include/asm/alternative.h
+++ b/xen/arch/arm/include/asm/alternative.h
@@ -13,16 +13,16 @@
 
 struct alt_instr {
 	s32 orig_offset;	/* offset to original instruction */
-	s32 alt_offset;		/* offset to replacement instruction */
+	s32 repl_offset;	/* offset to replacement instruction */
 	u16 cpufeature;		/* cpufeature bit set for replacement */
 	u8  orig_len;		/* size of original instruction(s) */
-	u8  alt_len;		/* size of new instruction(s), <= orig_len */
+	u8  repl_len;		/* size of new instruction(s), <= orig_len */
 };
 
 /* Xen: helpers used by common code. */
 #define __ALT_PTR(a,f)		((void *)&(a)->f + (a)->f)
 #define ALT_ORIG_PTR(a)		__ALT_PTR(a, orig_offset)
-#define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
+#define ALT_REPL_PTR(a)		__ALT_PTR(a, repl_offset)
 
 typedef void (*alternative_cb_t)(const struct alt_instr *alt,
 				 const uint32_t *origptr, uint32_t *updptr,
@@ -90,12 +90,12 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 #include <asm/asm_defns.h>
 #include <asm/macros.h>
 
-.macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
+.macro altinstruction_entry orig_offset repl_offset feature orig_len repl_len
 	.word \orig_offset - .
-	.word \alt_offset - .
+	.word \repl_offset - .
 	.hword \feature
 	.byte \orig_len
-	.byte \alt_len
+	.byte \repl_len
 .endm
 
 .macro alternative_insn insn1, insn2, cap, enable = 1
-- 
2.30.2

On Mon, 17 Apr 2023, Andrew Cooper wrote:
> Alternatives auditing for livepatches is currently broken.  To fix it, the
> livepatch code needs to inspect more fields of alt_instr.
> 
> Rename ARM's fields to match x86's, because:
> 
>  * ARM already exposes alt_offset under the repl name via ALT_REPL_PTR()
>  * "alt" is somewhat ambiguous in a structure entirely about alternatives
>    already.
>  * "repl", being the same number of character as orig leads to slightly neater
>    code.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> The other option is to make alt_instr an entirely common structure, but it's
> already different between ARM and x86 and I'm not sure the result of doing
> this would result in nicer code.
> ---
>  xen/arch/arm/alternative.c             |  6 +++---
>  xen/arch/arm/include/asm/alternative.h | 12 ++++++------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index f00e3b9b3c11..7366af4ea646 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -44,7 +44,7 @@ static bool branch_insn_requires_update(const struct alt_instr *alt,
>          return true;
>  
>      replptr = (unsigned long)ALT_REPL_PTR(alt);
> -    if ( pc >= replptr && pc <= (replptr + alt->alt_len) )
> +    if ( pc >= replptr && pc <= (replptr + alt->repl_len) )
>          return false;
>  
>      /*
> @@ -128,9 +128,9 @@ static int __apply_alternatives(const struct alt_region *region,
>              continue;
>  
>          if ( alt->cpufeature == ARM_CB_PATCH )
> -            BUG_ON(alt->alt_len != 0);
> +            BUG_ON(alt->repl_len != 0);
>          else
> -            BUG_ON(alt->alt_len != alt->orig_len);
> +            BUG_ON(alt->repl_len != alt->orig_len);
>  
>          origptr = ALT_ORIG_PTR(alt);
>          updptr = (void *)origptr + update_offset;
> diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h
> index 1eb4b60fbb3e..d3210e82f9e5 100644
> --- a/xen/arch/arm/include/asm/alternative.h
> +++ b/xen/arch/arm/include/asm/alternative.h
> @@ -13,16 +13,16 @@
>  
>  struct alt_instr {
>  	s32 orig_offset;	/* offset to original instruction */
> -	s32 alt_offset;		/* offset to replacement instruction */
> +	s32 repl_offset;	/* offset to replacement instruction */
>  	u16 cpufeature;		/* cpufeature bit set for replacement */
>  	u8  orig_len;		/* size of original instruction(s) */
> -	u8  alt_len;		/* size of new instruction(s), <= orig_len */
> +	u8  repl_len;		/* size of new instruction(s), <= orig_len */
>  };
>  
>  /* Xen: helpers used by common code. */
>  #define __ALT_PTR(a,f)		((void *)&(a)->f + (a)->f)
>  #define ALT_ORIG_PTR(a)		__ALT_PTR(a, orig_offset)
> -#define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
> +#define ALT_REPL_PTR(a)		__ALT_PTR(a, repl_offset)
>  
>  typedef void (*alternative_cb_t)(const struct alt_instr *alt,
>  				 const uint32_t *origptr, uint32_t *updptr,
> @@ -90,12 +90,12 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>  #include <asm/asm_defns.h>
>  #include <asm/macros.h>
>  
> -.macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
> +.macro altinstruction_entry orig_offset repl_offset feature orig_len repl_len
>  	.word \orig_offset - .
> -	.word \alt_offset - .
> +	.word \repl_offset - .
>  	.hword \feature
>  	.byte \orig_len
> -	.byte \alt_len
> +	.byte \repl_len
>  .endm
>  
>  .macro alternative_insn insn1, insn2, cap, enable = 1
> -- 
> 2.30.2
>