Re: [patch V2 09/29] x86/exceptions: Add structs for exception stacks

[Thomas Gleixner]

On Fri, 5 Apr 2019, Sean Christopherson wrote:
> On Fri, Apr 05, 2019 at 05:07:07PM +0200, Thomas Gleixner wrote:
> > +#ifdef CONFIG_X86_64
> > +
> > +/* Macro to enforce the same ordering and stack sizes */
> > +#define ESTACKS_MEMBERS(guardsize)		\
> > +	char	DF_stack[EXCEPTION_STKSZ];	\
> > +	char	DF_stack_guard[guardsize];	\
> > +	char	NMI_stack[EXCEPTION_STKSZ];	\
> > +	char	NMI_stack_guard[guardsize];	\
> > +	char	DB_stack[DEBUG_STKSZ];		\
> > +	char	DB_stack_guard[guardsize];	\
> > +	char	MCE_stack[EXCEPTION_STKSZ];	\
> > +	char	MCE_stack_guard[guardsize];	\
> 
> Conceptually, shouldn't the stack guard precede its associated stack
> since the stacks grow down?  And don't we want a guard page below the
> DF_stack?  There could still be a guard page above MCE_stack,
> e.g. IST_stack_guard or something.

Yes and no. :)

Defacto we have already a guard page below #DF. See struct
cpu_entry_area. And because I come from 8 bit microcontrollers, it's just
an instinct to spare/share stuff whereever its possible.

But yes, it looks a bit odd and we can reorder that and have an extra guard
page below the first stack.

> > +#define CEA_ESTACK_TOP(ceastp, st)			\
> > +	((unsigned long)&(ceastp)->st## _stack_guard)
> 
> IMO, using the stack guard to define the top of stack is unnecessarily
> confusing and fragile, e.g. reordering the names of the stack guards
> would break this macro.

For me it's obvious, obviously :)

> What about:
> 
> #define CEA_ESTACK_TOP(ceastp, st)			\
> 	(CEA_ESTACK_BOT(ceastp, st) + CEA_ESTACK_SIZE(st))

Yeah. No problem.

Thanks,

	tglx