On Fri, Nov 29 2024 at 11:31, Eliav Farber 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.
> +void machine_kexec_mask_interrupts(void)
> +{
> + unsigned int i;
> + struct irq_desc *desc;
struct irq_desc *desc;
unsigned int i;
please
> + 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?
> + /*
> + * 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.
> + */
> + 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?
> + }
> +
> + if (check_eoi && chip->irq_eoi &&
> irqd_irq_inprogress(&desc->irq_data))
> + chip->irq_eoi(&desc->irq_data);
Thanks,
tglx
