[PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[Oleksii Kurochko]

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.0

Re: [PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[Jan Beulich]

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.

Jan

Re: [PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[Oleksii Kurochko]

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

Re: [PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[Jan Beulich]

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

Re: [PATCH v1] xen/riscv: route unhandled interrupts to do_unexpected_trap()

[Oleksii Kurochko]

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