Re: [PATCH v7] um: Enable preemption in UML

[Anton Ivanov]

On 28/03/2024 09:27, Johannes Berg wrote:

@@ -23,7 +23,7 @@ struct thread_info {
        int                     preempt_count;  /* 0 => preemptable,
                                                   <0 => BUG */
        struct thread_info      *real_thread;    /* Points to non-IRQ stack */
-       unsigned long aux_fp_regs[FP_SIZE];     /* auxiliary fp_regs to 
save/restore
+       unsigned long aux_fp_regs[FP_SIZE] __aligned(64);       /* auxiliary 
fp_regs to save/restore
                                                   them out-of-band */

nit: that comment looks strange now, maybe pull it up before the member?

  /* auxiliary ... out-of-band */
  unsigned long aux_fp_regs[...] __aligned(64);

Ack.

+#ifdef CONFIG_64BIT
+       if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
+               __builtin_ia32_xsaveopt64(&current_thread_info()->aux_fp_regs, 
KNOWN_387_FEATURES);
+       else {
+               if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE)))
+                       
__builtin_ia32_xsave64(&current_thread_info()->aux_fp_regs, KNOWN_387_FEATURES);
+               else
+                       
__builtin_ia32_fxsave64(&current_thread_info()->aux_fp_regs);
+       }

Why not write this as a chain?

        if (likely(cpu_has(...))
                __builtin_ia32_xsaveopt64(...);
        else if (likely(cpu_has(...)))
                __builtin_ia32_xave64(...);
        else
                __builtin_ia32_fxsave64(...);


and IMHO pulling the "&current_thread_info()->aux_fp_regs" that appears
on all three of them into a local variable would make that more readable
too.

Ack.

+#else
+       if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
+               __builtin_ia32_xsaveopt(&current->aux_fp_regs, 
KNOWN_387_FEATURES);
+       else {
+               if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE)))
+                       __builtin_ia32_xsave(&current->aux_fp_regs, 
KNOWN_387_FEATURES);
+               else
+                       __builtin_ia32_fxsave(&current->aux_fp_regs);
+       }
+#endif

And all of the above also applies for 32-bit,

Ack.

+void kernel_fpu_end(void)

and this as well.

johannes

Will fix, rebase, retest and resubmit.

--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/