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

["H. Peter Anvin" <hpa@xxxxxxxxx>]

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