Re: [PATCH 2/2] xen: arm: traps: correct cond

[Peng Fan <b51431@freescale.com>]

On Mon, Sep 21, 2015 at 11:10:11AM +0100, Julien Grall wrote:
>Hi Peng,
>
>On 21/09/15 08:07, Peng Fan wrote:
>> From "G6.2.29 CPSR, Current Program Status Register" of Aarch64 ARM
>> and "B1.3.3 Program Status Registers (PSRs)" of ARMv7-A ARM:
>> "
>
>The section number may change between the different version of the spec.
>Can you also precise the spec version?
>
>For instance on my ARM64 spec (ARM DDI 0497A.d) the section G6.2.29
>points to "CSSELR" and not "CPSR".
>
>> IT[7:5] holds the base condition for the IT block. The base condition is
>>         the top 3 bits of the condition code specified by the first
>> 	condition field of the IT instruction.
>> IT[4:0] encodes the size of the IT block, which is the number of
>>         instructions that are to be conditionally executed, by the
>> 	position of the least significant 1 in this field. It also
>> 	encodes the value of the least significant bit of the condition
>> 	code for each instruction in the block.
>> "
>> So should be "cond = ( it >> 5 );" but not "cond = ( it >> 4 );"
>
>IT[7:5] encodes the top 3 bits of the condition code and one bit of
>IT[4:0] will contain the least significant bit of the condition code.
>
>In order to get the full condition code you have to use IT[7:4] which
>the current code does (see A2.5.2 ARM DDI 0406C.b).
>
>So the current code looks valid to me. Did I miss something?

No, you are correct.

Thanks,
Peng.

>
>Regards,
>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>> Cc: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/traps.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 2e2b1f2..b2879b7 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1561,8 +1561,8 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
>>          if ( it == 0 )
>>              return 1;
>>  
>> -        /* The cond for this instruction works out as the top 4 bits. */
>> -        cond = ( it >> 4 );
>> +        /* The cond for this instruction works out as the top 3 bits. */
>> +        cond = ( it >> 5 );
>>      }
>>  
>>      cpsr_cond = cpsr >> 28;
>> 
>
>-- 
>Julien Grall

--