Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "H. Peter Anvin" <hpa@zytor.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        x86@kernel.org
Subject: Re: [PATCH v3 7/8] x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt
In-Reply-To: <20210519212154.511983-8-hpa@zytor.com>
References: <20210519212154.511983-1-hpa@zytor.com> <20210519212154.511983-8-hpa@zytor.com>
Date:   Thu, 20 May 2021 16:00:44 +0200
Message-ID: <87o8d5bj3n.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, May 19 2021 at 14:21, H. Peter Anvin wrote:
> The current IRQ vector allocation code should be "clean" and never
> issue a IRQ_MOVE_CLEANUP_VECTOR IPI for an interrupt that could still
> be pending. This should make it possible to move it to the "normal"
> system IRQ vector range. This should probably be a three-step process:
>
> 1. Introduce this WARN_ONCE() on this event ever occurring.
> 2. Remove the self-IPI hack.
> 3. Move the IRQ_MOVE_CLEANUP_VECTOR to the sysvec range.

Second thoughts on this.

Despite having a halfways sane mechanism now, that warning still can be
triggered for remapped interrupts which can be moved from task context.

Interrupt is bound to CPU1 and moved to CPU2.

CPU0               CPU1                 CPU2            Device

set_affinity()     local_irq_disable()
                                                        Raise interrupt
                      -> pending in IRR                 on CPU1
  remap to CPU2
  (after this point all interrupts will go to CPU2)

                                                        Raise interrupt
                                                        on CPU2
                                        handle_irq()
                                        send_cleanup_ipi()

                   local_irq_enable()
                   handle_irq()
                   handle_cleanup_ipi()

Now if we move the cleanup vector up (higher priority) then CPU1 will
have:

                   local_irq_enable()
                   handle_cleanup_ipi()
                      WARN(irq set in IRR)

Unlikely but bound to happen.

Adding the WARN_ON() to the current code is harmless because it can't
trigger. Let me think some more about that.

Thanks,

        tglx