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

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

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