diff options
| author | Dustin L. Howett <dustin@howett.net> | 2022-12-18 10:13:00 -0600 |
|---|---|---|
| committer | Chromeos LUCI <chromeos-scoped@luci-project-accounts.iam.gserviceaccount.com> | 2023-01-23 17:29:35 +0000 |
| commit | 9ae74605306a5d5cb8e6f209fe62e09208e4577f (patch) | |
| tree | 37e2d27267d729ae744c830af42ea7f14b8dae17 | |
| parent | 51afaca321fc4df3277568fb241e0b804bb96a60 (diff) | |
| download | chrome-ec-9ae74605306a5d5cb8e6f209fe62e09208e4577f.tar.gz | |
mchp: Remove undefined behavior in espi msvw handlers
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=None
BRANCH=main
TEST=Examined the disassembly for espi_msvw[12]_interrupt; see above
Change-Id: I68a5c753233a17b6b0fb61a31f1eeccf78c00aba
Signed-off-by: Dustin L. Howett <dustin@howett.net>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4114450
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-by: Alexandru Stan <amstan@chromium.org>
| -rw-r--r-- | chip/mchp/espi.c | 10 |
1 files changed, 4 insertions, 6 deletions
diff --git a/chip/mchp/espi.c b/chip/mchp/espi.c index dce81448d7..552739abc7 100644 --- a/chip/mchp/espi.c +++ b/chip/mchp/espi.c @@ -1059,14 +1059,13 @@ static 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); @@ -1080,14 +1079,13 @@ static 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); |