Re: [Xen-devel] [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?


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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 
>> Cc: Ian Campbell 
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>> ---
>>  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

-- 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [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 
> Cc: Ian Campbell 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> ---
>  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel