On 11/29/2024 3:30 PM, Thomas Gleixner wrote:
>> Move the machine_kexec_mask_interrupts function to a common location in
>> kernel/kexec_core.c, removing duplicate implementations from architecture
>> specific files (arch/arm, arch/arm64, arch/powerpc, and arch/riscv).
>
> Can you please move this into kernel/irq/kexec.c?
>
> It's pure interrupt core internal code and there is no point to make
> core internal functions visible to random other code just because.
Done (in v5 series)
>> +void machine_kexec_mask_interrupts(void)
>> +{
>> + unsigned int i;
>> + struct irq_desc *desc;
>
> struct irq_desc *desc;
> unsigned int i;
>
> please
Done (in v5 series)
>> + for_each_irq_desc(i, desc) {
>> + struct irq_chip *chip;
>> + int check_eoi = 1;
>> +
>> + chip = irq_desc_get_chip(desc);
>> + if (!chip)
>> + continue;
>> +
>> + if (IS_ENABLED(CONFIG_ARM64)) {
>
> This should not be CONFIG_ARM64. Add something like:
>
> config GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD
> bool
>
> and select this from ARM64?
Done (in v5 series)
>> + /*
>> + * First try to remove the active state. If this fails, try to EOI the
>> + * interrupt.
>
> This comment does not really explain what this is about. I know you
> copied it from the ARM64 implementation, but it should explain what this
> actually means. Something like:
>
> First try to remove the active state from an interrupt which is
> forwarded to a VM. If the interrupt is not forwarded, try to
> EOI the interrupt.
>
> or something like that.
Done (in v5 series)
>> + */
>> + check_eoi = irq_set_irqchip_state(i, IRQCHIP_STATE_ACTIVE, false);
>
> Looking deeper. This function actually cannot be called from this
> context. It does:
>
> irq_get_desc_buslock(irq, &flags, 0);
>
> which means for any interrupt which has an actual buslock implementation
> it will end up in a sleepable function and deadlock in the worst case.
>
> Marc?
I will wait for Marc's response regarding this issue.
Regardless, if any changes are required, I believe it would be better
to address them in a separate patch, as this behavior existed before my
modification.
Thanks, Eliav
