Re: [PATCH v6 8/8] x86/vdso: Move out the CPU initialization

[Ingo Molnar]

* Chang S. Bae  wrote:

> The CPU and node number will be written, as early enough,
> to the segment limit of per CPU data and TSC_AUX MSR entry.
> The information has been retrieved by vgetcpu in user space
> and will be also loaded from the paranoid entry, when
> FSGSBASE enabled.
>
> The new setup function is named after the getcpu(2) system
> call, and will be called during each CPU initialization
> (before setting up IST). It makes a facility useful to both
> the kernel and userspace unconditionally available much
> sooner.
> 
> The change brings a substantial code removal. The redundant
> setting of the segment in entry/vdso/vma.c and hotplug
> notifier are removed.

The title and the changelog is totally unreadable, full of grammar errors
which makes it actively misleading...

A good changelog should explain not what it does, but _why_ it is
done:

  x86/vdso: Initialize the CPU/node NR segment descriptor earlier

  Currently the CPU/node NR segment descriptor (GDT_ENTRY_CPU_NUMBER) is
  initialized relatively late during CPU init, from the vCPU code, which
  has a number of disadvantages, such as hotplug CPU notifiers and SMP
  cross-calls.

  Instead just initialize it much earlier, directly in cpu_init().

  This reduces complexity and increases robustness.

I've edited the changelog, but please keep this in mind for future submissions.

I also made a number of other cleanups to the code, will push them out
after some testing.

Thanks,

Ingo

[PATCH v6 8/8] x86/vdso: Move out the CPU initialization

[Chang S. Bae]

The CPU and node number will be written, as early enough,
to the segment limit of per CPU data and TSC_AUX MSR entry.
The information has been retrieved by vgetcpu in user space
and will be also loaded from the paranoid entry, when
FSGSBASE enabled.

The new setup function is named after the getcpu(2) system
call, and will be called during each CPU initialization
(before setting up IST). It makes a facility useful to both
the kernel and userspace unconditionally available much
sooner.

The change brings a substantial code removal. The redundant
setting of the segment in entry/vdso/vma.c and hotplug
notifier are removed.

Suggested-by: H. Peter Anvin 
Suggested-by: Andy Lutomirski 
Signed-off-by: Chang S. Bae 
Cc: Thomas Gleixner 
Cc: Dave Hansen 
Cc: Ingo Molnar 
Cc: Andi Kleen 
---
 arch/x86/entry/vdso/vma.c| 33 +
 arch/x86/kernel/cpu/common.c | 24 
 2 files changed, 25 insertions(+), 32 deletions(-)

diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 39b5584..3f9d43f 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -332,35 +332,6 @@ static __init int vdso_setup(char *s)
return 0;
 }
 __setup("vdso=", vdso_setup);
-#endif
-
-#ifdef CONFIG_X86_64
-static void vgetcpu_cpu_init(void *arg)
-{
-   int cpu = smp_processor_id();
-   struct desc_struct d = { };
-   unsigned long cpudata = vdso_encode_cpu_node(cpu, cpu_to_node(cpu));
-
-   if (static_cpu_has(X86_FEATURE_RDTSCP))
-   write_rdtscp_aux(cpudata);
-
-   /* Store CPU and node number in limit */
-   d.limit0 = cpudata;
-   d.limit1 = cpudata >> 16;
-
-   d.type = 5; /* RO data, expand down, accessed */
-   d.dpl = 3;  /* Visible to user code */
-   d.s = 1;/* Not a system segment */
-   d.p = 1;/* Present */
-   d.d = 1;/* 32-bit */
-
-   write_gdt_entry(get_cpu_gdt_rw(cpu), GDT_ENTRY_CPU_NUMBER, &d, 
DESCTYPE_S);
-}
-
-static int vgetcpu_online(unsigned int cpu)
-{
-   return smp_call_function_single(cpu, vgetcpu_cpu_init, NULL, 1);
-}
 
 static int __init init_vdso(void)
 {
@@ -370,9 +341,7 @@ static int __init init_vdso(void)
init_vdso_image(&vdso_image_x32);
 #endif
 
-   /* notifier priority > KVM */
-   return cpuhp_setup_state(CPUHP_AP_X86_VDSO_VMA_ONLINE,
-"x86/vdso/vma:online", vgetcpu_online, NULL);
+   return 0;
 }
 subsys_initcall(init_vdso);
 #endif /* CONFIG_X86_64 */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 1ac7e6e..359a422 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1660,6 +1660,29 @@ static void wait_for_master_cpu(int cpu)
 #endif
 }
 
+#ifdef CONFIG_X86_64
+static void setup_getcpu(int cpu)
+{
+   unsigned long cpudata = vdso_encode_cpu_node(cpu, 
early_cpu_to_node(cpu));
+   struct desc_struct d = { };
+
+   if (static_cpu_has(X86_FEATURE_RDTSCP))
+   write_rdtscp_aux(cpudata);
+
+   /* Store CPU and node number in limit. */
+   d.limit0 = cpudata;
+   d.limit1 = cpudata >> 16;
+
+   d.type = 5; /* RO data, expand down, accessed */
+   d.dpl = 3;  /* Visible to user code */
+   d.s = 1;/* Not a system segment */
+   d.p = 1;/* Present */
+   d.d = 1;/* 32-bit */
+
+   write_gdt_entry(get_cpu_gdt_rw(cpu), GDT_ENTRY_CPU_NUMBER, &d, 
DESCTYPE_S);
+}
+#endif
+
 /*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
@@ -1697,6 +1720,7 @@ void cpu_init(void)
early_cpu_to_node(cpu) != NUMA_NO_NODE)
set_numa_node(early_cpu_to_node(cpu));
 #endif
+   setup_getcpu(cpu);
 
me = current;
 
-- 
2.7.4