Re: [PATCH v1 11/14] xen/riscv: add external interrupt handling for hypervisor mode

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3198 bytes --]


On 4/15/25 4:42 PM, Jan Beulich wrote:
>
>> +    /* clear the pending bit */
>> +    csr_clear(CSR_SIP, 1UL << IRQ_S_EXT);
>> +
>> +    /* dispatch the interrupt */
>> +    do_IRQ(regs, csr_swap(CSR_STOPEI, 0) >> TOPI_IID_SHIFT);
>> +
>> +    /* enable external interrupts */
>> +    csr_set(CSR_SIE, 1UL << IRQ_S_EXT);
>> +}
> Why does "cause" need passing into here? I realize the function is used ...
>
>> @@ -278,6 +293,7 @@ static const struct intc_hw_operations aplic_ops = {
>>       .host_irq_type       = &aplic_host_irq_type,
>>       .set_irq_priority    = aplic_set_irq_priority,
>>       .set_irq_type        = aplic_set_irq_type,
>> +    .handle_interrupt    = aplic_handle_interrupt,
>>   };
> ... as a hook, yet it's still unclear whether (why) any other such hook
> would need the cause to be passed in.

I don't remember a particular reason, but it could have been dropped. If, for some reason,
the cause is needed in|handle_interrupt()|, it can be retrieved explicitly from a register.

>
>> @@ -33,6 +44,20 @@ do {                            \
>>       csr_clear(CSR_SIREG, v);    \
>>   } while (0)
>>   
>> +void imsic_ids_local_delivery(bool enable)
> __init as long as the sole caller is such?

Yes, it make sense. Also, I noticed some other functions that could be __init in imsic.c (but likely
you mentioned that in the previous patches).

>> --- a/xen/arch/riscv/intc.c
>> +++ b/xen/arch/riscv/intc.c
>> @@ -51,6 +51,13 @@ static void intc_set_irq_priority(struct irq_desc *desc, unsigned int priority)
>>       intc_hw_ops->set_irq_priority(desc, priority);
>>   }
>>   
>> +void intc_handle_external_irqs(unsigned long cause, struct cpu_user_regs *regs)
>> +{
>> +    ASSERT(intc_hw_ops && intc_hw_ops->handle_interrupt);
> I don't view such checks (on every interrupt) as very useful. If you checked
> once early on - okay. But here you gain nothing at all ...
>
>> +    intc_hw_ops->handle_interrupt(cause, regs);
> ... towards the use here, when considering a release build.

I will try to find a better place then.

>
>
>> @@ -83,3 +87,42 @@ void __init init_IRQ(void)
>>       if ( init_irq_data() < 0 )
>>           panic("initialization of IRQ data failed\n");
>>   }
>> +
>> +/* Dispatch an interrupt */
>> +void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
>> +{
>> +    struct irq_desc *desc = irq_to_desc(irq);
>> +    struct irqaction *action;
>> +
>> +    irq_enter();
>> +
>> +    spin_lock(&desc->lock);
>> +    desc->handler->ack(desc);
>> +
>> +    if ( test_bit(_IRQ_DISABLED, &desc->status) )
>> +        goto out;
>> +
>> +    set_bit(_IRQ_INPROGRESS, &desc->status);
> Same comment as on the earlier patch - atomic bitop when in a suitably
> locked region?

Agree, it could be used non-atomic bitop.

>
>> +    action = desc->action;
>> +
>> +    spin_unlock_irq(&desc->lock);
>> +
>> +#ifndef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> Stolen from Arm? What's this about?

Yes, it is stolen from Arm. I thought that it is a generic one, but the config is defined
inside Arm's config.h.
Then it could be dropped now as I don't know, at the moment, the cases when it is neeeded
to exectute several handler for an irq for RISC-V.

Thanks.

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 5183 bytes --]