Re: [PATCH v1 10/17] xen/riscv: introduce vintc_state_{save,restore}()

On 2026-08-18 10:31 +0200, Jan Beulich wrote:
> On 18.08.2026 10:28, Baptiste Le Duc wrote:
> > On 2026-08-17 10:31 +0200, Oleksii Kurochko wrote:
> >>
> >>
> >> On 8/13/26 11:42 AM, Baptiste Le Duc wrote:
> >>>>   #endif /* ASM__RISCV__INTERRUPT_CONTOLLER_H */
> >>>> diff --git a/xen/arch/riscv/intc.c b/xen/arch/riscv/intc.c
> >>>> index 372c8d3a20..879d513374 100644
> >>>> --- a/xen/arch/riscv/intc.c
> >>>> +++ b/xen/arch/riscv/intc.c
> >>>> @@ -163,3 +163,17 @@ bool vintc_reserve_virq(const struct domain *d, unsigned int virq)
> >>>>   
> >>>>       return !test_and_set_bit(virq, d->arch.vintc->used_irqs);
> >>>>   }
> >>>> +
> >>>> +void vintc_state_save(struct vcpu *vcpu)
> >>>> +{
> >>>> +    const struct vintc_ops *ops = vcpu->domain->arch.vintc->ops;
> >>> Is there a situation where ops could be NULL? If yes, add a check.
> >>
> >> It is unlikely that there is nothing to do during a context switch for 
> >> vINTC, so vINTC should provide an implementation for saving and 
> >> restoring its context. This also ensures that a NULL pointer dereference 
> >> will lead to a trap, allowing us to catch cases where a 
> >> context-switch/restore handler is missing.
> >>
> >> Even if it turns out that vINTC does not need to perform any actions 
> >> during a context switch, it is perfectly fine to provide an empty 
> >> implementation. However, as mentioned above, this is unlikely. 
> >> Therefore, having a NULL pointer dereference here is intentional: it 
> >> helps catch cases where someone adds a new interrupt controller driver 
> >> but forgets to implement the corresponding context switch functionality.
> > 
> > Thanks for these explanations. However, wouldn't it be better to have a
> > dedicated BUG_ON in case of NULL dereference to indicate clean call
> > trace to people who missed to implement context-switch functionality?
> 
> How would BUG_ON() provide any better (or worse) call trace, compared to
> a NULL deref?

I wanted the file:line and function printed directly, but sepc in the
trap's register dump resolves to the same place, and BUG_ON() ends up in
the same handler anyway. Fair enough, dropping it.

Thanks,
Baptiste

> 
> Jan
> 
> 
>