[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Luc Van Oostenryck]

get_alt_insn() is used to read and create ARM instructions, which
are always stored in memory in little-endian order. These values
are thus correctly converted to/from native order when processed
but the pointers used to hold the address of these instructions
are declared as for native order values.

Fix this by declaring the pointers as __le32* instead of u32* and
make the few appropriate needed changes.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 arch/arm64/kernel/alternative.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 8840c109c..56bfda8cb 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -60,7 +60,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
 
 #define align_down(x, a)	((unsigned long)(x) & ~(((unsigned long)(a)) - 1))
 
-static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
+static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnptr)
 {
 	u32 insn;
 
@@ -109,7 +109,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 {
 	struct alt_instr *alt;
 	struct alt_region *region = alt_region;
-	u32 *origptr, *replptr, *updptr;
+	__le32 *origptr, *replptr, *updptr;
 
 	for (alt = region->begin; alt < region->end; alt++) {
 		u32 insn;
@@ -122,9 +122,9 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 
 		pr_info_once("patching kernel code\n");
 
-		origptr = ALT_ORIG_PTR(alt);
-		replptr = ALT_REPL_PTR(alt);
-		updptr = use_linear_alias ? (u32 *)lm_alias(origptr) : origptr;
+		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
+		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
+		updptr = use_linear_alias ? (__le32 *)lm_alias(origptr) : origptr;
 		nr_inst = alt->alt_len / sizeof(insn);
 
 		for (i = 0; i < nr_inst; i++) {
-- 
2.13.0

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Will Deacon]

On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> get_alt_insn() is used to read and create ARM instructions, which
> are always stored in memory in little-endian order. These values
> are thus correctly converted to/from native order when processed
> but the pointers used to hold the address of these instructions
> are declared as for native order values.
> 
> Fix this by declaring the pointers as __le32* instead of u32* and
> make the few appropriate needed changes.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  arch/arm64/kernel/alternative.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 8840c109c..56bfda8cb 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -60,7 +60,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
>  
>  #define align_down(x, a)	((unsigned long)(x) & ~(((unsigned long)(a)) - 1))
>  
> -static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
> +static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnptr)
>  {
>  	u32 insn;
>  
> @@ -109,7 +109,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>  {
>  	struct alt_instr *alt;
>  	struct alt_region *region = alt_region;
> -	u32 *origptr, *replptr, *updptr;
> +	__le32 *origptr, *replptr, *updptr;
>  
>  	for (alt = region->begin; alt < region->end; alt++) {
>  		u32 insn;
> @@ -122,9 +122,9 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>  
>  		pr_info_once("patching kernel code\n");
>  
> -		origptr = ALT_ORIG_PTR(alt);
> -		replptr = ALT_REPL_PTR(alt);
> -		updptr = use_linear_alias ? (u32 *)lm_alias(origptr) : origptr;
> +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);

Why is the __force needed here?

Will

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Luc Van Oostenryck]

On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > get_alt_insn() is used to read and create ARM instructions, which
> > are always stored in memory in little-endian order. These values
> > are thus correctly converted to/from native order when processed
> > but the pointers used to hold the address of these instructions
> > are declared as for native order values.
> > 
> > Fix this by declaring the pointers as __le32* instead of u32* and
> > make the few appropriate needed changes.
> > 
> > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> 
> Why is the __force needed here?

Because of the cast to u32* in:
	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)

Of course, if this (u32*) is not really needed, then the __force
is also not needed.

And since, it seems indeed to be the case, I'll gladly sent a patch:
	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
if it suits you.

-- Luc

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Will Deacon]

On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > > get_alt_insn() is used to read and create ARM instructions, which
> > > are always stored in memory in little-endian order. These values
> > > are thus correctly converted to/from native order when processed
> > > but the pointers used to hold the address of these instructions
> > > are declared as for native order values.
> > > 
> > > Fix this by declaring the pointers as __le32* instead of u32* and
> > > make the few appropriate needed changes.
> > > 
> > > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > 
> > Why is the __force needed here?
> 
> Because of the cast to u32* in:
> 	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> 	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> 	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> 
> Of course, if this (u32*) is not really needed, then the __force
> is also not needed.
> 
> And since, it seems indeed to be the case, I'll gladly sent a patch:
> 	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> 	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> if it suits you.

Yes, please!

Will

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Mark Rutland]

