[1/4] libpore: Fix incorrect mtspr instruction generation

Message ID 20171109004411.9193-2-cyril.bur@au1.ibm.com
State New
Headers show
Series
  • Bug fix and coverity fixes
Related show

Commit Message

Cyril Bur Nov. 9, 2017, 12:44 a.m.
From coverity defect 173758:
CID 173758 (#1 of 1): Unused value (UNUSED_VALUE)
assigned_value: Assigning value from (uint8_t)i_Rs << 21 to
mtsprInstOpcode here, but that stored value is overwritten before it can
be used.

This causes the generated mtspr to always move from register r0 as
opposed to the function parameter i_Rs.

Luckily the only call to getMtsprInstruction is:
getMtsprInstruction( 0, (uint16_t)i_regId );
the first parameter is the register so in an incredible stroke of luck,
the requirement is to generate a mtspr from r0.

Therefore no bug exists today, this is still a fairly important fix
because if anyone uses getMtsprInstruction() with a non zero first
parameter, it will cause them endless headache.

Fixes: CID 173758
Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 libpore/p9_stop_api.C | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Stewart Smith Nov. 14, 2017, 2:57 a.m. | #1
Cyril Bur <cyril.bur@au1.ibm.com> writes:
> From coverity defect 173758:
> CID 173758 (#1 of 1): Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from (uint8_t)i_Rs << 21 to
> mtsprInstOpcode here, but that stored value is overwritten before it can
> be used.
>
> This causes the generated mtspr to always move from register r0 as
> opposed to the function parameter i_Rs.
>
> Luckily the only call to getMtsprInstruction is:
> getMtsprInstruction( 0, (uint16_t)i_regId );
> the first parameter is the register so in an incredible stroke of luck,
> the requirement is to generate a mtspr from r0.
>
> Therefore no bug exists today, this is still a fairly important fix
> because if anyone uses getMtsprInstruction() with a non zero first
> parameter, it will cause them endless headache.
>
> Fixes: CID 173758
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  libpore/p9_stop_api.C | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
> index 26a14bbf..3f0b248d 100644
> --- a/libpore/p9_stop_api.C
> +++ b/libpore/p9_stop_api.C
> @@ -255,12 +255,10 @@ static uint32_t getOrisInstruction( const uint16_t i_Rs, const uint16_t i_Ra,
>   */
>  static uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
>  {
> -    uint32_t mtsprInstOpcode = 0;
> -    uint32_t temp = (( i_Spr & 0x03FF ) << 11);
> -    mtsprInstOpcode = (uint8_t)i_Rs << 21;
> -    mtsprInstOpcode = ( temp  & 0x0000F800 ) << 5;
> -    mtsprInstOpcode |= ( temp & 0x001F0000 ) >> 5;
> -    mtsprInstOpcode |= MTSPR_BASE_OPCODE;
> +    uint32_t mtsprInstOpcode = MTSPR_BASE_OPCODE;
> +    uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);
> +
> +    mtsprInstOpcode |= ((i_Rs & 0x1F) << 21) | ((temp & 0x03FF) << 11);
>  
>      return SWIZZLE_4_BYTE(mtsprInstOpcode);

So, this is what is sitting in (internal) gerrit for a fix, it looks
like I should probably take this one instead? Of course, we actually
have to re-sync with upstream p9_stop_api for that... and there's other
changes coming soon for it, so we should be okay.... one hopes.

diff --git a/chips/p9/procedures/utils/stopreg/p9_stop_api.C b/chips/p9/procedures/utils/stopreg/p9_stop_api.C
index 7419fb4..4b048bb 100755
--- a/chips/p9/procedures/utils/stopreg/p9_stop_api.C
+++ b/chips/p9/procedures/utils/stopreg/p9_stop_api.C
@@ -245,21 +245,21 @@
  * @brief generates instruction for mtspr
  * @param[in] i_Rs      source register number
  * @param[in] i_Spr represents spr where data is to be moved.
  * @return returns 32 bit number representing mtspr instruction.
  */
 static uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
 {
     uint32_t mtsprInstOpcode = 0;
     uint32_t temp = (( i_Spr & 0x03FF ) << 11);
     mtsprInstOpcode = (uint8_t)i_Rs << 21;
-  mtsprInstOpcode = ( temp  & 0x0000F800 ) << 5;
+  mtsprInstOpcode |= ( temp  & 0x0000F800 ) << 5;
     mtsprInstOpcode |= ( temp & 0x001F0000 ) >> 5;
     mtsprInstOpcode |= MTSPR_BASE_OPCODE;
 
     return SWIZZLE_4_BYTE(mtsprInstOpcode);
 }
 
 //----------------------------------------------------------------------------- 
 
 /**
  * @brief generates rldicr instruction.
Stewart Smith Nov. 14, 2017, 5:48 a.m. | #2
Cyril Bur <cyril.bur@au1.ibm.com> writes:
> From coverity defect 173758:
> CID 173758 (#1 of 1): Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from (uint8_t)i_Rs << 21 to
> mtsprInstOpcode here, but that stored value is overwritten before it can
> be used.
>
> This causes the generated mtspr to always move from register r0 as
> opposed to the function parameter i_Rs.
>
> Luckily the only call to getMtsprInstruction is:
> getMtsprInstruction( 0, (uint16_t)i_regId );
> the first parameter is the register so in an incredible stroke of luck,
> the requirement is to generate a mtspr from r0.
>
> Therefore no bug exists today, this is still a fairly important fix
> because if anyone uses getMtsprInstruction() with a non zero first
> parameter, it will cause them endless headache.
>
> Fixes: CID 173758
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  libpore/p9_stop_api.C | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
> index 26a14bbf..3f0b248d 100644
> --- a/libpore/p9_stop_api.C
> +++ b/libpore/p9_stop_api.C
> @@ -255,12 +255,10 @@ static uint32_t getOrisInstruction( const uint16_t i_Rs, const uint16_t i_Ra,
>   */
>  static uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
>  {
> -    uint32_t mtsprInstOpcode = 0;
> -    uint32_t temp = (( i_Spr & 0x03FF ) << 11);
> -    mtsprInstOpcode = (uint8_t)i_Rs << 21;
> -    mtsprInstOpcode = ( temp  & 0x0000F800 ) << 5;
> -    mtsprInstOpcode |= ( temp & 0x001F0000 ) >> 5;
> -    mtsprInstOpcode |= MTSPR_BASE_OPCODE;
> +    uint32_t mtsprInstOpcode = MTSPR_BASE_OPCODE;
> +    uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);
> +
> +    mtsprInstOpcode |= ((i_Rs & 0x1F) << 21) | ((temp & 0x03FF) << 11);
>  
>      return SWIZZLE_4_BYTE(mtsprInstOpcode);
>  }


libpore/p9_stop_api.C: In function ‘getMtsprInstruction’:
libpore/p9_stop_api.C:259:31: error: invalid suffix "F" on integer constant
     uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);
Cyril Bur Nov. 14, 2017, 5:50 a.m. | #3
On Tue, 2017-11-14 at 16:48 +1100, Stewart Smith wrote:
> Cyril Bur <cyril.bur@au1.ibm.com> writes:
> > From coverity defect 173758:
> > CID 173758 (#1 of 1): Unused value (UNUSED_VALUE)
> > assigned_value: Assigning value from (uint8_t)i_Rs << 21 to
> > mtsprInstOpcode here, but that stored value is overwritten before it can
> > be used.
> > 
> > This causes the generated mtspr to always move from register r0 as
> > opposed to the function parameter i_Rs.
> > 
> > Luckily the only call to getMtsprInstruction is:
> > getMtsprInstruction( 0, (uint16_t)i_regId );
> > the first parameter is the register so in an incredible stroke of luck,
> > the requirement is to generate a mtspr from r0.
> > 
> > Therefore no bug exists today, this is still a fairly important fix
> > because if anyone uses getMtsprInstruction() with a non zero first
> > parameter, it will cause them endless headache.
> > 
> > Fixes: CID 173758
> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > ---
> >  libpore/p9_stop_api.C | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
> > index 26a14bbf..3f0b248d 100644
> > --- a/libpore/p9_stop_api.C
> > +++ b/libpore/p9_stop_api.C
> > @@ -255,12 +255,10 @@ static uint32_t getOrisInstruction( const uint16_t i_Rs, const uint16_t i_Ra,
> >   */
> >  static uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
> >  {
> > -    uint32_t mtsprInstOpcode = 0;
> > -    uint32_t temp = (( i_Spr & 0x03FF ) << 11);
> > -    mtsprInstOpcode = (uint8_t)i_Rs << 21;
> > -    mtsprInstOpcode = ( temp  & 0x0000F800 ) << 5;
> > -    mtsprInstOpcode |= ( temp & 0x001F0000 ) >> 5;
> > -    mtsprInstOpcode |= MTSPR_BASE_OPCODE;
> > +    uint32_t mtsprInstOpcode = MTSPR_BASE_OPCODE;
> > +    uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);
> > +
> > +    mtsprInstOpcode |= ((i_Rs & 0x1F) << 21) | ((temp & 0x03FF) << 11);
> >  
> >      return SWIZZLE_4_BYTE(mtsprInstOpcode);
> >  }
> 
> 
> libpore/p9_stop_api.C: In function ‘getMtsprInstruction’:
> libpore/p9_stop_api.C:259:31: error: invalid suffix "F" on integer constant
>      uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);

Awkward sorry, do you want a fix, or does this mean you'll just take
the other patch?

>

Patch

diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
index 26a14bbf..3f0b248d 100644
--- a/libpore/p9_stop_api.C
+++ b/libpore/p9_stop_api.C
@@ -255,12 +255,10 @@  static uint32_t getOrisInstruction( const uint16_t i_Rs, const uint16_t i_Ra,
  */
 static uint32_t getMtsprInstruction( const uint16_t i_Rs, const uint16_t i_Spr )
 {
-    uint32_t mtsprInstOpcode = 0;
-    uint32_t temp = (( i_Spr & 0x03FF ) << 11);
-    mtsprInstOpcode = (uint8_t)i_Rs << 21;
-    mtsprInstOpcode = ( temp  & 0x0000F800 ) << 5;
-    mtsprInstOpcode |= ( temp & 0x001F0000 ) >> 5;
-    mtsprInstOpcode |= MTSPR_BASE_OPCODE;
+    uint32_t mtsprInstOpcode = MTSPR_BASE_OPCODE;
+    uint32_t temp = ((i_Spr & 1F) << 5) | ((i_Spr & 0x8F) >> 5);
+
+    mtsprInstOpcode |= ((i_Rs & 0x1F) << 21) | ((temp & 0x03FF) << 11);
 
     return SWIZZLE_4_BYTE(mtsprInstOpcode);
 }