[PATCH v2 0/4] arm: allign check_conditional_instr() with ARMv8

[PATCH v2 0/4] arm: allign check_conditional_instr() with ARMv8

[Volodymyr Babchuk]

Hello all,

This is second version of patch series for correct handling of SMC32
in check_conditional_instr().
After changes proposed by Julien, first patch ("arm: processor: rename iss
to res0 in hsr_cond union") does not strictly belongs to this series.
But I decided to leave it anyways, becase it does right thing. It can be
dropped, though.

Volodymyr Babchuk (4):
  arm: processor: rename iss to res0 in hsr_cond union
  arm: processor: add new struct hsr_smc32 into hsr union
  arm: traps: handle unknown exceptions in check_conditional_instr()
  arm: traps: handle SMC32 in check_conditional_instr()

 xen/arch/arm/traps.c            | 18 +++++++++++++++++-
 xen/include/asm-arm/processor.h | 21 ++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

-- 
2.7.4


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

[PATCH v2 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 (ARM DDI 0487A.k pages D7-1939 - D7-1949).

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

- Added reference to ARM archtecture manual in the commit message.

---
 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

[PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr 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.
(ARM DDI 0487A.k page D7-1949)

This is instruction specific field, so better to add new structure
to union hsr. This structure describes ISS encoding for an exception
from SMC instruction execution in AArch32 state. But we define this
struct for both ARMv7 and ARMv8. The reason is described in comment
to the structure:

"Nevertheless, we define this encoding for both ARMv7 and ARMv8,
because check_conditional_inst() should properly handle SMC
instruction in all modes: ARMv7, aarch32 and aarch64."

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

- Created new stucture for HSR_EC_SMC32 instead of extending
  fields in hsr.cond.
- Added references to ARM manual.
- Wrote comment with rationale.

---
 xen/include/asm-arm/processor.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index f640d54..af4a0f7 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -488,6 +488,25 @@ union hsr {
         unsigned long ec:6;     /* Exception Class */
     } cp; /* HSR_EC_CP */
 
+    /*
+     * This encoding is valid only for ARMv8 (ARM DDI 0487A.k pages D7-1949 and
+     * G6-4405). On ARMv7, encoding ISS for EC=0x13 is defined as UNK/SBZP
+     * (ARM DDI 0406C.c page B3-1431). UNK/SBZP means that hardware implements
+     * this field as Read-As-Zero.
+     *
+     * Nevertheless, we define this encoding for both ARMv7 and ARMv8, because
+     * check_conditional_inst() should properly handle SMC instruction in all
+     * modes: ARMv7, aarch32 and aarch64.
+     */
+    struct hsr_smc32 {
+        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 */
+        unsigned long ec:6;    /* Exception Class */
+    } smc32; /* HSR_EC_SMC32 */
+
 #ifdef CONFIG_ARM_64
     struct hsr_sysreg {
         unsigned long read:1;   /* Direction */
-- 
2.7.4


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

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

[Volodymyr Babchuk]

According to ARM architecture reference manual (ARM DDI 0487A.k page D7-1938,
ARM DDI 0406C.c page B3-1426), 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 unconditional.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

- Added reference to the ARM manuals

---
 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

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

[Volodymyr Babchuk]

On ARMv8 architecture SMC instruction from aarch32 state can be conditional.
(ARM DDI 0487A.k page D7-1949)
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 (CCKNOWNPASS) to check before perfoming standard handling
of CCVALID and COND fields.

Because we can't distinguish ARMv7 from aarch32 state, we will always
check CCKNOWNPASS field. On ARMv7 it will be read as 0
(ARM DDI 0406C.c page B3-1431), so there will be no problem.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

- Added references to the ARM manuals.
- Moved code from under #ifdef CONFIG_ARM_64
- Added comment with rationale for this

---
 xen/arch/arm/traps.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index eae2212..384219d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1716,8 +1716,24 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
     unsigned long cpsr, cpsr_cond;
     int cond;
 
+    /*
+     * SMC32 instruction case is very special. Under SMC32 we mean
+     * SMC instruction on ARMv7 or SMC instruction originating from
+     * aarch32 state on ARMv8.
+     * On ARMv7 it is uncoditional (ARM DDI 0406C.c page B3-1431),
+     * but is conditional on ARMv8 (ARM DDI 0487A.k page D7-1949).
+     * Problem is that we can't determine if XEN is running on
+     * ARMv7 on ARMv8 in aarch32 mode. Fortunately, while running
+     * on ARMv7 all conditonal bits of HSR will read as 0
+     * (ARM DDI 0406C.c page B3-1431).
+     * This includes CCKNOWNPASS bit of HSR. Thereby we will be okay,
+     * if we'll check that bit.
+     */
+    if (hsr.ec == HSR_EC_SMC32 && hsr.smc32.ccknownpass == 0)
+        return 1;
+
     /* Unconditional Exception classes */
-    if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
+    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != HSR_EC_SMC32))
         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 v2 1/4] arm: processor: rename iss to res0 in hsr_cond union

[Julien Grall]

Hi Volodymyr,

On 09/08/2017 20:44, 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
> part of ISS as RES0 when it describes encoding for conditional
> exceptions (ARM DDI 0487A.k pages D7-1939 - D7-1949).	

Please use the latest ARM manual (i.e ARM DDI 0487A.b). And this is 
still not true. If you look at:
	- WFI/WFE, bit 1 is not res0.
	- MCR/MRC, all bits are defined

If you really want to rename this field, then name it pad or ign. But 
res0 is completely bogus.

Cheers,

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> - Added reference to ARM archtecture manual in the commit message.
>
> ---
>  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

Re: [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Julien Grall]

On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> On ARMv8, one of conditional exceptions (SMC that originates
> from aarch32 state) have extra field in HCR.ISS encoding:

s/aarch32/AArch32/
s/have/has/

And the register is called HSR and not HCR.

>
> 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.
> (ARM DDI 0487A.k page D7-1949)

Please use the latest ARM ARM.

>
> This is instruction specific field, so better to add new structure

This is an instruction...

> to union hsr. This structure describes ISS encoding for an exception
> from SMC instruction execution in AArch32 state. But we define this
> struct for both ARMv7 and ARMv8. The reason is described in comment
> to the structure:
>
> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
> because check_conditional_inst() should properly handle SMC
> instruction in all modes: ARMv7, aarch32 and aarch64."

Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is 
just a version of the specification which happen to only support AArch32.

Actually Xen does not care about ARMv8 vs ARMv7. It only care about 
AArch32 vs AArch64.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> - Created new stucture for HSR_EC_SMC32 instead of extending
>   fields in hsr.cond.
> - Added references to ARM manual.
> - Wrote comment with rationale.
>
> ---
>  xen/include/asm-arm/processor.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index f640d54..af4a0f7 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -488,6 +488,25 @@ union hsr {
>          unsigned long ec:6;     /* Exception Class */
>      } cp; /* HSR_EC_CP */
>
> +    /*
> +     * This encoding is valid only for ARMv8 (ARM DDI 0487A.k pages D7-1949 and
> +     * G6-4405). On ARMv7, encoding ISS for EC=0x13 is defined as UNK/SBZP
> +     * (ARM DDI 0406C.c page B3-1431). UNK/SBZP means that hardware implements
> +     * this field as Read-As-Zero.
> +     *
> +     * Nevertheless, we define this encoding for both ARMv7 and ARMv8, because
> +     * check_conditional_inst() should properly handle SMC instruction in all
> +     * modes: ARMv7, aarch32 and aarch64.

See my comment above.

> +     */
> +    struct hsr_smc32 {
> +        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 */
> +        unsigned long ec:6;    /* Exception Class */
> +    } smc32; /* HSR_EC_SMC32 */
> +
>  #ifdef CONFIG_ARM_64
>      struct hsr_sysreg {
>          unsigned long read:1;   /* Direction */
>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> According to ARM architecture reference manual (ARM DDI 0487A.k page D7-1938,

Please quote a more recent ARM ARM. In general, please use the latest 
ARM ARM when sending code specific to the architecture.

Assuming this will be fixed in the next revision:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,


> ARM DDI 0406C.c page B3-1426), 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 unconditional.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> - Added reference to the ARM manuals
>
> ---
>  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

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

[Julien Grall]

On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> On ARMv8 architecture SMC instruction from aarch32 state can be conditional.

s/aarch32/AArch32/. But SMC instruction are conditional even on ARMv7. 
The main difference is the ARMv8 architecture allows conditional SMCs to 
be trapped even if they failed their condition check.

> (ARM DDI 0487A.k page D7-1949)
> 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 (CCKNOWNPASS) to check before perfoming standard handling

s/perfoming/performing/

> of CCVALID and COND fields.
>
> Because we can't distinguish ARMv7 from aarch32 state, we will always

See my comment on patch #2.

> check CCKNOWNPASS field. On ARMv7 it will be read as 0
> (ARM DDI 0406C.c page B3-1431), so there will be no problem.

Field are usually made RES{0,1} to allow later revision of the 
architecture to use them and not break previous version. There are now 
ARMv8.1, ARMv8.2, ARMv8.3. It does not mean we have 3 new modes. It just 
add more features on top of the earlier revision.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> - Added references to the ARM manuals.
> - Moved code from under #ifdef CONFIG_ARM_64
> - Added comment with rationale for this
>
> ---
>  xen/arch/arm/traps.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index eae2212..384219d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1716,8 +1716,24 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
>      unsigned long cpsr, cpsr_cond;
>      int cond;
>
> +    /*
> +     * SMC32 instruction case is very special. Under SMC32 we mean
> +     * SMC instruction on ARMv7 or SMC instruction originating from
> +     * aarch32 state on ARMv8.
> +     * On ARMv7 it is uncoditional (ARM DDI 0406C.c page B3-1431),
> +     * but is conditional on ARMv8 (ARM DDI 0487A.k page D7-1949).
> +     * Problem is that we can't determine if XEN is running on
> +     * ARMv7 on ARMv8 in aarch32 mode. Fortunately, while running

See my comment on top.

> +     * on ARMv7 all conditonal bits of HSR will read as 0
> +     * (ARM DDI 0406C.c page B3-1431).
> +     * This includes CCKNOWNPASS bit of HSR. Thereby we will be okay,
> +     * if we'll check that bit.
> +     */
> +    if (hsr.ec == HSR_EC_SMC32 && hsr.smc32.ccknownpass == 0)
> +        return 1;
> +
>      /* Unconditional Exception classes */
> -    if ( hsr.ec == HSR_EC_UNKNOWN || hsr.ec >= 0x10 )
> +    if ( hsr.ec == HSR_EC_UNKNOWN || (hsr.ec >= 0x10 && hsr.ec != HSR_EC_SMC32))
>          return 1;
>
>      /* Check for valid condition in hsr */
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Volodymyr Babchuk]

Hi Julien,

On 09.08.17 23:34, Julien Grall wrote:
> 
> 
> On 09/08/2017 20:44, Volodymyr Babchuk wrote:
>> On ARMv8, one of conditional exceptions (SMC that originates
>> from aarch32 state) have extra field in HCR.ISS encoding:
> 
> s/aarch32/AArch32/
> s/have/has/
> 
> And the register is called HSR and not HCR.
> 
>>
>> 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.
>> (ARM DDI 0487A.k page D7-1949)
> 
> Please use the latest ARM ARM.
> 
>>
>> This is instruction specific field, so better to add new structure
> 
> This is an instruction...
> 
>> to union hsr. This structure describes ISS encoding for an exception
>> from SMC instruction execution in AArch32 state. But we define this
>> struct for both ARMv7 and ARMv8. The reason is described in comment
>> to the structure:
>>
>> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
>> because check_conditional_inst() should properly handle SMC
>> instruction in all modes: ARMv7, aarch32 and aarch64."
> 
> Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is 
> just a version of the specification which happen to only support AArch32.
Yeah, I wondered how to formulate that better. Problem is that ARMv7
specification does not use term "AArch32". So I decided to mention ARMv7 
explicitly.
How about this: "check_conditional_inst() should properly handle SMC 
instruction on both architectures (ARMv7 and ARMv8) while running in 
aarch32 or aarch64 mode" ?

> Actually Xen does not care about ARMv8 vs ARMv7. It only care about 
> AArch32 vs AArch64.
Yes. And probably it can be problem in the future. Because, as we can 
see, there are differences between ARMv7 and ARMv8.

>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>
>> - Created new stucture for HSR_EC_SMC32 instead of extending
>>   fields in hsr.cond.
>> - Added references to ARM manual.
>> - Wrote comment with rationale.
>>
>> ---
>>  xen/include/asm-arm/processor.h | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/processor.h 
>> b/xen/include/asm-arm/processor.h
>> index f640d54..af4a0f7 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -488,6 +488,25 @@ union hsr {
>>          unsigned long ec:6;     /* Exception Class */
>>      } cp; /* HSR_EC_CP */
>>
>> +    /*
>> +     * This encoding is valid only for ARMv8 (ARM DDI 0487A.k pages 
>> D7-1949 and
>> +     * G6-4405). On ARMv7, encoding ISS for EC=0x13 is defined as 
>> UNK/SBZP
>> +     * (ARM DDI 0406C.c page B3-1431). UNK/SBZP means that hardware 
>> implements
>> +     * this field as Read-As-Zero.
>> +     *
>> +     * Nevertheless, we define this encoding for both ARMv7 and 
>> ARMv8, because
>> +     * check_conditional_inst() should properly handle SMC 
>> instruction in all
>> +     * modes: ARMv7, aarch32 and aarch64.
> 
> See my comment above.
> 
>> +     */
>> +    struct hsr_smc32 {
>> +        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 */
>> +        unsigned long ec:6;    /* Exception Class */
>> +    } smc32; /* HSR_EC_SMC32 */
>> +
>>  #ifdef CONFIG_ARM_64
>>      struct hsr_sysreg {
>>          unsigned long read:1;   /* Direction */
>>
> 
> Cheers,
> 

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

Re: [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Julien Grall]

On 09/08/2017 22:06, Volodymyr Babchuk wrote:
> Hi Julien,
>
> On 09.08.17 23:34, Julien Grall wrote:
>>
>>
>> On 09/08/2017 20:44, Volodymyr Babchuk wrote:
>>> On ARMv8, one of conditional exceptions (SMC that originates
>>> from aarch32 state) have extra field in HCR.ISS encoding:
>>
>> s/aarch32/AArch32/
>> s/have/has/
>>
>> And the register is called HSR and not HCR.
>>
>>>
>>> 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.
>>> (ARM DDI 0487A.k page D7-1949)
>>
>> Please use the latest ARM ARM.
>>
>>>
>>> This is instruction specific field, so better to add new structure
>>
>> This is an instruction...
>>
>>> to union hsr. This structure describes ISS encoding for an exception
>>> from SMC instruction execution in AArch32 state. But we define this
>>> struct for both ARMv7 and ARMv8. The reason is described in comment
>>> to the structure:
>>>
>>> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
>>> because check_conditional_inst() should properly handle SMC
>>> instruction in all modes: ARMv7, aarch32 and aarch64."
>>
>> Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is
>> just a version of the specification which happen to only support AArch32.
> Yeah, I wondered how to formulate that better. Problem is that ARMv7
> specification does not use term "AArch32". So I decided to mention ARMv7
> explicitly.

The term AArch32 was introduced with ARMv8 and use to refer 32-bit 
state. ARMv7 is only 32-bit, and therefore has only AArch32 state.

> How about this: "check_conditional_inst() should properly handle SMC
> instruction on both architectures (ARMv7 and ARMv8) while running in
> aarch32 or aarch64 mode" ?

"ARMv8 allows to trap conditional SMC from AArch32 state even if the 
condition check failed. Modify check_conditional_inst() to handle them."

>
>> Actually Xen does not care about ARMv8 vs ARMv7. It only care about
>> AArch32 vs AArch64.
> Yes. And probably it can be problem in the future. Because, as we can
> see, there are differences between ARMv7 and ARMv8.

I don't see any problem. Bits not used are usually made RES{0,1} to 
allow later revision using them for new features.

There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they 
always ensure backward compatibility on reading or a way to detect the 
new feature if the kernel has to set/clear bits.

In the case of the ISS for SMC, the bits used are RES0, with the new 
meaning 0 means the SMC is unconditional or the condition passed. This 
is compatible with ARMv7 because conditional SMC are only trapped when 
the condition check passed.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Volodymyr Babchuk]

On 10.08.17 00:22, Julien Grall wrote:
> 
> 
> On 09/08/2017 22:06, Volodymyr Babchuk wrote:
>> Hi Julien,
>>
>> On 09.08.17 23:34, Julien Grall wrote:
>>>
>>>
>>> On 09/08/2017 20:44, Volodymyr Babchuk wrote:
>>>> On ARMv8, one of conditional exceptions (SMC that originates
>>>> from aarch32 state) have extra field in HCR.ISS encoding:
>>>
>>> s/aarch32/AArch32/
>>> s/have/has/
>>>
>>> And the register is called HSR and not HCR.
>>>
>>>>
>>>> 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.
>>>> (ARM DDI 0487A.k page D7-1949)
>>>
>>> Please use the latest ARM ARM.
>>>
>>>>
>>>> This is instruction specific field, so better to add new structure
>>>
>>> This is an instruction...
>>>
>>>> to union hsr. This structure describes ISS encoding for an exception
>>>> from SMC instruction execution in AArch32 state. But we define this
>>>> struct for both ARMv7 and ARMv8. The reason is described in comment
>>>> to the structure:
>>>>
>>>> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
>>>> because check_conditional_inst() should properly handle SMC
>>>> instruction in all modes: ARMv7, aarch32 and aarch64."
>>>
>>> Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is
>>> just a version of the specification which happen to only support 
>>> AArch32.
>> Yeah, I wondered how to formulate that better. Problem is that ARMv7
>> specification does not use term "AArch32". So I decided to mention ARMv7
>> explicitly.
> 
> The term AArch32 was introduced with ARMv8 and use to refer 32-bit 
> state. ARMv7 is only 32-bit, and therefore has only AArch32 state.
Hmm, maybe it is only me, but when I see term "AArch32" I automatically 
think about ARMv8 only, because I know that there was no such term in 
ARMv7. So for me "AArch32 or AArch64 state" sounds like "It is 
ARMv8-only thing, no ARMv7 there". Thus, I'd prefer to leave mention 
about ARMv7 for clarity. Maybe, just phrase it differently.

>> How about this: "check_conditional_inst() should properly handle SMC
>> instruction on both architectures (ARMv7 and ARMv8) while running in
>> aarch32 or aarch64 mode" ?
> 
> "ARMv8 allows to trap conditional SMC from AArch32 state even if the 
> condition check failed. Modify check_conditional_inst() to handle them."
> 
>>
>>> Actually Xen does not care about ARMv8 vs ARMv7. It only care about
>>> AArch32 vs AArch64.
>> Yes. And probably it can be problem in the future. Because, as we can
>> see, there are differences between ARMv7 and ARMv8.
> 
> I don't see any problem. Bits not used are usually made RES{0,1} to 
> allow later revision using them for new features.
Okay, I'll refrain from use word "problem" :)

> There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they 
> always ensure backward compatibility on reading or a way to detect the 
> new feature if the kernel has to set/clear bits.
> 
> In the case of the ISS for SMC, the bits used are RES0, with the new 
> meaning 0 means the SMC is unconditional or the condition passed. This 
> is compatible with ARMv7 because conditional SMC are only trapped when 
> the condition check passed.
I'll rewrite comments in terms of backward compatibility then.

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

Re: [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Julien Grall]

On 11/08/17 14:26, Volodymyr Babchuk wrote:
>
>
> On 10.08.17 00:22, Julien Grall wrote:
>>
>>
>> On 09/08/2017 22:06, Volodymyr Babchuk wrote:
>>> Hi Julien,
>>>
>>> On 09.08.17 23:34, Julien Grall wrote:
>>>>
>>>>
>>>> On 09/08/2017 20:44, Volodymyr Babchuk wrote:
>>>>> On ARMv8, one of conditional exceptions (SMC that originates
>>>>> from aarch32 state) have extra field in HCR.ISS encoding:
>>>>
>>>> s/aarch32/AArch32/
>>>> s/have/has/
>>>>
>>>> And the register is called HSR and not HCR.
>>>>
>>>>>
>>>>> 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.
>>>>> (ARM DDI 0487A.k page D7-1949)
>>>>
>>>> Please use the latest ARM ARM.
>>>>
>>>>>
>>>>> This is instruction specific field, so better to add new structure
>>>>
>>>> This is an instruction...
>>>>
>>>>> to union hsr. This structure describes ISS encoding for an exception
>>>>> from SMC instruction execution in AArch32 state. But we define this
>>>>> struct for both ARMv7 and ARMv8. The reason is described in comment
>>>>> to the structure:
>>>>>
>>>>> "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
>>>>> because check_conditional_inst() should properly handle SMC
>>>>> instruction in all modes: ARMv7, aarch32 and aarch64."
>>>>
>>>> Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is
>>>> just a version of the specification which happen to only support
>>>> AArch32.
>>> Yeah, I wondered how to formulate that better. Problem is that ARMv7
>>> specification does not use term "AArch32". So I decided to mention ARMv7
>>> explicitly.
>>
>> The term AArch32 was introduced with ARMv8 and use to refer 32-bit
>> state. ARMv7 is only 32-bit, and therefore has only AArch32 state.
> Hmm, maybe it is only me, but when I see term "AArch32" I automatically
> think about ARMv8 only, because I know that there was no such term in
> ARMv7. So for me "AArch32 or AArch64 state" sounds like "It is
> ARMv8-only thing, no ARMv7 there". Thus, I'd prefer to leave mention
> about ARMv7 for clarity. Maybe, just phrase it differently.

You are right here. For me, they are the same because ARMv8 32-bit has 
been designed to be compatible with the former. I would tend to use ARM 
32-bit and AArch32 interchangeably.

>
>>> How about this: "check_conditional_inst() should properly handle SMC
>>> instruction on both architectures (ARMv7 and ARMv8) while running in
>>> aarch32 or aarch64 mode" ?
>>
>> "ARMv8 allows to trap conditional SMC from AArch32 state even if the
>> condition check failed. Modify check_conditional_inst() to handle them."
>>
>>>
>>>> Actually Xen does not care about ARMv8 vs ARMv7. It only care about
>>>> AArch32 vs AArch64.
>>> Yes. And probably it can be problem in the future. Because, as we can
>>> see, there are differences between ARMv7 and ARMv8.
>>
>> I don't see any problem. Bits not used are usually made RES{0,1} to
>> allow later revision using them for new features.
> Okay, I'll refrain from use word "problem" :)

There are a few differences between the both. But I would not call that 
a problem as they are mostly the same.

>
>> There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they
>> always ensure backward compatibility on reading or a way to detect the
>> new feature if the kernel has to set/clear bits.
>>
>> In the case of the ISS for SMC, the bits used are RES0, with the new
>> meaning 0 means the SMC is unconditional or the condition passed. This
>> is compatible with ARMv7 because conditional SMC are only trapped when
>> the condition check passed.
> I'll rewrite comments in terms of backward compatibility then.

Cheers,

-- 
Julien Grall

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