On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > > get_alt_insn() is used to read and create ARM instructions, which
> > > are always stored in memory in little-endian order. These values
> > > are thus correctly converted to/from native order when processed
> > > but the pointers used to hold the address of these instructions
> > > are declared as for native order values.
> > > 
> > > Fix this by declaring the pointers as __le32* instead of u32* and
> > > make the few appropriate needed changes.
> > > 
> > > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > 
> > Why is the __force needed here?
> 
> Because of the cast to u32* in:
> 	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> 	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> 	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> 
> Of course, if this (u32*) is not really needed, then the __force
> is also not needed.
> 
> And since, it seems indeed to be the case, I'll gladly sent a patch:
> 	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> 	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> if it suits you.

Given __ALT_PTR is intended to give a pointer to A64 instructions, which
are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
__le32 * ?

Thanks,
Mark.

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Will Deacon]

On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
> On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > > > get_alt_insn() is used to read and create ARM instructions, which
> > > > are always stored in memory in little-endian order. These values
> > > > are thus correctly converted to/from native order when processed
> > > > but the pointers used to hold the address of these instructions
> > > > are declared as for native order values.
> > > > 
> > > > Fix this by declaring the pointers as __le32* instead of u32* and
> > > > make the few appropriate needed changes.
> > > > 
> > > > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > > > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > > 
> > > Why is the __force needed here?
> > 
> > Because of the cast to u32* in:
> > 	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> > 	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> > 	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > 
> > Of course, if this (u32*) is not really needed, then the __force
> > is also not needed.
> > 
> > And since, it seems indeed to be the case, I'll gladly sent a patch:
> > 	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > 	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> > if it suits you.
> 
> Given __ALT_PTR is intended to give a pointer to A64 instructions, which
> are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
> __le32 * ?

Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.

Will

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Mark Rutland]

On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote:
> On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
> > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > > > > get_alt_insn() is used to read and create ARM instructions, which
> > > > > are always stored in memory in little-endian order. These values
> > > > > are thus correctly converted to/from native order when processed
> > > > > but the pointers used to hold the address of these instructions
> > > > > are declared as for native order values.
> > > > > 
> > > > > Fix this by declaring the pointers as __le32* instead of u32* and
> > > > > make the few appropriate needed changes.
> > > > > 
> > > > > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > > > > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > > > 
> > > > Why is the __force needed here?
> > > 
> > > Because of the cast to u32* in:
> > > 	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> > > 	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> > > 	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > > 
> > > Of course, if this (u32*) is not really needed, then the __force
> > > is also not needed.
> > > 
> > > And since, it seems indeed to be the case, I'll gladly sent a patch:
> > > 	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > > 	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> > > if it suits you.
> > 
> > Given __ALT_PTR is intended to give a pointer to A64 instructions, which
> > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
> > __le32 * ?
> 
> Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.

Maybe, but that's one cast, rather than two, and matches other similar
casts from a pointer to unsigned long (e.g. the the addr cast in
__range_ok()).

Thanks,
Mark.

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Will Deacon]

On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote:
> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote:
> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > > > > On Wed, Jun 28, 2017 at 04:55:57PM +0200, Luc Van Oostenryck wrote:
> > > > > > get_alt_insn() is used to read and create ARM instructions, which
> > > > > > are always stored in memory in little-endian order. These values
> > > > > > are thus correctly converted to/from native order when processed
> > > > > > but the pointers used to hold the address of these instructions
> > > > > > are declared as for native order values.
> > > > > > 
> > > > > > Fix this by declaring the pointers as __le32* instead of u32* and
> > > > > > make the few appropriate needed changes.
> > > > > > 
> > > > > > +		origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > > > > > +		replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > > > > 
> > > > > Why is the __force needed here?
> > > > 
> > > > Because of the cast to u32* in:
> > > > 	#define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> > > > 	#define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> > > > 	#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > > > 
> > > > Of course, if this (u32*) is not really needed, then the __force
> > > > is also not needed.
> > > > 
> > > > And since, it seems indeed to be the case, I'll gladly sent a patch:
> > > > 	-#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > > > 	+#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> > > > if it suits you.
> > > 
> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which
> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
> > > __le32 * ?
> > 
> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.
> 
> Maybe, but that's one cast, rather than two, and matches other similar
> casts from a pointer to unsigned long (e.g. the the addr cast in
> __range_ok()).

Sure, but isn't it a __force cast? I'd like to avoid that if we can.

Will

[PATCH v2] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Luc Van Oostenryck]

