tests: kernel: interrupt: Rework nested interrupt test #23614

stephanosio · 2020-03-19T17:26:53Z

The current nested interrupt test implementation is both buggy and
fundamentally flawed because it does not trigger a higher priority
interrupt from a lower priority interrupt context and relies on the
system timer interrupt, which is not fully governed by the test;
moreover, the current implementation does not properly validate the
test results and can report success if no interrupt is triggered and
serviced at all.

This commit reworks this test to have the following well-defined
and logical procedure:

[thread] Trigger IRQ 0 (lower priority)
[isr0] Set ISR 0 result token and trigger IRQ 1 (higher priority)
[isr1] Set ISR 1 result token and return
[isr0] Validate ISR 1 result token and return
[thread] Validate ISR 0 result token

The reworked test scenario ensures that the interrupt nesting works
properly and any abnormal conditions are detected (e.g. interrupts not
triggering at all, or ISR 1 not being nested under ISR 0).

Signed-off-by: Stephanos Ioannidis [email protected]

Closes #23593

zephyrbot · 2020-03-19T17:29:26Z

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

stephanosio · 2020-03-19T17:30:47Z

Output from the reworked test:

starting test - test_nested_isr
Triggering irq : 42
isr0: Enter
Triggering irq : 41
isr1: Enter
isr1: Leave
isr0: Leave
PASS - test_nested_isr

Output from the old (very broken) test:

starting test - test_nested_isr
isr0 running !!
Triggering irq : 41
isr1 ran !!
isr0 execution completed !!
Triggering irq : 42
isr0 running !!
isr0 execution completed !!
PASS - test_nested_isr

stephanosio · 2020-03-19T17:35:00Z

NOTE: Shippable will fail for this test until #23593 is fixed, because the new test does not lie.

nashif · 2020-03-19T22:01:55Z

This commit reworks this test to have the following well-defined
and logical procedure:

1. [thread] Trigger IRQ 0 (lower priority)

2. [isr0] Set ISR 0 result token and trigger IRQ 1 (higher priority)

3. [isr1] Set ISR 1 result token and return

4. [isr0] Validate ISR 1 result token and return

5. [thread] Validate ISR 0 result token

Can you put this in the test documentation via a doxygen header somewhere.

This commit re-organises the kernel interrupt tests for consistency. In addition, it removes any references to the `irq_offload` feature, which is no longer used by this test. Signed-off-by: Stephanos Ioannidis <[email protected]>

The current nested interrupt test implementation is both buggy and fundamentally flawed because it does not trigger a higher priority interrupt from a lower priority interrupt context and relies on the system timer interrupt, which is not fully governed by the test; moreover, the current implementation does not properly validate the test results and can report success if no interrupt is triggered and serviced at all. This commit reworks this test to have the following well-defined and logical procedure: 1. [thread] Trigger IRQ 0 (lower priority) 2. [isr0] Set ISR 0 result token and trigger IRQ 1 (higher priority) 3. [isr1] Set ISR 1 result token and return 4. [isr0] Validate ISR 1 result token and return 5. [thread] Validate ISR 0 result token The reworked test scenario ensures that the interrupt nesting works properly and any abnormal conditions are detected (e.g. interrupts not triggering at all, or ISR 1 not being nested under ISR 0). Signed-off-by: Stephanos Ioannidis <[email protected]>

