| Message ID | 20171109004411.9193-2-cyril.bur@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug fix and coverity fixes | expand |
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.
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);
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? >
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); }
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(-)