[PATCH] hw/net/i82596: Correct command bitmask (CID 1419392)

Philippe Mathieu-Daudé posted 1 patch 32 weeks ago
Test docker-quick@centos7 passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200214004753.15736-1-f4bug@amsat.org
hw/net/i82596.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

[PATCH] hw/net/i82596: Correct command bitmask (CID 1419392)

Posted by Philippe Mathieu-Daudé 32 weeks ago
The command is 32-bit, but we are loading the 16 upper bits with
the 'get_uint16(s->scb + 2)' call.

Once shifted by 16, the command bits match the status bits:

- Command
  Bit 31 ACK-CX   Acknowledges that the CU completed an Action Command.
  Bit 30 ACK-FR   Acknowledges that the RU received a frame.
  Bit 29 ACK-CNA  Acknowledges that the Command Unit became not active.
  Bit 28 ACK-RNR  Acknowledges that the Receive Unit became not ready.

- Status
  Bit 15 CX       The CU finished executing a command with its I(interrupt) bit set.
  Bit 14 FR       The RU finished receiving a frame.
  Bit 13 CNA      The Command Unit left the Active state.
  Bit 12 RNR      The Receive Unit left the Ready state.

Add the SCB_COMMAND_ACK_MASK definition to simplify the code.

This fixes Coverity 1419392 (CONSTANT_EXPRESSION_RESULT):

  /hw/net/i82596.c: 352 in examine_scb()
  346         cuc = (command >> 8) & 0x7;
  347         ruc = (command >> 4) & 0x7;
  348         DBG(printf("MAIN COMMAND %04x  cuc %02x ruc %02x\n", command, cuc, ruc));
  349         /* and clear the scb command word */
  350         set_uint16(s->scb + 2, 0);
  351
  >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
  >>>     "command & (2147483648UL /* 1UL << 31 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
  352         if (command & BIT(31))      /* ACK-CX */
  353             s->scb_status &= ~SCB_STATUS_CX;
  >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
  >>>     "command & (1073741824UL /* 1UL << 30 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
  354         if (command & BIT(30))      /*ACK-FR */
  355             s->scb_status &= ~SCB_STATUS_FR;
  >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
  >>>     "command & (536870912UL /* 1UL << 29 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
  356         if (command & BIT(29))      /*ACK-CNA */
  357             s->scb_status &= ~SCB_STATUS_CNA;
  >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
  >>>     "command & (268435456UL /* 1UL << 28 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
  358         if (command & BIT(28))      /*ACK-RNR */
  359             s->scb_status &= ~SCB_STATUS_RNR;

Fixes: Covertiy CID 1419392 (commit 376b851909)
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/i82596.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/net/i82596.c b/hw/net/i82596.c
index 3a0e1ec4c0..b7c2458a96 100644
--- a/hw/net/i82596.c
+++ b/hw/net/i82596.c
@@ -43,6 +43,9 @@
 #define SCB_STATUS_CNA  0x2000 /* CU left active state */
 #define SCB_STATUS_RNR  0x1000 /* RU left active state */
 
+#define SCB_COMMAND_ACK_MASK \
+        (SCB_STATUS_CX | SCB_STATUS_FR | SCB_STATUS_CNA | SCB_STATUS_RNR)
+
 #define CU_IDLE         0
 #define CU_SUSPENDED    1
 #define CU_ACTIVE       2
@@ -349,14 +352,7 @@ static void examine_scb(I82596State *s)
     /* and clear the scb command word */
     set_uint16(s->scb + 2, 0);
 