This commit disables the nested interrupt test for the RISC-V platform, as interrupt nesting is not supported on the current RISV-C architecture port. Furthermore, the current `trigger_irq` implementation for RISC-V is mostly incorrect and cannot be used, so there is no point in leaving that in the codebase (see zephyrproject-rtos#23593). Signed-off-by: Stephanos Ioannidis <[email protected]>

stephanosio · 2020-03-20T07:11:23Z

Can you put this in the test documentation via a doxygen header somewhere.

@nashif I have added the documentation for test_nested_isr.

stephanosio · 2020-03-20T07:16:28Z

Force pushing for further clean-up and adding documentation.

Also, I have added an additional commit that disables nested interrupt test for the RISC-V, as that seems to be not supported by the arch port and, even if it is supported, there is no practical way to test that in software because CLINT does not support priority-based interrupt nesting/preemption (for more details, see #23593).

stephanosio · 2020-03-25T00:17:58Z

@npitre @kgugala @pgielda @nategraff-sifive It would be very helpful if one of the RISC-V maintainers could comment on the removal of the (previously broken and still is) nested interrupt testing for the RISC-V architectures.

tests/kernel/interrupt/prj.conf

ioannisg · 2020-03-27T10:49:56Z

tests/kernel/interrupt/src/dynamic_isr.c

+ */
+#if (defined(CONFIG_DYNAMIC_INTERRUPTS)		\
+	&& defined(CONFIG_GEN_SW_ISR_TABLE))
+#define DYNTEST 1


@stephanosio i wonder if you could clean up this local macro define - and use directly the corresponding CONFIG_ macro defines.

Sure, I will address that in the PR for #23614 (comment) as well.

ioannisg · 2020-03-27T10:59:07Z

tests/kernel/interrupt/src/nested_irq.c

-#define ISR1_PRIO 1
+/*
+ * For Cortex-M NVIC, unused and available IRQs are automatically detected when
+ * when the test is run.


double "when"

I will fix that in another PR since this already got merged.

Addressed in #23881

ioannisg · 2020-03-27T10:59:45Z

tests/kernel/interrupt/src/nested_irq.c

+ * and must be specified for every supported platform.
+ *
+ * In terms of priority, the IRQ1 is triggered from the ISR of the IRQ0;
+ * therefore, the priority of IRQ1 must be greater than that of the IRQ0.


"greater than" is this Cortex-specific requirement or applies to all? Just asking

"greater" here means "greater in priority" and not "greater in numerical value," so it applies universally.

ioannisg · 2020-03-27T11:05:49Z

tests/kernel/interrupt/src/nested_irq.c

+ * For Cortex-M NVIC, unused and available IRQs are automatically detected when
+ * when the test is run.
+ */
+#define IRQ0_PRIO	2


@stephanosio what is the ultimate reason that we need different priorities for Cortex-M compared to all other ARCHes?

Mainly, I just copied what it was doing before, but the "ultimate reason" would be:

zephyr/include/arch/arm/aarch32/exc.h

Line 44 in 685bf54

#define _EXCEPTION_RESERVED_PRIO 0

But now that I am looking at it, it might have been better to do _EXCEPTION_RESERVED_PRIO + N here since the reserved priority offset is 1 instead of 0 if CONFIG_CPU_CORTEX_M_HAS_PROGRAMMABLE_FAULT_PRIOS is set.

it might have been better to do _EXCEPTION_RESERVED_PRIO + N

Actually, this is not necessary because z_arm_irq_priority_set already does that:

zephyr/arch/arm/core/aarch32/irq_manage.c

Line 84 in 685bf54

prio += _IRQ_PRIO_OFFSET;

We should be okay as long as we do not use the priority value 0; I will add a comment about this in the upcoming PR.

Addressed in #23881

ioannisg · 2020-03-27T11:24:01Z

tests/kernel/interrupt/src/interrupt_util.h

@@ -55,19 +60,8 @@ static void trigger_irq(int irq)
 #endif
 }

-#elif defined(CONFIG_RISCV)


@stephanosio can you make a note that for RISCv the test does not run any more and link to the respective github issue?

There is an issue about this:
#23593

As far as I can see, since this test should have never been enabled for RISC-V in the first place, I think this is really a non-issue.

#23593 (comment)

ioannisg

Looks OK, I have some minor questions and comments for improvement
@stephanosio still the nested-test runs only for Cortex-M - not Cortex-R, correct?

carlocaione · 2020-03-27T15:17:20Z

This was already tested on ARM64. ACK.

stephanosio · 2020-03-28T01:40:00Z

Looks OK, I have some minor questions and comments for improvement
@stephanosio still the nested-test runs only for Cortex-M - not Cortex-R, correct?

@ioannisg This test runs on Cortex-R (and future AArch32 Cortex-A) as well, given that the system uses GIC (qemu_cortex_r5 does).

