On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
>
> +/*
> + * The actual assembly entry and exit points
> + */
> +extern __visible void fred_entrypoint_user(void);
Why is this defined in this patch and not at the point where the
function is introduced?
> +/*
> + * Initialization
> + */
> +void cpu_init_fred_exceptions(void);
> +void fred_setup_apic(void);
> +
> #endif /* __ASSEMBLY__ */
>
> +#else
> +#define cpu_init_fred_exceptions() BUG()
> +#define fred_setup_apic() BUG()
static inline stubs please.
> @@ -2054,28 +2055,6 @@ static void wrmsrl_cstar(unsigned long val)
> /* May not be marked __init: used by software suspend */
> void syscall_init(void)
> {
> - wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
> - wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
> -
> -#ifdef CONFIG_IA32_EMULATION
> - wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
> - /*
> - * This only works on Intel CPUs.
> - * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
> - * This does not cause SYSENTER to jump to the wrong location, because
> - * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
> - */
> - wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
> - wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> - (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
> - wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
> -#else
> - wrmsrl_cstar((unsigned long)ignore_sysret);
> - wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> - wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> - wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> -#endif
> -
> /*
> * Flags to clear on syscall; clear as much as possible
> * to minimize user space-kernel interference.
> @@ -2086,6 +2065,41 @@ void syscall_init(void)
> X86_EFLAGS_IF|X86_EFLAGS_DF|X86_EFLAGS_OF|
> X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_RF|
> X86_EFLAGS_AC|X86_EFLAGS_ID);
> +
> + /*
> + * The default user and kernel segments
> + */
> + wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
> +
> + if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> + /* Both sysexit and sysret cause #UD when FRED is enabled */
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> + } else {
> + wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
> +
> +#ifdef CONFIG_IA32_EMULATION
> + wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
> + /*
> + * This only works on Intel CPUs.
> + * On AMD CPUs these MSRs are 32-bit, CPU truncates
> + * MSR_IA32_SYSENTER_EIP.
> + * This does not cause SYSENTER to jump to the wrong
> + * location, because AMD doesn't allow SYSENTER in
> + * long mode (either 32- or 64-bit).
> + */
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> + (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
> +#else
> + wrmsrl_cstar((unsigned long)ignore_sysret);
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> +#endif
> + }
> }
Sigh. Can you please split this into
static void idt_syscall_init(void)
{
All the existing gunk
}
void syscall_init(void)
{
/* The default user and kernel segments */
wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
idt_syscall_init();
}
in a first step and then in the next patch add the FRED muck?
> +/*
> + * Initialize FRED on this CPU. This cannot be __init as it is called
> + * during CPU hotplug.
Really no need to repeat this comment vs. __init all over the place.
> + */
> +void cpu_init_fred_exceptions(void)
> +{
> + wrmsrl(MSR_IA32_FRED_CONFIG,
> + FRED_CONFIG_REDZONE | /* Reserve for CALL emulation */
Please don't use tail comments. Nowhere.
> + FRED_CONFIG_INT_STKLVL(0) |
> + FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user));
> +
> +/*
> + * Initialize system vectors from a FRED perspective, so
> + * lapic_assign_system_vectors() can do its job.
> + */
> +void __init fred_setup_apic(void)
> +{
> + int i;
> +
> + for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
> + set_bit(i, system_vectors);
> + /*
> + * Don't set the non assigned system vectors in the
> + * system_vectors bitmap. Otherwise they show up in
> + * /proc/interrupts.
> + */
> +#ifdef CONFIG_SMP
> + set_bit(IRQ_MOVE_CLEANUP_VECTOR, system_vectors);
> +#endif
> +
> + for (i = 0; i < NR_SYSTEM_VECTORS; i++) {
> + if (get_system_interrupt_handler(i) != NULL) {
This _cannot be NULL. The system vector table must be fully populated
with either the real handler or the spurious handler. Otherwise you need
a NULL pointer check in the dispatch path.
> + set_bit(i + FIRST_SYSTEM_VECTOR, system_vectors);
> + }
> + }
> +
> + /* The rest are fair game... */
Can you please refrain from adding useless comments. Commenting the
obvious is a distraction and not helpful in any way. Comment the things
which are not obvious in the first place.
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1537,6 +1537,14 @@ static system_interrupt_handler system_interrupt_handlers[NR_SYSTEM_VECTORS] = {
>
> #undef SYSV
>
> +system_interrupt_handler get_system_interrupt_handler(unsigned int i)
> +{
> + if (i >= NR_SYSTEM_VECTORS)
> + return NULL;
Seriously?
> + return system_interrupt_handlers[i];
Get rid of this completely confusing and useless function and look the
table up at the only call site. I'm all for defensive programming, but
this is hideous.
Thanks,
tglx