-    if (command & BIT(31))      /* ACK-CX */
-        s->scb_status &= ~SCB_STATUS_CX;
-    if (command & BIT(30))      /*ACK-FR */
-        s->scb_status &= ~SCB_STATUS_FR;
-    if (command & BIT(29))      /*ACK-CNA */
-        s->scb_status &= ~SCB_STATUS_CNA;
-    if (command & BIT(28))      /*ACK-RNR */
-        s->scb_status &= ~SCB_STATUS_RNR;
+    s->scb_status &= ~(command & SCB_COMMAND_ACK_MASK);
 
     switch (cuc) {
     case 0:     /* no change */
-- 
2.21.1


Re: [PATCH] hw/net/i82596: Correct command bitmask (CID 1419392)

Posted by Peter Maydell 28 weeks ago
On Fri, 14 Feb 2020 at 00:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The command is 32-bit, but we are loading the 16 upper bits with
> the 'get_uint16(s->scb + 2)' call.
>
> Once shifted by 16, the command bits match the status bits:
>
> - Command
>   Bit 31 ACK-CX   Acknowledges that the CU completed an Action Command.
>   Bit 30 ACK-FR   Acknowledges that the RU received a frame.
>   Bit 29 ACK-CNA  Acknowledges that the Command Unit became not active.
>   Bit 28 ACK-RNR  Acknowledges that the Receive Unit became not ready.
>
> - Status
>   Bit 15 CX       The CU finished executing a command with its I(interrupt) bit set.
>   Bit 14 FR       The RU finished receiving a frame.
>   Bit 13 CNA      The Command Unit left the Active state.
>   Bit 12 RNR      The Receive Unit left the Ready state.
>
> Add the SCB_COMMAND_ACK_MASK definition to simplify the code.
>
> This fixes Coverity 1419392 (CONSTANT_EXPRESSION_RESULT):
>
>   /hw/net/i82596.c: 352 in examine_scb()
>   346         cuc = (command >> 8) & 0x7;
>   347         ruc = (command >> 4) & 0x7;
>   348         DBG(printf("MAIN COMMAND %04x  cuc %02x ruc %02x\n", command, cuc, ruc));
>   349         /* and clear the scb command word */
>   350         set_uint16(s->scb + 2, 0);
>   351
>   >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>   >>>     "command & (2147483648UL /* 1UL << 31 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>   352         if (command & BIT(31))      /* ACK-CX */
>   353             s->scb_status &= ~SCB_STATUS_CX;
>   >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>   >>>     "command & (1073741824UL /* 1UL << 30 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>   354         if (command & BIT(30))      /*ACK-FR */
>   355             s->scb_status &= ~SCB_STATUS_FR;
>   >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>   >>>     "command & (536870912UL /* 1UL << 29 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>   356         if (command & BIT(29))      /*ACK-CNA */
>   357             s->scb_status &= ~SCB_STATUS_CNA;
>   >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>   >>>     "command & (268435456UL /* 1UL << 28 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>   358         if (command & BIT(28))      /*ACK-RNR */
>   359             s->scb_status &= ~SCB_STATUS_RNR;
>
> Fixes: Covertiy CID 1419392 (commit 376b851909)

("Coverity")

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Jason, are you planning to pick this one up?

thanks
-- PMM

Re: [PATCH] hw/net/i82596: Correct command bitmask (CID 1419392)

Posted by Jason Wang 27 weeks ago
On 2020/3/13 下午7:01, Peter Maydell wrote:
> On Fri, 14 Feb 2020 at 00:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> The command is 32-bit, but we are loading the 16 upper bits with
>> the 'get_uint16(s->scb + 2)' call.
>>
>> Once shifted by 16, the command bits match the status bits:
>>
>> - Command
>>    Bit 31 ACK-CX   Acknowledges that the CU completed an Action Command.
>>    Bit 30 ACK-FR   Acknowledges that the RU received a frame.
>>    Bit 29 ACK-CNA  Acknowledges that the Command Unit became not active.
>>    Bit 28 ACK-RNR  Acknowledges that the Receive Unit became not ready.
>>
>> - Status
>>    Bit 15 CX       The CU finished executing a command with its I(interrupt) bit set.
>>    Bit 14 FR       The RU finished receiving a frame.
>>    Bit 13 CNA      The Command Unit left the Active state.
>>    Bit 12 RNR      The Receive Unit left the Ready state.
>>
>> Add the SCB_COMMAND_ACK_MASK definition to simplify the code.
>>
>> This fixes Coverity 1419392 (CONSTANT_EXPRESSION_RESULT):
>>
>>    /hw/net/i82596.c: 352 in examine_scb()
>>    346         cuc = (command >> 8) & 0x7;
>>    347         ruc = (command >> 4) & 0x7;
>>    348         DBG(printf("MAIN COMMAND %04x  cuc %02x ruc %02x\n", command, cuc, ruc));
>>    349         /* and clear the scb command word */
>>    350         set_uint16(s->scb + 2, 0);
>>    351
>>    >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>>    >>>     "command & (2147483648UL /* 1UL << 31 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>>    352         if (command & BIT(31))      /* ACK-CX */
>>    353             s->scb_status &= ~SCB_STATUS_CX;
>>    >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>>    >>>     "command & (1073741824UL /* 1UL << 30 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>>    354         if (command & BIT(30))      /*ACK-FR */
>>    355             s->scb_status &= ~SCB_STATUS_FR;
>>    >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>>    >>>     "command & (536870912UL /* 1UL << 29 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>>    356         if (command & BIT(29))      /*ACK-CNA */
>>    357             s->scb_status &= ~SCB_STATUS_CNA;
>>    >>>     CID 1419392:    (CONSTANT_EXPRESSION_RESULT)
>>    >>>     "command & (268435456UL /* 1UL << 28 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if".
>>    358         if (command & BIT(28))      /*ACK-RNR */
>>    359             s->scb_status &= ~SCB_STATUS_RNR;
>>
>> Fixes: Covertiy CID 1419392 (commit 376b851909)
> ("Coverity")
>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Jason, are you planning to pick this one up?


Yes. queued.

Thanks


> thanks
> -- PMM
>