get_alt_insn() is used to read and create ARM instructions, which
are always stored in memory in little-endian order. These values
are thus correctly converted to/from native order when processed
but the pointers used to hold the address of these instructions
are declared as for native order values.

Fix this by declaring the pointers as __le32* instead of u32* and
make the few appropriate needed changes like removing the unneeded
cast '(u32*)' in front of __ALT_PTR()'s definition.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

---
Change since v1:
- remove the unneeded cast '(u32*)' in front of __ALT_PTR()'s definition.
- remove the now unneeded forced cast to __le32* when using
  ALT_ORIG_PTR() or ALT_REPL_PTR().
- remove an unnneded cast in front of lm_alias().

 arch/arm64/kernel/alternative.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 8840c109c..6dd0a3a3e 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -28,7 +28,7 @@
 #include <asm/sections.h>
 #include <linux/stop_machine.h>
 
-#define __ALT_PTR(a,f)		(u32 *)((void *)&(a)->f + (a)->f)
+#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)
 
@@ -60,7 +60,7 @@ static bool branch_insn_requires_update(struct alt_instr *alt, unsigned long pc)
 
 #define align_down(x, a)	((unsigned long)(x) & ~(((unsigned long)(a)) - 1))
 
-static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
+static u32 get_alt_insn(struct alt_instr *alt, __le32 *insnptr, __le32 *altinsnptr)
 {
 	u32 insn;
 
@@ -109,7 +109,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 {
 	struct alt_instr *alt;
 	struct alt_region *region = alt_region;
-	u32 *origptr, *replptr, *updptr;
+	__le32 *origptr, *replptr, *updptr;
 
 	for (alt = region->begin; alt < region->end; alt++) {
 		u32 insn;
@@ -124,7 +124,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
 
 		origptr = ALT_ORIG_PTR(alt);
 		replptr = ALT_REPL_PTR(alt);
-		updptr = use_linear_alias ? (u32 *)lm_alias(origptr) : origptr;
+		updptr = use_linear_alias ? lm_alias(origptr) : origptr;
 		nr_inst = alt->alt_len / sizeof(insn);
 
 		for (i = 0; i < nr_inst; i++) {
-- 
2.13.0

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Luc Van Oostenryck]

On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote:
>> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote:
>> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
>> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
>> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
>> > > > > > +           origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
>> > > > > > +           replptr = (__le32 __force *) ALT_REPL_PTR(alt);
>> > > > >
>> > > > > Why is the __force needed here?
>> > > >
>> > > > Because of the cast to u32* in:
>> > > >         #define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
>> > > >         #define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
>> > > >         #define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
>> > > >
>> > > > Of course, if this (u32*) is not really needed, then the __force
>> > > > is also not needed.
>> > > >
>> > > > And since, it seems indeed to be the case, I'll gladly sent a patch:
>> > > >         -#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
>> > > >         +#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
>> > > > if it suits you.
>> > >
>> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which
>> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
>> > > __le32 * ?

Yes, it could.

>> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.
>>
>> Maybe, but that's one cast, rather than two, and matches other similar
>> casts from a pointer to unsigned long (e.g. the the addr cast in
>> __range_ok()).
>
> Sure, but isn't it a __force cast? I'd like to avoid that if we can.

No, once you cast to an integer all the specificities of pointers are
thrown away,
like for a const pointer and here the __bitwise underlying the __le32.
It's exactly the same fro cast from/to void*, the reasoning behind it
is something
like : "it's OK to drop all the qualifiers and such because you will anyway need
to cast the result to another pointer before being able to dereference it".

I already sent a version leaving the type as void*, without having seen your
replies here, but if you wish I can, of course, resend something for __le32*.

-- Luc

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Will Deacon]

On Thu, Jun 29, 2017 at 05:15:19PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote:
> >> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote:
> >> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
> >> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> >> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> >> > > > > > +           origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> >> > > > > > +           replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> >> > > > >
> >> > > > > Why is the __force needed here?
> >> > > >
> >> > > > Because of the cast to u32* in:
> >> > > >         #define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> >> > > >         #define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> >> > > >         #define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> >> > > >
> >> > > > Of course, if this (u32*) is not really needed, then the __force
> >> > > > is also not needed.
> >> > > >
> >> > > > And since, it seems indeed to be the case, I'll gladly sent a patch:
> >> > > >         -#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> >> > > >         +#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> >> > > > if it suits you.
> >> > >
> >> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which
> >> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
> >> > > __le32 * ?
> 
> Yes, it could.
> 
> >> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.
> >>
> >> Maybe, but that's one cast, rather than two, and matches other similar
> >> casts from a pointer to unsigned long (e.g. the the addr cast in
> >> __range_ok()).
> >
> > Sure, but isn't it a __force cast? I'd like to avoid that if we can.
> 
> No, once you cast to an integer all the specificities of pointers are
> thrown away,
> like for a const pointer and here the __bitwise underlying the __le32.
> It's exactly the same fro cast from/to void*, the reasoning behind it
> is something
> like : "it's OK to drop all the qualifiers and such because you will anyway need
> to cast the result to another pointer before being able to dereference it".

Ok, so does that mean that the __force cast in __range_ok is not needed?

> I already sent a version leaving the type as void*, without having seen your
> replies here, but if you wish I can, of course, resend something for __le32*.

I'm happy with the patch you sent, so no need to resend.

Will

[PATCH] arm64: fix endianness annotation for __apply_alternatives()/get_alt_insn()

[Luc Van Oostenryck]

On Thu, Jun 29, 2017 at 04:20:17PM +0100, Will Deacon wrote:
> On Thu, Jun 29, 2017 at 05:15:19PM +0200, Luc Van Oostenryck wrote:
> > On Thu, Jun 29, 2017 at 4:26 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > On Thu, Jun 29, 2017 at 03:22:34PM +0100, Mark Rutland wrote:
> > >> On Thu, Jun 29, 2017 at 03:19:44PM +0100, Will Deacon wrote:
> > >> > On Thu, Jun 29, 2017 at 03:13:23PM +0100, Mark Rutland wrote:
> > >> > > On Thu, Jun 29, 2017 at 03:26:47PM +0200, Luc Van Oostenryck wrote:
> > >> > > > On Thu, Jun 29, 2017 at 11:28:51AM +0100, Will Deacon wrote:
> > >> > > > > > +           origptr = (__le32 __force *) ALT_ORIG_PTR(alt);
> > >> > > > > > +           replptr = (__le32 __force *) ALT_REPL_PTR(alt);
> > >> > > > >
> > >> > > > > Why is the __force needed here?
> > >> > > >
> > >> > > > Because of the cast to u32* in:
> > >> > > >         #define ALT_ORIG_PTR(a)         __ALT_PTR(a, orig_offset)
> > >> > > >         #define ALT_REPL_PTR(a)         __ALT_PTR(a, alt_offset)
> > >> > > >         #define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > >> > > >
> > >> > > > Of course, if this (u32*) is not really needed, then the __force
> > >> > > > is also not needed.
> > >> > > >
> > >> > > > And since, it seems indeed to be the case, I'll gladly sent a patch:
> > >> > > >         -#define __ALT_PTR(a,f)          (u32 *)((void *)&(a)->f + (a)->f)
> > >> > > >         +#define __ALT_PTR(a,f)          ((void *)&(a)->f + (a)->f)
> > >> > > > if it suits you.
> > >> > >
> > >> > > Given __ALT_PTR is intended to give a pointer to A64 instructions, which
> > >> > > are in le32 format, wouldn't it make more sense for __ALT_PTR to cast to
> > >> > > __le32 * ?
> > 
> > Yes, it could.
> > 
> > >> > Might be a bit weird for ALT_REPL_PTR, which is cast to unsigned long.
> > >>
> > >> Maybe, but that's one cast, rather than two, and matches other similar
> > >> casts from a pointer to unsigned long (e.g. the the addr cast in
> > >> __range_ok()).
> > >
> > > Sure, but isn't it a __force cast? I'd like to avoid that if we can.
> > 
> > No, once you cast to an integer all the specificities of pointers are
> > thrown away,
> > like for a const pointer and here the __bitwise underlying the __le32.
> > It's exactly the same fro cast from/to void*, the reasoning behind it
> > is something
> > like : "it's OK to drop all the qualifiers and such because you will anyway need
> > to cast the result to another pointer before being able to dereference it".
> 
> Ok, so does that mean that the __force cast in __range_ok is not needed?

Indeed. 

And I just verified this with a quick check with sparse only on arch/arm64/
with and without the __force.

> > I already sent a version leaving the type as void*, without having seen your
> > replies here, but if you wish I can, of course, resend something for __le32*.
> 
> I'm happy with the patch you sent, so no need to resend.

OK.

-- Luc