Skip to content

mchp: Remove undefined behavior in espi msvw handlers #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 10, 2023
Merged

mchp: Remove undefined behavior in espi msvw handlers #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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=EmbeddedController#21
BRANCH=hx20-hx30
TEST=Examined the disassembly for espi_msvw[12]_interrupt; see above

Signed-off-by: Dustin L. Howett <[email protected]>
  • Loading branch information
@DHowett
DHowett committed Jan 5, 2023
commit 23d6a818f6641a7140349b2ef7d04d29f476e7d3
10 changes: 4 additions & 6 deletions chip/mchp/espi.c
View file
Open in desktop
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