stephanosio requested review from andrewboie, andyross and nashif as code owners March 19, 2020 17:26

stephanosio added area: Architectures area: Kernel area: Tests Issues related to a particular existing or missing test labels Mar 19, 2020

stephanosio added this to the v2.3.0 milestone Mar 19, 2020

stephanosio requested a review from ioannisg March 19, 2020 17:27

andrewboie approved these changes Mar 19, 2020

View reviewed changes

stephanosio added the DNM This PR should not be merged (Do Not Merge) label Mar 19, 2020

stephanosio added 3 commits March 20, 2020 15:44

tests: kernel: interrupt: Reorganise tests

810c75a

This commit re-organises the kernel interrupt tests for consistency. In addition, it removes any references to the `irq_offload` feature, which is no longer used by this test. Signed-off-by: Stephanos Ioannidis <[email protected]>

stephanosio force-pushed the rework_nested_irq_test branch from e24b12a to f95b550 Compare March 20, 2020 07:10

stephanosio requested a review from npitre March 20, 2020 07:19

stephanosio mentioned this pull request Mar 20, 2020

Support nested interrupts on AArch32 (non-Cortex-M) #23636

Merged

stephanosio requested review from pgielda, nategraff-sifive and kgugala March 25, 2020 00:16

stephanosio removed the DNM This PR should not be merged (Do Not Merge) label Mar 26, 2020

ioannisg mentioned this pull request Mar 27, 2020

ARM: Core Stack Improvements/Bug fixes for 2.3 release #23366

Closed

17 tasks

ioannisg reviewed Mar 27, 2020

View reviewed changes

tests/kernel/interrupt/prj.conf Show resolved Hide resolved

ioannisg reviewed Mar 27, 2020

View reviewed changes

ioannisg requested a review from carlocaione March 27, 2020 15:11

carlocaione approved these changes Mar 27, 2020

View reviewed changes

nashif merged commit e57da82 into zephyrproject-rtos:master Mar 27, 2020

stephanosio deleted the rework_nested_irq_test branch April 23, 2020 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: kernel: interrupt: Rework nested interrupt test #23614

tests: kernel: interrupt: Rework nested interrupt test #23614

stephanosio commented Mar 19, 2020 •

edited

Loading

zephyrbot commented Mar 19, 2020 •

edited

Loading

stephanosio commented Mar 19, 2020

stephanosio commented Mar 19, 2020

nashif commented Mar 19, 2020

stephanosio commented Mar 20, 2020

stephanosio commented Mar 20, 2020

stephanosio commented Mar 25, 2020

ioannisg Mar 27, 2020

stephanosio Mar 28, 2020

ioannisg Mar 27, 2020

stephanosio Mar 28, 2020

stephanosio Mar 28, 2020

ioannisg Mar 27, 2020

stephanosio Mar 28, 2020

ioannisg Mar 27, 2020 •

edited

Loading

stephanosio Mar 28, 2020

stephanosio Mar 28, 2020

stephanosio Mar 28, 2020

ioannisg Mar 27, 2020

stephanosio Mar 28, 2020

ioannisg left a comment

carlocaione commented Mar 27, 2020

stephanosio commented Mar 28, 2020

tests: kernel: interrupt: Rework nested interrupt test #23614

tests: kernel: interrupt: Rework nested interrupt test #23614

Conversation

stephanosio commented Mar 19, 2020 • edited Loading

zephyrbot commented Mar 19, 2020 • edited Loading

stephanosio commented Mar 19, 2020

stephanosio commented Mar 19, 2020

nashif commented Mar 19, 2020

stephanosio commented Mar 20, 2020

stephanosio commented Mar 20, 2020

stephanosio commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg left a comment

Choose a reason for hiding this comment

carlocaione commented Mar 27, 2020

stephanosio commented Mar 28, 2020

stephanosio commented Mar 19, 2020 •

edited

Loading

zephyrbot commented Mar 19, 2020 •

edited

Loading

ioannisg Mar 27, 2020 •

edited

Loading