Re: [PATCHv2 1/5] arm64/entry-common: push the judgement of nmi ahead

From: Mark Rutland <mark.rutland@arm.com>
To: Pingfan Liu <kernelfans@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	Joey Gouly <joey.gouly@arm.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Julien Thierry <julien.thierry@arm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Yuichi Ito <ito-yuichi@fujitsu.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCHv2 1/5] arm64/entry-common: push the judgement of nmi ahead
Date: Thu, 30 Sep 2021 14:32:57 +0100	[thread overview]
Message-ID: <20210930133257.GB18258@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <YU9Cy9kTew4ySeGZ@piliu.users.ipa.redhat.com>

On Sat, Sep 25, 2021 at 11:39:55PM +0800, Pingfan Liu wrote:
> On Fri, Sep 24, 2021 at 06:53:06PM +0100, Mark Rutland wrote:
> > On Fri, Sep 24, 2021 at 09:28:33PM +0800, Pingfan Liu wrote:
> > > In enter_el1_irq_or_nmi(), it can be the case which NMI interrupts an
> > > irq, which makes the condition !interrupts_enabled(regs) fail to detect
> > > the NMI. This will cause a mistaken account for irq.
> > 
> Sorry about the confusing word "account", it should be "lockdep/rcu/.."
> 
> > Can you please explain this in more detail? It's not clear which
> > specific case you mean when you say "NMI interrupts an irq", as that
> > could mean a number of distinct scenarios.
> > 
> > AFAICT, if we're in an IRQ handler (with NMIs unmasked), and an NMI
> > causes a new exception we'll do the right thing. So either I'm missing a
> > subtlety or you're describing a different scenario..
> > 
> > Note that the entry code is only trying to distinguish between:
> > 
> > a) This exception is *definitely* an NMI (because regular interrupts
> >    were masked).
> > 
> > b) This exception is *either* and IRQ or an NMI (and this *cannot* be
> >    distinguished until we acknowledge the interrupt), so we treat it as
> >    an IRQ for now.
> > 
> b) is the aim.
> 
> At the entry, enter_el1_irq_or_nmi() -> enter_from_kernel_mode()->rcu_irq_enter()/rcu_irq_enter_check_tick() etc.
> While at irqchip level, gic_handle_irq()->gic_handle_nmi()->nmi_enter(),
> which does not call rcu_irq_enter_check_tick(). So it is not proper to
> "treat it as an IRQ for now"

I'm struggling to understand the problem here. What is "not proper", and
why?

Do you think there's a correctness problem, or that we're doing more
work than necessary? 

If you could give a specific example of a problem, it would really help.

I'm aware that we do more work than strictly necessary when we take a
pNMI from a context with IRQs enabled, but that's how we'd intended this
to work, as it's vastly simpler to manage the state that way. Unless
there's a real problem with that approach I'd prefer to leave it as-is.

Thanks,
Mark.