Re: [PATCH v2 2/3] arm/alternatives: Rename alt_instr fields which are used in common code
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
Reviewed-by: Stefano Stabellini
> ---
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Konrad Rzeszutek Wilk
> CC: Ross Lagerwall
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
>
> 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
> #include
>
> -.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
>
[PATCH v2 2/3] arm/alternatives: Rename alt_instr fields which are used in common code
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
---
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
CC: Bertrand Marquis
CC: Konrad Rzeszutek Wilk
CC: Ross Lagerwall
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
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
#include
-.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
