[PATCH 0/4] arm: allign check_conditional_instr() with ARM64 requirements

[PATCH 0/4] arm: allign check_conditional_instr() with ARM64 requirements

[Volodymyr Babchuk]

Hello all,

Julien asked me to take a look at check_conditional_instr() in traps.c,
because it incorrectly handled SMC32 on ARMv8 architecture.

This patch series brings it into accordance with TRM (at least, I hope so).


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/4] arm: processor: rename iss to res0 in hsr_cond union

[Volodymyr Babchuk]

Name "iss" in this case was used not exactly correctly, because this
is only part of HSR.ISS field. ARM refence manual denotes this
part of ISS as RES0 when it describes encoding for conditional
exceptions.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/asm-arm/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 855ded1..f640d54 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -434,7 +434,7 @@ union hsr {
 
     /* Common to all conditional exception classes (0x0N, except 0x00). */
     struct hsr_cond {
-        unsigned long iss:20;  /* Instruction Specific Syndrome */
+        unsigned long res0:20;  /* Reserved */
         unsigned long cc:4;    /* Condition Code */
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] arm: processor: rename iss to res0 in hsr_cond union

[Julien Grall]

Hi,

On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
> Name "iss" in this case was used not exactly correctly, because this
> is only part of HSR.ISS field. ARM refence manual denotes this

s/refence/reference/

> part of ISS as RES0 when it describes encoding for conditional
> exceptions.

When you mention the ARM ARM, please mention the version of the manual 
and paragraph. At the moment looking at the ARM v7 (ARM DDI 0406C.c) 
B3.13.6, this is not true.

Cheers,

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/include/asm-arm/processor.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 855ded1..f640d54 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -434,7 +434,7 @@ union hsr {
>   
>       /* Common to all conditional exception classes (0x0N, except 0x00). */
>       struct hsr_cond {
> -        unsigned long iss:20;  /* Instruction Specific Syndrome */
> +        unsigned long res0:20;  /* Reserved */
>           unsigned long cc:4;    /* Condition Code */
>           unsigned long ccvalid:1;/* CC Valid */
>           unsigned long len:1;   /* Instruction length */
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/4] arm: processor: add ccknownpass field into hsr_cond union

[Volodymyr Babchuk]

On ARMv8, one of conditional exceptions (SMC that originates
from aarch32 state) have extra field in HCR.ISS encoding:

CCKNOWNPASS, bit [19]
Indicates whether the instruction might have failed its condition
code check.
   0 - The instruction was unconditional, or was conditional and
   passed  its condition code check.
   1 - The instruction was conditional, and might have failed its
   condition code check.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/asm-arm/processor.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index f640d54..0131e66 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -434,7 +434,8 @@ union hsr {
 
     /* Common to all conditional exception classes (0x0N, except 0x00). */
     struct hsr_cond {
-        unsigned long res0:20;  /* Reserved */
+        unsigned long res0:19;  /* Reserved */
+        unsigned long ccknownpass:1; /* Instruction passed conditional check */
         unsigned long cc:4;    /* Condition Code */
         unsigned long ccvalid:1;/* CC Valid */
         unsigned long len:1;   /* Instruction length */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] arm: processor: add ccknownpass field into hsr_cond union

[Julien Grall]

Hi,

On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
> On ARMv8, one of conditional exceptions (SMC that originates
> from aarch32 state) have extra field in HCR.ISS encoding:
> 
> CCKNOWNPASS, bit [19]
> Indicates whether the instruction might have failed its condition
> code check.
>     0 - The instruction was unconditional, or was conditional and
>     passed  its condition code check.
>     1 - The instruction was conditional, and might have failed its
>     condition code check.

Please mention the ARM ARM version and paragraph.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/include/asm-arm/processor.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index f640d54..0131e66 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -434,7 +434,8 @@ union hsr {
>   
>       /* Common to all conditional exception classes (0x0N, except 0x00). */

When I read this comment, I understand that hsr_cond contains common 
bits for all condition exception. However, with your changes this is not 
true at all.

>       struct hsr_cond {
> -        unsigned long res0:20;  /* Reserved */
> +        unsigned long res0:19;  /* Reserved */
> +        unsigned long ccknownpass:1; /* Instruction passed conditional check */
>           unsigned long cc:4;    /* Condition Code */
>           unsigned long ccvalid:1;/* CC Valid */
>           unsigned long len:1;   /* Instruction length */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] arm: processor: add ccknownpass field into hsr_cond union

[Julien Grall]

On 07/28/2017 09:31 PM, Julien Grall wrote:
> Hi,
> 
> On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
>> On ARMv8, one of conditional exceptions (SMC that originates
>> from aarch32 state) have extra field in HCR.ISS encoding:
>>
>> CCKNOWNPASS, bit [19]
>> Indicates whether the instruction might have failed its condition
>> code check.
>>     0 - The instruction was unconditional, or was conditional and
>>     passed  its condition code check.
>>     1 - The instruction was conditional, and might have failed its
>>     condition code check.
> 
> Please mention the ARM ARM version and paragraph.
> 
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/include/asm-arm/processor.h | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/processor.h 
>> b/xen/include/asm-arm/processor.h
>> index f640d54..0131e66 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -434,7 +434,8 @@ union hsr {
>>       /* Common to all conditional exception classes (0x0N, except 
>> 0x00). */
> 
> When I read this comment, I understand that hsr_cond contains common 
> bits for all condition exception. However, with your changes this is not 
> true at all.

To complete here. I think you should directly use hsr.smc.ccknownpass 
rather than hsr.cond in patch #4 given that this field is SMC specific.

> 
>>       struct hsr_cond {
>> -        unsigned long res0:20;  /* Reserved */
>> +        unsigned long res0:19;  /* Reserved */
>> +        unsigned long ccknownpass:1; /* Instruction passed 
>> conditional check */
>>           unsigned long cc:4;    /* Condition Code */
>>           unsigned long ccvalid:1;/* CC Valid */
>>           unsigned long len:1;   /* Instruction length */
>>
> 
> Cheers,
> 


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/4] arm: traps: handle unknown exceptions in check_conditional_instr()

[Volodymyr Babchuk]

According to ARM architecture reference manual, exception with
unknown reason (HSR.EC == 0) have no valid bits in HSR
(apart from HSR.EC), so we can't check if that was caused by
conditional instruction. We need  assume that it is uncoditional.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c07999b..eae2212 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1717,7 +1717,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
     int cond;
 
     /* Unconditional Exception classes */
-    if ( hsr.ec >= 0x10 )
+    if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
         return 1;
 
     /* Check for valid condition in hsr */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] arm: traps: handle unknown exceptions in check_conditional_instr()

[Julien Grall]

Hi,

On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
> According to ARM architecture reference manual, exception with

version + paragraph

> unknown reason (HSR.EC == 0) have no valid bits in HSR
> (apart from HSR.EC), so we can't check if that was caused by
> conditional instruction. We need  assume that it is uncoditional.

unconditional

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/traps.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c07999b..eae2212 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1717,7 +1717,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
>       int cond;
>   
>       /* Unconditional Exception classes */
> -    if ( hsr.ec >= 0x10 )
> +    if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
>           return 1;
>   
>       /* Check for valid condition in hsr */
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/4] arm: traps: handle SMC32 in check_conditional_instr()

[Volodymyr Babchuk]

On ARMv8 architecture SMC instruction in aarch32 state can be conditional.
Thus, we should not skip it while checking HSR.EC value.

For this type of exception special coding of HSR.ISS is used. There is
additional flag to check before perfoming standart handling of CCVALID
and COND fields.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/traps.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index eae2212..6a21763 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1717,8 +1717,20 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
     int cond;
 
     /* Unconditional Exception classes */
+#ifdef CONFIG_ARM_32
     if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
         return 1;
+#else
+    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != HSR_EC_SMC32))
+        return 1;
+
+    /*
+     * Special case for SMC32: we need to check CCKNOWNPASS before
+     * checking CCVALID
+     */
+    if (hsr.ec == HSR_EC_SMC32 && hsr.cond.ccknownpass == 0)
+        return 1;
+#endif
 
     /* Check for valid condition in hsr */
     cond = hsr.cond.ccvalid ? hsr.cond.cc : -1;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] arm: traps: handle SMC32 in check_conditional_instr()

