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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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?

> 
> ~ Oleksii
> 
>