Re: [PATCH v9 29/36] x86/fred: FRED entry/exit and dispatch code

["H. Peter Anvin" <hpa@zytor.com>]

On 7/30/23 23:41, Xin Li wrote:
> +static DEFINE_FRED_HANDLER(fred_other_default)
> +{
> +	regs->vector = X86_TRAP_UD;
> +	fred_emulate_fault(regs);
> +}
> +
> +static DEFINE_FRED_HANDLER(fred_syscall)
> +{
> +	regs->orig_ax = regs->ax;
> +	regs->ax = -ENOSYS;
> +	do_syscall_64(regs, regs->orig_ax);
> +}
> +
> +#if IS_ENABLED(CONFIG_IA32_EMULATION)
> +/*
> + * Emulate SYSENTER if applicable. This is not the preferred system
> + * call in 32-bit mode under FRED, rather int $0x80 is preferred and
> + * exported in the vdso.
> + */
> +static DEFINE_FRED_HANDLER(fred_sysenter)
> +{
> +	regs->orig_ax = regs->ax;
> +	regs->ax = -ENOSYS;
> +	do_fast_syscall_32(regs);
> +}
> +#else
> +#define fred_sysenter fred_other_default
> +#endif
> +
> +static DEFINE_FRED_HANDLER(fred_other)
> +{
> +	static const fred_handler user_other_handlers[FRED_NUM_OTHER_VECTORS] =
> +	{
> +		/*
> +		 * Vector 0 of the other event type is not used
> +		 * per FRED spec 5.0.
> +		 */
> +		[0]		= fred_other_default,
> +		[FRED_SYSCALL]	= fred_syscall,
> +		[FRED_SYSENTER]	= fred_sysenter
> +	};
> +
> +	user_other_handlers[regs->vector](regs);
> +}

OK, this is wrong.

Dispatching like fred_syscall() is only valid for syscall64, which means 
you have to check regs->l is set in addition to the correct regs->vector 
to determine validity.

Similarly, sysenter is only valid if regs->l is clear.

The best way is probably to drop the dispatch table here and just do an 
if ... else if ... else statement; gcc is smart enough that it will 
combine the vector test and the L bit test into a single mask and 
compare. This also allows stubs to be inlined.

However, emulating #UD on events other than wrong mode of SYSCALL and 
SYSENTER may be a bad idea. It would probably be better to invoke 
fred_bad_event() in that case.

Something like this:

+static DEFINE_FRED_HANDLER(fred_other_default)
+{
+	regs->vector = X86_TRAP_UD;
+	fred_emulate_fault(regs);
+}

1) rename this to fred_emulate_ud (since that is what it actually does.)

... then ...

	/* The compiler can fold these into a single test */

	if (likely(regs->vector == FRED_SYSCALL && regs->l)) {
		fred_syscall64(regs);
	} else if (likely(regs->vector == FRED_SYSENTER && !regs->l)) {
		fred_sysenter32(regs);
	} else if (regs->vector == FRED_SYSCALL ||
		   regs->vector == FRED_SYSENTER) {
		/* Invalid SYSCALL or SYSENTER instruction */
		fred_emulate_ud(regs);
	} else {
		/* Unknown event */
		fred_bad_event(regs);
	}

... or the SYSCALL64 and SYSENTER32 can be inlined with the appropriate 
comment (gcc will do so regardless.)

	-hpa



	-hpa