On Mon, Sep 13 2021 at 13:01, Sohil Mehta wrote:
> SENDUIPI is a special ring-3 instruction that makes a supervisor mode
> memory access to the UPID and UITT memory. Currently, KPTI needs to be
> off for User IPIs to work. Processors that support user interrupts are
> not affected by Meltdown so the auto mode of KPTI will default to off.
>
> Users who want to force enable KPTI will need to wait for a later
> version of this patch series that is compatible with KPTI. We need to
> allocate the UPID and UITT structures from a special memory region that
> has supervisor access but it is mapped into userspace. The plan is to
> implement a mechanism similar to LDT.
Seriously?
> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Signed-off-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>
This SOB chain is invalid. Ditto in several other patches.
>
> +config X86_USER_INTERRUPTS
> + bool "User Interrupts (UINTR)"
> + depends on X86_LOCAL_APIC && X86_64
X86_64 does not work w/o LOCAL_APIC so this dependency is pointless.
> + depends on CPU_SUP_INTEL
> + help
> + User Interrupts are events that can be delivered directly to
> + userspace without a transition through the kernel. The interrupts
> + could be generated by another userspace application, kernel or a
> + device.
> +
> + Refer, Documentation/x86/user-interrupts.rst for details.
"Refer, Documentation..." is not a sentence.
>
> +/* User Interrupt interface */
> +#define MSR_IA32_UINTR_RR 0x985
> +#define MSR_IA32_UINTR_HANDLER 0x986
> +#define MSR_IA32_UINTR_STACKADJUST 0x987
> +#define MSR_IA32_UINTR_MISC 0x988 /* 39:32-UINV, 31:0-UITTSZ */
Bah, these tail comments are crap. Please define proper masks/shift
constants for this instead of using magic numbers in the code.
> +static __always_inline void setup_uintr(struct cpuinfo_x86 *c)
This has to be always inline because it's performance critical or what?
> +{
> + /* check the boot processor, plus compile options for UINTR. */
Sentences start with uppercase letters.
> + if (!cpu_feature_enabled(X86_FEATURE_UINTR))
> + goto disable_uintr;
> +
> + /* checks the current processor's cpuid bits: */
> + if (!cpu_has(c, X86_FEATURE_UINTR))
> + goto disable_uintr;
> +
> + /*
> + * User Interrupts currently doesn't support PTI. For processors that
> + * support User interrupts PTI in auto mode will default to off. Need
> + * this check only for users who have force enabled PTI.
> + */
> + if (boot_cpu_has(X86_FEATURE_PTI)) {
> + pr_info_once("x86: User Interrupts (UINTR) not enabled. Please disable PTI using 'nopti' kernel parameter\n");
That message does not make sense. The admin has explicitly added 'pti'
to the kernel command line on a CPU which is not affected. So why would
he now have to add 'nopti' ?
Thanks,
tglx
