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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan