Skip to content

Commit

Permalink
mchp: Remove undefined behavior in espi msvw handlers
Browse files Browse the repository at this point in the history
The code in espi_msvw[12]_interrupt relies on undefined behavior today.
__builtin_ctz is specified as returning values in the range [0, 31], but
we are checking for 32.

This behavior may be unexpected compared to the CTZ/CLZ instruction on
ARM, which use the value 32 to indicate that there are no ones in the
provided input.

GCC 11+ optimizes the two loops below into infinite loops, as it can see
that the condition will never be met.

After this change, the disassembly of espi_mswv1_interrupt can be
confirmed to contain an exit behind a branch.

   ... // r4 is loaded with girq24_result and has bits successively cleared
   1a:   b90c            cbnz    r4, 20 <espi_mswv1_interrupt+0x20>
   1c:   e8bd 81f0       ldmia.w sp!, {r4, r5, r6, r7, r8, pc}
   20:   fa94 f5a4       rbit    r5, r4
   ...

BUG=EmbeddedController#21
BRANCH=hx20-hx30
TEST=Examined the disassembly for espi_msvw[12]_interrupt; see above

Signed-off-by: Dustin L. Howett <dustin@howett.net>
  • Loading branch information
DHowett committed Jan 5, 2023
1 parent 38c1b38 commit 23d6a81
Showing 1 changed file with 4 additions and 6 deletions.
10 changes: 4 additions & 6 deletions chip/mchp/espi.c
Original file line number Diff line number Diff line change
Expand Up @@ -1238,13 +1238,12 @@ void espi_mswv1_interrupt(void)
girq24_result = MCHP_INT_RESULT(24);
MCHP_INT_SOURCE(24) = girq24_result;

bpos = __builtin_ctz(girq24_result); /* rbit, clz sequence */
while (bpos != 32) {
while (girq24_result) {
bpos = __builtin_ctz(girq24_result); /* rbit, clz sequence */
d = *(uint8_t *)(MCHP_ESPI_MSVW_BASE + 8 +
(12 * (bpos >> 2)) + (bpos & 0x03)) & 0x01;
(girq24_vw_handlers[bpos])(d, bpos);
girq24_result &= ~(1ul << bpos);
bpos = __builtin_ctz(girq24_result);
}
}
DECLARE_IRQ(MCHP_IRQ_GIRQ24, espi_mswv1_interrupt, 2);
Expand All @@ -1259,13 +1258,12 @@ void espi_msvw2_interrupt(void)
girq25_result = MCHP_INT_RESULT(25);
MCHP_INT_SOURCE(25) = girq25_result;

bpos = __builtin_ctz(girq25_result); /* rbit, clz sequence */
while (bpos != 32) {
while (girq25_result) {
bpos = __builtin_ctz(girq25_result); /* rbit, clz sequence */
d = *(uint8_t *)(MCHP_ESPI_MSVW_BASE + (12 * 7) + 8 +
(12 * (bpos >> 2)) + (bpos & 0x03)) & 0x01;
(girq25_vw_handlers[bpos])(d, bpos);
girq25_result &= ~(1ul << bpos);
bpos = __builtin_ctz(girq25_result);
}
}
DECLARE_IRQ(MCHP_IRQ_GIRQ25, espi_msvw2_interrupt, 2);
Expand Down

0 comments on commit 23d6a81

Please sign in to comment.