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

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

[Peng Fan]

>From "G6.2.29 CPSR, Current Program Status Register" of Aarch64 ARM
and "B1.3.3 Program Status Registers (PSRs)" of ARMv7-A ARM:
"
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 );"

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

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

[Julien Grall]

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?

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

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

[Peng Fan]

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

-- 

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

[Ian Campbell]

On Mon, 2015-09-21 at 17:08 +0800, Peng Fan wrote:
> 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.

Were these two patches motivated by an actual issue you were seeing? Or
just from code inspection?

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

[Peng Fan]

On Mon, Sep 21, 2015 at 11:33:10AM +0100, Ian Campbell wrote:
>On Mon, 2015-09-21 at 17:08 +0800, Peng Fan wrote:
>> 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.
>
>Were these two patches motivated by an actual issue you were seeing? Or
>just from code inspection?

Just code inspection, not an actual issue.

Regards,
Peng.

>

--