SCM fixes and updates
Subsystems (3)
cover SCM fixes and updates
These are a few updates to SCM. The first two patches fix some bad code generation. The next patch saves a couple instructions on the slow path and the final patch determines the cacheline size dynamically instead of statically.
Stephen Boyd (4):
msm: scm: Mark inline asm as volatile
msm: scm: Fix improper register assignment
msm: scm: Check for interruption immediately
msm: scm: Get cacheline size from CTR
arch/arm/mach-msm/scm.c | 75 +++++++++++++++++++++++++++------------------- 1 files changed, 44 insertions(+), 31 deletions(-)
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH 1/4 msm: scm: Mark inline asm as volatile
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-msm/scm.c +2 -2
@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )
@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Hi Stephen,
Quoted 22 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 8 lines
arch/arm/mach-msm/scm.c +1
-1@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )
These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Will
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Quoted 27 lines · Will Deacon
Hi Stephen,
Quoted 24 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;
Quoted 10 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 6 lines
arch/arm/mach-msm/scm.c +1
-1mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
I'm not seeing any different code with or without this so I saw little harm in marking them as volatile. I really don't want the compiler moving them or deleting them so it seemed safer to just mark it volatile to make sure nothing happens to the smc instructions.
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
In reply to Will DeaconQuoted 25 lines · Will Deacon
Quoted 24 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;
Quoted 9 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 6 lines
arch/arm/mach-msm/scm.c +1
-1mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects. There isn't enough optimization, yet, in gcc to change the generated code in this case, so it happens to generate the correct code without it.
The second probably doesn't need it, unless we are expecting the version to change dynamically. The volatile makes the scm_get_version() function clearly a call to scm, though, so is probably useful to document the intent.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Quoted 44 lines · David Brown
Quoted 25 lines · Will Deacon
Quoted 24 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;Quoted 9 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 6 lines
arch/arm/mach-msm/scm.c +1
-1mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects. There isn't enough optimization, yet, in gcc to change the generated code in this case, so it happens to generate the correct code without it.
Quoted 4 lines
The second probably doesn't need it, unless we are expecting the version to change dynamically. The volatile makes the
scm_get_version()function clearly a call to scm, though, so is probably useful to document the intent.
If the inline asm does have side effects which are not obvious other than producing a result for the output operand then it is a good idea to add a comment to that effect. Otherwise it is always best to omit the volatile and let gcc move the inline asm around or even delete it entirely when possible.
Nicolas
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Quoted 51 lines · Nicolas Pitre
Quoted 44 lines · David Brown
Quoted 25 lines · Will Deacon
Quoted 24 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;Quoted 9 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 6 lines
arch/arm/mach-msm/scm.c +1
-1mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects. There isn't enough optimization, yet, in gcc to change the generated code in this case, so it happens to generate the correct code without it.
Quoted 4 lines
The second probably doesn't need it, unless we are expecting the version to change dynamically. The volatile makes the
scm_get_version()function clearly a call to scm, though, so is probably useful to document the intent.
Quoted 5 lines
If the inline asm does have side effects which are not obvious other than producing a result for the output operand then it is a good idea to add a comment to that effect. Otherwise it is always best to omit the volatile and let gcc move the inline asm around or even delete it entirely when possible.
Would this be better as a comment by the assembly or for the whole file or function? The entire purpose of this file is to communicate with another logical processor, so it's all about producing side effects other than just modifying the registers or the memory. Maybe a file comment briefly explaining that SCM runs in TrustZone and a short comment by each asm stating that it traps to the other logical cpu?
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Quoted 59 lines · David Brown
Quoted 51 lines · Nicolas Pitre
Quoted 44 lines · David Brown
Quoted 25 lines · Will Deacon
Quoted 24 lines · Stephen Boyd
We don't want the compiler to remove these asm statements or reorder them in any way. Mark them as volatile to be sure.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +1
-1@@ -174,7 +174,7 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asm( + asmvolatile ( __asmeq("%0", "r0") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )@@ -271,7 +271,7 @@ u32 scm_get_version(void)return version;Quoted 9 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 277
Quoted 6 lines
arch/arm/mach-msm/scm.c +1
-1mutex_lock (&scm_lock); - asm( + asmvolatile ( __asmeq("%0", "r1") __asmeq("%1", "r0")__asmeq ("%2" ,"r1" )These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects. There isn't enough optimization, yet, in gcc to change the generated code in this case, so it happens to generate the correct code without it.
Quoted 4 lines
The second probably doesn't need it, unless we are expecting the version to change dynamically. The volatile makes the
scm_get_version()function clearly a call to scm, though, so is probably useful to document the intent.Quoted 5 lines
If the inline asm does have side effects which are not obvious other than producing a result for the output operand then it is a good idea to add a comment to that effect. Otherwise it is always best to omit the volatile and let gcc move the inline asm around or even delete it entirely when possible.
Quoted 6 lines
Would this be better as a comment by the assembly or for the whole file or function? The entire purpose of this file is to communicate with another logical processor, so it's all about producing side effects other than just modifying the registers or the memory. Maybe a file comment briefly explaining that SCM runs in TrustZone and a short comment by each asm stating that it traps to the other logical cpu?
Now that I've looked more closely at the actual code, I think it is obvious enough that the volatile is needed and no extra comment should be required.
Nicolas
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
In reply to David BrownQuoted 8 lines · David Brown
Quoted 2 lines · Will Deacon
These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects.
As far as I know, volatile asm does two things:
(1) It stops the compiler from reordering the asm block with respect to other volatile statements.
(2) It prevents the compiler from optimising the block away when dataflow analysis indicates it's not required.
If side-effects need to be indicated, won't a memory clobber do the trick?
Will
Re: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Quoted 12 lines · Will Deacon
Quoted 8 lines · David Brown
Quoted 2 lines · Will Deacon
These asm blocks all have sensible looking output constraints. Why do they need to be marked volatile?
Without the volatile, the compiler is free to assume the only side effects of the asm are to modify the output registers. The volatile is needed to indicate to the compiler that the asm has other side effects.
As far as I know, volatile asm does two things:
Quoted 8 lines
(1) It stops the compiler from reordering the asm block with respect to other volatile statements.
(2) It prevents the compiler from optimising the block away when dataflow analysis indicates it's not required.
If side-effects need to be indicated, won't a memory clobber do the trick?
Per the gcc manual:
If your assembler instructions access memory in an unpredictable
fashion, add `memory' to the list of clobbered registers. This will cause GCC to not keep memory values cached in registers across the assembler instruction and not optimize stores or loads to that memory. You will also want to add the volatile' keyword if the memory affected is not listed in the inputs or outputs of the asm', as the memory' clobber does not count as a side-effect of the asm'. If you know how large the accessed memory is, you can add it as input or output but if this is not known, you should add `memory'. As an example, if you access ten bytes of a string, you can use a memory input like:
The smc instruction is similar to a syscall. When in the secure world, the processor is making state changes. It's not quite correct to declare this as memory, because the memory used when secure isn't even accessible to us. As far as I can tell, the volatile is the only way to tell the compiler this.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Will Deacon
Acked-byRe: [PATCH 1/4] msm: scm: Mark inline asm as volatile
Hi David,
Quoted 14 lines · David Brown
Per the gcc manual:
If your assembler instructions access memory in an unpredictable
fashion, add `memory' to the list of clobbered registers. This will cause GCC to not keep memory values cached in registers across the assembler instruction and not optimize stores or loads to that memory. You will also want to add thevolatile' keyword if the memory affected is not listed in the inputs or outputs of theasm', as thememory' clobber does not count as a side-effect of theasm'. If you know how large the accessed memory is, you can add it as input or output but if this is not known, you should add `memory'. As an example, if you access ten bytes of a string, you can use a memory input like:
Right, so if you neglected to check the output from the smc block then it would be a candidate for removal even with a memory clobber. Now I see why you want a volatile in there!
For what it's worth:
Will
PATCH 2/4 msm: scm: Fix improper register assignment
In reply to Stephen BoydAssign the registers used in the inline assembly immediately before the inline assembly block. This ensures the compiler doesn't optimize away dead register assignments when it shouldn't.
--- arch/arm/mach-msm/scm.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
arch/arm/mach-msm/scm.c +5 -2
@@ -264,13 +264,16 @@ u32 scm_get_version(void) {int context_id;static u32 version =-1 ; - register u32 r0 asm("r0") = 0x1 << 8; - register u32 r1 asm("r1") = (u32)&context_id; + registeru32 r0 asm("r0" ); + registeru32 r1 asm("r1" );if (version !=-1 )return version;mutex_lock (&scm_lock); + + r0 =0x1 <<8 ; + r1 = (u32)&context_id; asmvolatile ( __asmeq("%0", "r1")__asmeq ("%1" ,"r0" )
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 23 lines · Stephen Boyd
Assign the registers used in the inline assembly immediately before the inline assembly block. This ensures the compiler doesn't optimize away dead register assignments when it shouldn't.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +2
-2@@ -264,13 +264,16 @@ u32 scm_get_version(void) {int context_id;static u32 version =-1 ; - register u32 r0 asm("r0") = 0x1 << 8; - register u32 r1 asm("r1") = (u32)&context_id; + registeru32 r0 asm("r0" ); + registeru32 r1 asm("r1" );
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 279
Quoted 10 lines
arch/arm/mach-msm/scm.c +3
-0if (version !=-1 )return version;mutex_lock (&scm_lock); + + r0 =0x1 <<8 ; + r1 = (u32)&context_id; asmvolatile ( __asmeq("%0", "r1")__asmeq ("%1" ,"r0" )
Whoa, have you seen the compiler `optimise' the original assignments away? Since there is a use in the asm block, the definition shouldn't be omitted. What toolchain are you using?
Will
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 27 lines · Will Deacon
Quoted 26 lines · Stephen Boyd
Assign the registers used in the inline assembly immediately before the inline assembly block. This ensures the compiler doesn't optimize away dead register assignments when it shouldn't.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +2
-2@@ -264,13 +264,16 @@ u32 scm_get_version(void) {int context_id;static u32 version =-1 ; - register u32 r0 asm("r0") = 0x1 << 8; - register u32 r1 asm("r1") = (u32)&context_id; + registeru32 r0 asm("r0" ); + registeru32 r1 asm("r1" );if (version !=-1 )return version;
Quoted 12 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 279
Quoted 7 lines
arch/arm/mach-msm/scm.c +3
-0mutex_lock (&scm_lock); + + r0 =0x1 <<8 ; + r1 = (u32)&context_id; asmvolatile ( __asmeq("%0", "r1")__asmeq ("%1" ,"r0" )Whoa, have you seen the compiler `optimise' the original assignments away? Since there is a use in the asm block, the definition shouldn't be omitted. What toolchain are you using
Yes I've seen the r0 and r1 assignments get optimized away. I'm suspecting it's because the mutex_lock() is between the assignments and usage. I'm guessing the assignment to r0 and r1 are actually generated, but then they're optimized away because the mutex_lock() isn't inlined and thus r0 and r1 assigned to something that isn't &scm_lock can be "safely" removed (dead register assignments). I can't confirm any of this since I don't know what gcc is doing internally or even how to probe what it's doing (yeah I know I could go read gcc sources). Suggestions?
I've seen it with two different compilers so far:
gcc (Sourcery G++ Lite 2010.09-50) 4.5.1
arm-eabi-gcc (GCC) 4.4.0
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
In reply to Will DeaconQuoted 27 lines · Will Deacon
Quoted 26 lines · Stephen Boyd
Assign the registers used in the inline assembly immediately before the inline assembly block. This ensures the compiler doesn't optimize away dead register assignments when it shouldn't.
Signed-off-by Stephen Boyd<[email protected]>--- arch/arm/mach-msm/scm.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)arch/arm/mach-msm/scm.c +2
-2@@ -264,13 +264,16 @@ u32 scm_get_version(void) {int context_id;static u32 version =-1 ; - register u32 r0 asm("r0") = 0x1<< 8; - register u32 r1 asm("r1") = (u32)&context_id; + registeru32 r0 asm("r0" ); + registeru32 r1 asm("r1" );if (version !=-1 )return version;
Quoted 13 lines
arch/arm/mach-msm/scm.c u32 scm_get_version(void) · line 279
Quoted 7 lines
arch/arm/mach-msm/scm.c +3
-0mutex_lock (&scm_lock); + + r0 =0x1 <<8 ; + r1 = (u32)&context_id; asmvolatile ( __asmeq("%0", "r1")__asmeq ("%1" ,"r0" )Whoa, have you seen the compiler `optimise' the original assignments away? Since there is a use in the asm block, the definition shouldn't be omitted. What toolchain are you using?
Yeah, Stephen and I spent quite a bit of time discussing this and experimenting to figure out what the heck GCC was doing. But it kept optimizing the fake code we put in trying to force GCC to use a specific register.
My hypothesis at this point is that the "register xx asm("rx")" declarations are just for giving a symbolic name to refer to the specific register in C code. I doesn't tell GCC to reserve away the register and make sure the value is preserved. And the assignments to these said variables seem to translate to a pure "mov rx, 5" kinda instruction with no further preservation of rx either.
That's the only hypothesis I/we could come up with as to how this got optimized away.
I would be great if someone explains the exact meaning of these "register asm" declarations and the assignments in C code.
-Saravana
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 4 lines · Saravana Kannan
Yeah, Stephen and I spent quite a bit of time discussing this and experimenting to figure out what the heck GCC was doing. But it kept optimizing the fake code we put in trying to force GCC to use a specific register.
One way to look at it is that if you specify a value for r0, assign it, and then call a function, how do you expect the r0 value to be preserved? r0 will be corrupted by the called function as its used to pass arg0 and the return value.
I'm surprised the compiler didn't spit out an error.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Saravana Kannan
Yeah, Stephen and I spent quite a bit of time discussing this and experimenting to figure out what the heck GCC was doing. But it kept optimizing the fake code we put in trying to force GCC to use a specific register.
Quoted 6 lines
One way to look at it is that if you specify a value for r0, assign it, and then call a function, how do you expect the r0 value to be preserved? r0 will be corrupted by the called function as its used to pass arg0 and the return value.
I'm surprised the compiler didn't spit out an error.
The gcc docs say:
* Local register variables in specific registers do not reserve the registers, except at the point where they are used as input or output operands in an asm' statement and the asm' statement itself is not deleted. The compiler's data flow analysis is capable of determining where the specified registers contain live values, and where they are available for other uses. Stores into local register variables may be deleted when they appear to be dead according to dataflow analysis. References to local register variables may be deleted or moved or simplified.
which would suggest that it should at least detect that it can't keep the value in r0. What it seems to do is detect that the value can't be in the register, so it never bothers putting it there in the first place.
In any case, fortunately it works with the fix.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 9 lines · David Brown
Quoted 7 lines · Russell King - ARM Linux
On Fri, Feb 25, 2011 at 09:09:05PM -0800, Saravana Kannan wrote:
One way to look at it is that if you specify a value for r0, assign it, and then call a function, how do you expect the r0 value to be preserved? r0 will be corrupted by the called function as its used to pass arg0 and the return value.I'm surprised the compiler didn't spit out an error.
Me too. The compiler should have moved the content of r0 somewhere else before the function call, and restore it back into r0 if necessary before the point where the corresponding variable is used again. Or at least issue a warning if it can't do that.
Quoted 12 lines
The gcc docs say:
* Local register variables in specific registers do not reserve the registers, except at the point where they are used as input or output operands in an
asm' statement and theasm' statement itself is not deleted. The compiler's data flow analysis is capable of determining where the specified registers contain live values, and where they are available for other uses. Stores into local register variables may be deleted when they appear to be dead according to dataflow analysis. References to local register variables may be deleted or moved or simplified.
Quoted 4 lines
which would suggest that it should at least detect that it can't keep the value in r0. What it seems to do is detect that the value can't be in the register, so it never bothers putting it there in the first place.
Right. A minimal test case may look like this if someone feels like filling a gcc bug report:
extern int foo(int x);
int bar(int x)
{
register int a asm("r0") = 1;
x = foo(x);
asm ("add %0, %1, %2" : "=r" (x) : "r" (a), "r" (x));
return x;
}And the produced code is:
bar:
stmfd sp!, {r3, lr}
bl foo
#APP
add r0, r0, r0
ldmfd sp!, {r3, pc}So this is clearly bogus.
Quoted 1 line
In any case, fortunately it works with the fix.
Please add a comment in your patch to explain the issue.
Nicolas
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Hi Nicolas,
Quoted 12 lines · Nicolas Pitre
Quoted 12 lines
The gcc docs say:
* Local register variables in specific registers do not reserve the registers, except at the point where they are used as input or output operands in an
asm' statement and theasm' statement itself is not deleted. The compiler's data flow analysis is capable of determining where the specified registers contain live values, and where they are available for other uses. Stores into local register variables may be deleted when they appear to be dead according to dataflow analysis. References to local register variables may be deleted or moved or simplified.
Quoted 4 lines
Quoted 4 lines
which would suggest that it should at least detect that it can't keep the value in r0. What it seems to do is detect that the value can't be in the register, so it never bothers putting it there in the first place.
I suspect it sees the function call as a write to r0 and then somehow infers that the live range of the int r0 variable ends there. Without a use in the live range it then decides it can optimise away the definition. It really comes down to whether or not the variable is characterised by its identifier or the register in which it resides.
Quoted 15 lines
Right. A minimal test case may look like this if someone feels like filling a gcc bug report:
extern int foo(int x);
int bar(int x)
{ register int a asm("r0") = 1; x = foo(x); asm ("add %0, %1, %2" : "=r" (x) : "r" (a), "r" (x)); return x; }And the produced code is:
Quoted 9 lines
bar:
stmfd sp!, {r3, lr} bl foo #APP add r0, r0, r0 ldmfd sp!, {r3, pc}So this is clearly bogus.
I agree that this is wrong, but the compiler people may try and argue the other way. I'll ask some of the compiler guys at ARM and see what they think.
Quoted 4 lines
Quoted 1 line
In any case, fortunately it works with the fix.
Please add a comment in your patch to explain the issue.
Perhaps a more robust fix would be to remove the register int declarations and handle the parameter marshalling in the same asm block that contains the smc?
Will
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 25 lines · Will Deacon
Hi Nicolas,
Quoted 22 lines · Nicolas Pitre
Right. A minimal test case may look like this if someone feels like filling a gcc bug report:
extern int foo(int x);
int bar(int x)
{ register int a asm("r0") = 1; x = foo(x); asm ("add %0, %1, %2" : "=r" (x) : "r" (a), "r" (x)); return x; }And the produced code is:
bar:
stmfd sp!, {r3, lr} bl foo #APP add r0, r0, r0 ldmfd sp!, {r3, pc}
Quoted 6 lines
Quoted 2 lines
So this is clearly bogus.
I agree that this is wrong, but the compiler people may try and argue the other way. I'll ask some of the compiler guys at ARM and see what they think.
Nicolas and Will,
Thanks for the sample bug code and thanks for checking with the compiler guys and validating (in another thread) that this is indeed a bug in GCC. Glad to know we weren't doing something stupid.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
In any case, fortunately it works with the fix.
Please add a comment in your patch to explain the issue.
Perhaps a more robust fix would be to remove the register int declarations and handle the parameter marshalling in the same asm block that contains the smc?
I was thinking the same, but the opposing idea I heard was that not doing it inside the asm block would allow GCC to be make better use of the registers. Didn't have a strong opinion either way, so we went with the implementation that was sent out.
Thanks,
Saravana
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
Quoted 7 lines · Saravana Kannan
Quoted 3 lines · Will Deacon
Perhaps a more robust fix would be to remove the register int declarations and handle the parameter marshalling in the same asm block that contains the smc?
I was thinking the same, but the opposing idea I heard was that not doing it inside the asm block would allow GCC to be make better use of the registers.
Indeed. And a significant body of code out there does rely on this gcc feature, so it has to minimally work.
Quoted 2 lines
Didn't have a strong opinion either way, so we went with the implementation that was sent out.
ACK.
Nicolas
Re: [PATCH 2/4] msm: scm: Fix improper register assignment
In reply to Nicolas PitreNicolas,
Quoted 15 lines · Nicolas Pitre
Right. A minimal test case may look like this if someone feels like filling a gcc bug report:
extern int foo(int x);
int bar(int x)
{ register int a asm("r0") = 1; x = foo(x); asm ("add %0, %1, %2" : "=r" (x) : "r" (a), "r" (x)); return x; }And the produced code is:
Quoted 8 lines
bar:
stmfd sp!, {r3, lr} bl foo #APP add r0, r0, r0 ldmfd sp!, {r3, pc}So this is clearly bogus.
I've had a chat with the compiler guys and they confirmed that this is a known bug. There's a really hairy bug report here:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815
It looks like the GCC stance will change in the future so that register variables will only be guaranteed to live in the specified register during asm blocks which use them. If the register is required elsewhere, spill/reload code will be emitted as necessary. This might break some weird and wonderful code (passing hidden operands to functions?) but I don't think we rely on the current behaviour anywhere in the kernel.
Will
PATCH 3/4 msm: scm: Check for interruption immediately
In reply to Stephen BoydWhen we're interrupted on the secure side, we should just issue another smc instruction again instead of replaying the arguments to smc. Fix it.
--- arch/arm/mach-msm/scm.c | 51 ++++++++++++++++++++++++---------------------- 1 files changed, 27 insertions(+), 24 deletions(-)
arch/arm/mach-msm/scm.c +27 -24
@@ -174,15 +174,18 @@ static u32 smc(u32 cmd_addr) register u32 r0 asm("r0") = 1; register u32 r1 asm("r1") = (u32)&context_id; register u32 r2 asm("r2") = cmd_addr; - asmvolatile ( - __asmeq("%0", "r0") - __asmeq("%1", "r0") - __asmeq("%2", "r1") - __asmeq("%3", "r2") - "smc #0 @ switch to secure world\n" - : "=r" (r0) - : "r" (r0), "r" (r1), "r" (r2) - : "r3"); +do { + asmvolatile ( + __asmeq("%0", "r0") + __asmeq("%1", "r0") + __asmeq("%2", "r1") + __asmeq("%3", "r2") + "smc #0 @ switch to secure world\n" + : "=r" (r0) + : "r" (r0), "r" (r1), "r" (r2) + : "r3"); + }while (r0 ==SCM_INTERRUPTED ); +return r0; }
@@ -197,13 +200,9 @@ static int __scm_call(const struct scm_command *cmd)* side in the buffer. */ flush_cache_all (); -do { - ret =smc (cmd_addr); -if (ret <0 ) { - ret =scm_remap_error (ret); -break ; - } - }while (ret ==SCM_INTERRUPTED ); + ret =smc (cmd_addr); +if (ret <0 ) + ret =scm_remap_error (ret);return ret; }
@@ -274,14 +273,18 @@ u32 scm_get_version(void) r0 =0x1 <<8 ; r1 = (u32)&context_id; - asmvolatile ( - __asmeq("%0", "r1") - __asmeq("%1", "r0") - __asmeq("%2", "r1") - "smc #0 @ switch to secure world\n" - : "=r" (r1) - : "r" (r0), "r" (r1) - : "r2", "r3"); +do { + asmvolatile ( + __asmeq("%0", "r0") + __asmeq("%1", "r1") + __asmeq("%2", "r0") + __asmeq("%3", "r1") + "smc #0 @ switch to secure world\n" + : "=r" (r0), "=r" (r1) + : "r" (r0), "r" (r1) + : "r2", "r3"); + }while (r0 ==SCM_INTERRUPTED ); + version = r1;mutex_unlock (&scm_lock);
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH 4/4 msm: scm: Get cacheline size from CTR
In reply to Stephen BoydInstead of hardcoding the cacheline size as 32, get the cacheline size from the CTR register.
--- arch/arm/mach-msm/scm.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)
arch/arm/mach-msm/scm.c +12 -5
@@ -26,9 +26,6 @@#include "scm.h" -/* Cache line size for msm8x60 */ -#define CACHELINESIZE 32 -#define SCM_ENOMEM -5#define SCM_EOPNOTSUPP -4#define SCM_EINVAL_ADDR -3
@@ -207,6 +204,14 @@ static int __scm_call(const struct scm_command *cmd)return ret; } +static inline u32 dcache_line_size (void ) +{ +u32 ctr; + + asmvolatile ("mrc p15, 0, %0, c0, c0, 1" :"=r" (ctr)); +return 4 << ((ctr >>16 ) &0xf ); +} +/** * scm_call() - Send an SCM command * @svc_id: service identifier
@@ -243,11 +248,13 @@ int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len,do {u32 start = (u32 )rsp;u32 end = (u32 )scm_get_response_buffer (rsp) + resp_len; - start &= ~(CACHELINESIZE -1 ); +u32 cacheline_size =dcache_line_size (); + + start &= ~(cacheline_size -1 );while (start < end) { asm ("mcr p15, 0, %0, c7, c6, 1" : :"r" (start) :"memory" ); - start +=CACHELINESIZE ; + start += cacheline_size; } }while (!rsp->is_complete);
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
Quoted 38 lines · Stephen Boyd
Instead of hardcoding the cacheline size as 32, get the cacheline size from the CTR register.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)arch/arm/mach-msm/scm.c +8
-3@@ -26,9 +26,6 @@#include "scm.h" -/* Cache line size for msm8x60 */ -#define CACHELINESIZE 32 -#define SCM_ENOMEM -5#define SCM_EOPNOTSUPP -4#define SCM_EINVAL_ADDR -3@@ -207,6 +204,14 @@ static int __scm_call(const struct scm_command *cmd)return ret; } +static inline u32 dcache_line_size (void ) +{ +u32 ctr; + + asmvolatile ("mrc p15, 0, %0, c0, c0, 1" :"=r" (ctr)); +return 4 << ((ctr >>16 ) &0xf ); +} +/** * scm_call() - Send an SCM command * @svc_id: service identifier
arch/arm/mach-msm/scm.c int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, · line 251
Quoted 5 lines
arch/arm/mach-msm/scm.c +1
-1@@ -243,11 +248,13 @@ int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, do { u32 start = (u32)rsp; u32 end = (u32)scm_get_response_buffer(rsp) + resp_len; - start &= ~(CACHELINESIZE - 1); + u32 cacheline_size = dcache_line_size();
And why do you want to do that on every scm_call() invocation and on every loop of that code? If your dcache_line_size() changes at runtime, then you might have other problems.
Thanks,
tglx
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
Quoted 16 lines · Thomas Gleixner
arch/arm/mach-msm/scm.c int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, · line 251
Quoted 10 lines · Stephen Boyd
/**
*scm_call()- Send an SCM command
* @svc_id: service identifierarch/arm/mach-msm/scm.c +1
-1@@ -243,11 +248,13 @@ int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, do { u32 start = (u32)rsp; u32 end = (u32)scm_get_response_buffer(rsp) + resp_len; - start &= ~(CACHELINESIZE - 1); + u32 cacheline_size = dcache_line_size();And why do you want to do that on every
scm_call()invocation and on every loop of that code? If yourdcache_line_size()changes at runtime, then you might have other problems.
I definitely don't want to do it for every loop. I'm fine with getting it every scm_call() invocation though.
For now, I'll pull the end and cacheline_size variables out of the do-while loop.
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
Quoted 18 lines · Stephen Boyd
Quoted 16 lines · Thomas Gleixner
arch/arm/mach-msm/scm.c int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, · line 251
Quoted 10 lines · Stephen Boyd
/**
*scm_call()- Send an SCM command
* @svc_id: service identifierarch/arm/mach-msm/scm.c +1
-1@@ -243,11 +248,13 @@ int scm_call(u32 svc_id, u32 cmd_id, const void *cmd_buf, size_t cmd_len, do { u32 start = (u32)rsp; u32 end = (u32)scm_get_response_buffer(rsp) + resp_len; - start &= ~(CACHELINESIZE - 1); + u32 cacheline_size = dcache_line_size();And why do you want to do that on every
scm_call()invocation and on every loop of that code? If yourdcache_line_size()changes at runtime, then you might have other problems.
Quoted 5 lines
I definitely don't want to do it for every loop. I'm fine with getting it every
scm_call()invocation though.For now, I'll pull the end and
cacheline_sizevariables out of the do-while loop.
Why not do it correct right away and retrieve it in an __init function?
Thanks,
tglx
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
Quoted 11 lines · Thomas Gleixner
Quoted 6 lines · Stephen Boyd
I definitely don't want to do it for every loop. I'm fine with getting it every
scm_call()invocation though.For now, I'll pull the end and
cacheline_sizevariables out of the do-while loop.Why not do it correct right away and retrieve it in an
__initfunction?
That would require an early_initcall, so hopefully that is fine.
I wonder why the generic arm v7 cache operations don't do the same thing and store the dcache line size somewhere. Every dma operation is essentially calling dcache_line_size(). Perhaps some generic arm code should be determining the dcache line size really early on and storing it in the proc_info_list? Then both the dma code and scm code could query the processor for the dcache line size with something like cpu_dcache_line_size?
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
In reply to Stephen BoydHello.
Quoted 12 lines · Stephen Boyd
Instead of hardcoding the cacheline size as 32, get the cacheline size from the CTR register.
Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/mach-msm/scm.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)arch/arm/mach-msm/scm.c +0
-0
[...]
arch/arm/mach-msm/scm.c static int __scm_call(const struct scm_command *cmd) · line 213
Quoted 11 lines
arch/arm/mach-msm/scm.c +7
-0@@ -207,6 +204,14 @@ static int __scm_call(const struct scm_command *cmd)return ret; } +static inline u32 dcache_line_size (void ) +{ +u32 ctr; + + asmvolatile ("mrc p15, 0, %0, c0, c0, 1" :"=r" (ctr)); +return 4 << ((ctr >>16 ) &0xf ); +}
Won't generic cache_line_size() macro do instead? It's defined asL1_CACHE_BYTES.
WBR, Sergei
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
Quoted 7 lines · Sergei Shtylyov
Quoted 5 lines · Stephen Boyd
arch/arm/mach-msm/scm.c +0
-0@@ -207,6 +204,14 @@ static int __scm_call(const struct scm_command *cmd)return ret; }
Quoted 11 lines
arch/arm/mach-msm/scm.c static int __scm_call(const struct scm_command *cmd) · line 213
Quoted 7 lines
arch/arm/mach-msm/scm.c +7
-0+static inline u32 dcache_line_size (void ) +{ +u32 ctr; + + asmvolatile ("mrc p15, 0, %0, c0, c0, 1" :"=r" (ctr)); +return 4 << ((ctr >>16 ) &0xf ); +}Won't generic
cache_line_size()macro do instead? It's defined asL1_CACHE_BYTES.
Interesting. It would be the same value (32) but I'm not sure how multi-platform friendly that will be since L1_CACHE_BYTES is (1 << CONFIG_ARM_L1_CACHE_SHIFT). I suppose we can punt supporting platforms with different cache line sizes in one kernel for another day.
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 4/4] msm: scm: Get cacheline size from CTR
In reply to Sergei ShtylyovQuoted 2 lines · Sergei Shtylyov
Won't generic
cache_line_size()macro do instead? It's defined asL1_CACHE_BYTES.
L1_CACHE_BYTES needs to be a compile time constant. As such it ends up being defined to the largest cache line size for the range of CPUs built into the kernel. This allows us to appropriately align data structures to cache line boundaries which are boundaries for any of the CPUs which are to be supported.
However, if you need to know exactly what cache line size you have for doing things like cache maintainence then you can not use L1_CACHE_BYTES or anything related to that.
One of the issues which complicates decoding the cache line size is that on some CPUs, there's no way to read it. On later CPUs, there's the cache type register, of which there's several different formats which makes decoding it rather painful and complicated. Then there's the related issue as to which cache line size you want - L1 Dcache, L1 Icache, or L2 cache, or some other level of cache?
It's all rather messy.
Quoted 11 lines · Stephen Boyd
These are a few updates to SCM. The first two patches fix some bad code generation. The next patch saves a couple instructions on the slow path and the final patch determines the cacheline size dynamically instead of statically.
Stephen Boyd (4):
msm: scm: Mark inline asm as volatile
msm: scm: Fix improper register assignment
msm: scm: Check for interruption immediately
msm: scm: Get cacheline size from CTR
Can we queue up patches 1 to 3 from this series for the next window? It looks like everyone is ok with them. I'll respin the fourth patch once I figure out where to go with it.
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 12 lines · Stephen Boyd
Quoted 10 lines · Stephen Boyd
These are a few updates to SCM. The first two patches fix some bad code generation. The next patch saves a couple instructions on the slow path and the final patch determines the cacheline size dynamically instead of statically.
Stephen Boyd (4):
msm: scm: Mark inline asm as volatile
msm: scm: Fix improper register assignment
msm: scm: Check for interruption immediately
msm: scm: Get cacheline size from CTR
Quoted 3 lines
Can we queue up patches 1 to 3 from this series for the next window? It looks like everyone is ok with them. I'll respin the fourth patch once I figure out where to go with it.
Seems reasonable to me. I'll break the first three out and include them into msm-next.
Stephen Boyd (3):
msm: scm: Mark inline asm as volatile
msm: scm: Fix improper register assignment
msm: scm: Check for interruption immediately
arch/arm/mach-msm/scm.c | 58 ++++++++++++++++++++++++++--------------------- 1 files changed, 32 insertions(+), 26 deletions(-)
I have pulled these changes into my MSM tree, which can be found at
git://codeaurora.org/quic/kernel/davidb/linux-msm.git
in the for-next branch
This patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The path will hopefully also be merged in Linus' tree for the next -rc kernel release.
Thanks,
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.