[Julien Grall]

Hi,

On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
> On ARMv8 architecture SMC instruction in aarch32 state can be conditional.

version + paragraph please.

Also, ARMv8 supports both AArch32 and AArch64. As I said in my answer on 
"arm: smccc: handle SMCs/HVCs according to SMCCC" ([1]), This field 
exists for both architecture. I really don't want to tie the 32-bit port 
to ARMv7. We should be able to use ARMv8 too.

> Thus, we should not skip it while checking HSR.EC value. >
> For this type of exception special coding of HSR.ISS is used. There is
> additional flag to check before perfoming standart handling of CCVALID

performing standard

> and COND fields.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/traps.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index eae2212..6a21763 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1717,8 +1717,20 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
>       int cond;
>   
>       /* Unconditional Exception classes */
> +#ifdef CONFIG_ARM_32
>       if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
>           return 1;
> +#else
> +    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != HSR_EC_SMC32))
> +        return 1;
> +
> +    /*
> +     * Special case for SMC32: we need to check CCKNOWNPASS before
> +     * checking CCVALID

Missing full stop.

> +     */
> +    if (hsr.ec == HSR_EC_SMC32 && hsr.cond.ccknownpass == 0)
> +        return 1;
> +#endif
>   
>       /* Check for valid condition in hsr */
>       cond = hsr.cond.ccvalid ? hsr.cond.cc : -1;
> 

Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2017-07/msg01671.html"

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] arm: traps: handle SMC32 in check_conditional_instr()

