Currently, an interrupt cause which is not explicitly handled is silently
ignored, and execution resumes without reporting the fault. This is
incorrect and do_unexpected_trap() should be called in the case of
unhandled interrupt.
Fixes: a8b85fabf6090 ("xen/riscv: add external interrupt handling for hypervisor mode")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/traps.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 84b5ab4142f6..34920f4e5693 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -196,6 +196,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
{
/* Handle interrupt */
unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
+ bool intr_handled = true;
switch ( icause )
{
@@ -204,10 +205,12 @@ void do_trap(struct cpu_user_regs *cpu_regs)
break;
default:
+ intr_handled = false;
break;
}
- break;
+ if ( intr_handled )
+ break;
}
do_unexpected_trap(cpu_regs);
--
2.52.0On 29.01.2026 15:40, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -196,6 +196,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
> {
> /* Handle interrupt */
> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
> + bool intr_handled = true;
Of course I don't know what your further plans are here, so maybe doing
it this way really is desirable. As the code is right now, I wonder if
you couldn't make this a 2-line change, ...
> @@ -204,10 +205,12 @@ void do_trap(struct cpu_user_regs *cpu_regs)
> break;
... using return here and ...
> default:
> + intr_handled = false;
> break;
> }
>
> - break;
> + if ( intr_handled )
> + break;
... simply dropping this break altogether.
Jan
On 1/29/26 4:43 PM, Jan Beulich wrote:
> On 29.01.2026 15:40, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/traps.c
>> +++ b/xen/arch/riscv/traps.c
>> @@ -196,6 +196,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>> {
>> /* Handle interrupt */
>> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
>> + bool intr_handled = true;
> Of course I don't know what your further plans are here, so maybe doing
> it this way really is desirable. As the code is right now, I wonder if
> you couldn't make this a 2-line change, ...
>
>> @@ -204,10 +205,12 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>> break;
> ... using return here and ...
>
>> default:
>> + intr_handled = false;
>> break;
>> }
>>
>> - break;
>> + if ( intr_handled )
>> + break;
> ... simply dropping this break altogether.
Well, your change is better but it won't apply to my current code of do_trap():
....
default:
if ( cause & CAUSE_IRQ_FLAG )
{
/* Handle interrupt */
unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
bool intr_handled = true;
switch ( icause )
{
case IRQ_S_EXT:
intc_handle_external_irqs(cpu_regs);
break;
...
default:
intr_handled = false;
break;
}
if ( intr_handled )
break;
}
do_unexpected_trap(cpu_regs);
break;
}
if ( cpu_regs->hstatus & HSTATUS_SPV )
check_for_pcpu_work();
}
So if to use return instead of break here, I will miss the call of check_for_pcpu_work()
which is syncing interrupt and check if some softirq should be done:
static void check_for_pcpu_work(void)
{
ASSERT(!local_irq_is_enabled());
while ( softirq_pending(smp_processor_id()) )
{
local_irq_enable();
do_softirq();
local_irq_disable();
}
vcpu_flush_interrupts(current);
vcpu_sync_interrupts(current);
}
~ Oleksii
On 29.01.2026 17:56, Oleksii Kurochko wrote:
>
> On 1/29/26 4:43 PM, Jan Beulich wrote:
>> On 29.01.2026 15:40, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -196,6 +196,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>> {
>>> /* Handle interrupt */
>>> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
>>> + bool intr_handled = true;
>> Of course I don't know what your further plans are here, so maybe doing
>> it this way really is desirable. As the code is right now, I wonder if
>> you couldn't make this a 2-line change, ...
>>
>>> @@ -204,10 +205,12 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>> break;
>> ... using return here and ...
>>
>>> default:
>>> + intr_handled = false;
>>> break;
>>> }
>>>
>>> - break;
>>> + if ( intr_handled )
>>> + break;
>> ... simply dropping this break altogether.
>
> Well, your change is better but it won't apply to my current code of do_trap():
> ....
> default:
> if ( cause & CAUSE_IRQ_FLAG )
> {
> /* Handle interrupt */
> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
> bool intr_handled = true;
>
> switch ( icause )
> {
> case IRQ_S_EXT:
> intc_handle_external_irqs(cpu_regs);
> break;
> ...
> default:
> intr_handled = false;
> break;
> }
>
> if ( intr_handled )
> break;
> }
>
> do_unexpected_trap(cpu_regs);
> break;
> }
>
> if ( cpu_regs->hstatus & HSTATUS_SPV )
> check_for_pcpu_work();
> }
>
> So if to use return instead of break here, I will miss the call of check_for_pcpu_work()
Ah, I see. But how should I have known without the description saying anything
along these lines?
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
On 1/29/26 6:03 PM, Jan Beulich wrote:
> On 29.01.2026 17:56, Oleksii Kurochko wrote:
>> On 1/29/26 4:43 PM, Jan Beulich wrote:
>>> On 29.01.2026 15:40, Oleksii Kurochko wrote:
>>>> --- a/xen/arch/riscv/traps.c
>>>> +++ b/xen/arch/riscv/traps.c
>>>> @@ -196,6 +196,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>>> {
>>>> /* Handle interrupt */
>>>> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
>>>> + bool intr_handled = true;
>>> Of course I don't know what your further plans are here, so maybe doing
>>> it this way really is desirable. As the code is right now, I wonder if
>>> you couldn't make this a 2-line change, ...
>>>
>>>> @@ -204,10 +205,12 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>>> break;
>>> ... using return here and ...
>>>
>>>> default:
>>>> + intr_handled = false;
>>>> break;
>>>> }
>>>>
>>>> - break;
>>>> + if ( intr_handled )
>>>> + break;
>>> ... simply dropping this break altogether.
>> Well, your change is better but it won't apply to my current code of do_trap():
>> ....
>> default:
>> if ( cause & CAUSE_IRQ_FLAG )
>> {
>> /* Handle interrupt */
>> unsigned long icause = cause & ~CAUSE_IRQ_FLAG;
>> bool intr_handled = true;
>>
>> switch ( icause )
>> {
>> case IRQ_S_EXT:
>> intc_handle_external_irqs(cpu_regs);
>> break;
>> ...
>> default:
>> intr_handled = false;
>> break;
>> }
>>
>> if ( intr_handled )
>> break;
>> }
>>
>> do_unexpected_trap(cpu_regs);
>> break;
>> }
>>
>> if ( cpu_regs->hstatus & HSTATUS_SPV )
>> check_for_pcpu_work();
>> }
>>
>> So if to use return instead of break here, I will miss the call of check_for_pcpu_work()
> Ah, I see. But how should I have known without the description saying anything
> along these lines?
Of course, without proper description it was impossible to understand that.
> Acked-by: Jan Beulich <jbeulich@suse.com>
Thanks.
~ Oleksii
© 2016 - 2026 Red Hat, Inc.