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