[Volodymyr Babchuk]

Hi Julien,

On 28.07.17 23:37, Julien Grall wrote:
> Hi,
> 
> On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
>> On ARMv8 architecture SMC instruction in aarch32 state can be 
>> conditional.
> 
> version + paragraph please.
> 
> Also, ARMv8 supports both AArch32 and AArch64. As I said in my answer on 
> "arm: smccc: handle SMCs/HVCs according to SMCCC" ([1]), This field 
> exists for both architecture. I really don't want to tie the 32-bit port 
> to ARMv7. We should be able to use ARMv8 too.
Not sure if I got this.

My ARM 7 ARM (ARM DDI 0406C.c ID051414 page B3-1431) say following:

"SMC instructions cannot be trapped if they fail their condition code 
check.
Therefore, the syndrome information for this exception does not include
conditionality information."

ARMv8 ARM (ARM DDI 0487A.k ID092916) says that SMC from aarch32 state can
be conditional and my patch checks this. But SMC from aarch64 state is
unconditional, so there are nothing to check. At least, when looking at
ISS encoding, i see imm16 field and RES0 field. No conditional flags.

>> Thus, we should not skip it while checking HSR.EC value. >
>> For this type of exception special coding of HSR.ISS is used. There is
>> additional flag to check before perfoming standart handling of CCVALID
> 
> performing standard
> 
>> and COND fields.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/traps.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index eae2212..6a21763 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1717,8 +1717,20 @@ static int check_conditional_instr(struct 
>> cpu_user_regs *regs,
>>       int cond;
>>       /* Unconditional Exception classes */
>> +#ifdef CONFIG_ARM_32
>>       if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
>>           return 1;
>> +#else
>> +    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != 
>> HSR_EC_SMC32))
>> +        return 1;
>> +
>> +    /*
>> +     * Special case for SMC32: we need to check CCKNOWNPASS before
>> +     * checking CCVALID
> 
> Missing full stop.
> 
>> +     */
>> +    if (hsr.ec == HSR_EC_SMC32 && hsr.cond.ccknownpass == 0)
>> +        return 1;
>> +#endif
>>       /* Check for valid condition in hsr */
>>       cond = hsr.cond.ccvalid ? hsr.cond.cc : -1;
>>
> 
> Cheers,
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2017-07/msg01671.html"
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] arm: traps: handle SMC32 in check_conditional_instr()

[Julien Grall]

On 08/08/17 21:42, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

> On 28.07.17 23:37, Julien Grall wrote:
>> Hi,
>>
>> On 07/28/2017 08:43 PM, Volodymyr Babchuk wrote:
>>> On ARMv8 architecture SMC instruction in aarch32 state can be
>>> conditional.
>>
>> version + paragraph please.
>>
>> Also, ARMv8 supports both AArch32 and AArch64. As I said in my answer
>> on "arm: smccc: handle SMCs/HVCs according to SMCCC" ([1]), This field
>> exists for both architecture. I really don't want to tie the 32-bit
>> port to ARMv7. We should be able to use ARMv8 too.
> Not sure if I got this.
>
> My ARM 7 ARM (ARM DDI 0406C.c ID051414 page B3-1431) say following:
>
> "SMC instructions cannot be trapped if they fail their condition code
> check.
> Therefore, the syndrome information for this exception does not include
> conditionality information."
>
> ARMv8 ARM (ARM DDI 0487A.k ID092916) says that SMC from aarch32 state can
> be conditional and my patch checks this. But SMC from aarch64 state is
> unconditional, so there are nothing to check. At least, when looking at
> ISS encoding, i see imm16 field and RES0 field. No conditional flags.

ARM 32-bit is not only ARMv7, it could also be ARMv8. If you look at 
Part G describing the AArch32 state, specifically G1-4434, "The ARMv8-A 
architecture permits, but does not require, this trap to apply to 
conditional SMC instructions that fail their Condition code check...".

Xen ARM 32-bits should be able to boot any 32-bit ARM platform (ARMv7, 
ARMv8). But your #ifdef belows prevent that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel