Re: [patch V4 part 1 27/36] arm64: Prepare arch_nmi_enter() for recursion

[Will Deacon]

On Fri, May 15, 2020 at 04:04:39PM +0200, Frederic Weisbecker wrote:
> On Wed, May 13, 2020 at 07:28:34PM -0400, Mathieu Desnoyers wrote:
> > ----- On May 5, 2020, at 9:16 AM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:
> > 
> > > +#define arch_nmi_enter()						\
> > [...]							\
> > > +	___hcr = read_sysreg(hcr_el2);					\
> > > +	if (!(___hcr & HCR_TGE)) {					\
> > > +		write_sysreg(___hcr | HCR_TGE, hcr_el2);		\
> > > +		isb();							\
> > 
> > Why is there an isb() above ^ ....
> > 
> > > +	}								\
> > > +	/*								\
> > [...]
> > > -#define arch_nmi_exit()								\
> > [...]
> > > +	/*								\
> > > +	 * Make sure ___ctx->cnt release is visible before we		\
> > > +	 * restore the sysreg. Otherwise a new NMI occurring		\
> > > +	 * right after write_sysreg() can be fooled and think		\
> > > +	 * we secured things for it.					\
> > > +	 */								\
> > > +	barrier();							\
> > > +	if (!___ctx->cnt && !(___hcr & HCR_TGE))			\
> > > +		write_sysreg(___hcr, hcr_el2);				\
> > 
> > And not here ?
> 
> I have to defer to Will on this detail...

I think it's because we have to make sure that the register update has
taken effect before we can safely run the NMI handler (and so an ISB is
needed), but on the return path the exception return back to the interrupted
context has an implicit ISB so there's no need for an extra one here.

Make sense?

Will