lkml.org 
[lkml]   [2018]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RESEND PATCH V5 0/8] x86: infrastructure to enable FSGSBASE

* Chang S. Bae <chang.seok.bae@intel.com> wrote:

> Resending the patchset that was posted before [6].
>
> Given feedbacks from [1], it was suggested to separate two parts
> and to (re-)submit this patchset first.
>
> To facilitate FSGSBASE, Andy's FS/GS base read fix is first
> ordered, then some helper functions and refactoring work
> are included. Cleanup for the vDSO initialization is
> for preparing per-CPU base finder that will be used in the
> paranoid entry, when FSGSBASE enabled.
>
> Changes from V4 [5]:
> * Change patch ordering; putting the fix first before introducing
> the helper functions
> * Cleanup further for vDSO CPU initialization codes
>
> Changes from V3 [4]:
> * Unify CPU number initialization
>
> Changes from V2 [3]:
> * Bisect the CPU number initialization patch
> * Drop patches for introducing i386 CPU_NUMBER and switching
> write_rdtscp_aux() to use wrmsr_safe()
>
> Changes from V1 [2]:
> * Rename the x86-64 CPU_NUMBER segment from PER_CPU
> * Add i386 CPU_NUMBER equivalent to x86-64 at GDT entry 23
> * Add additional helper function to store CPU number
> * Switch write_rdtscp_aux() to use wrmsr_safe()
>
> [1] FSGSBASE patch set V2:
> https://lore.kernel.org/patchwork/cover/912063/
> [2] infrastructure for FSGSBASE
> V1: https://lore.kernel.org/patchwork/cover/913139/
> [3] V2: https://lore.kernel.org/patchwork/cover/913644/
> [4] V3: https://lore.kernel.org/patchwork/cover/949775/
> [5] V4: https://lore.kernel.org/patchwork/cover/951712/
> [6] V5: https://lore.kernel.org/patchwork/cover/956526/
>
> Andy Lutomirski (1):
> x86/arch_prctl/64: Make ptrace read FS/GS base accurately
>
> Chang S. Bae (7):
> x86/fsgsbase/64: Introduce FS/GS base helper functions
> x86/fsgsbase/64: Make ptrace use FS/GS base helpers
> x86/fsgsbase/64: Use FS/GS base helpers in core dump
> x86/fsgsbase/64: Factor out load FS/GS segments from __switch_to
> x86/segments/64: Rename PER_CPU segment to CPU_NUMBER
> x86/vdso: Introduce helper functions for CPU and node number
> x86/vdso: Move out the CPU initialization
>
> arch/x86/entry/vdso/vgetcpu.c | 9 +-
> arch/x86/entry/vdso/vma.c | 38 +--------
> arch/x86/include/asm/elf.h | 6 +-
> arch/x86/include/asm/fsgsbase.h | 47 +++++++++++
> arch/x86/include/asm/segment.h | 46 +++++++++-
> arch/x86/include/asm/vgtod.h | 26 ------
> arch/x86/kernel/cpu/common.c | 24 ++++++
> arch/x86/kernel/process_64.c | 183 +++++++++++++++++++++++++++++++---------
> arch/x86/kernel/ptrace.c | 28 ++----
> 9 files changed, 270 insertions(+), 137 deletions(-)
> create mode 100644 arch/x86/include/asm/fsgsbase.h

Looks mostly good, here's a couple of details I noticed before I almost applied the series:


- Please use a single, unified name-space for all the new FS/GS helpers, such as:

x86_fsbase_read_cpu()
x86_fsbase_read_task()
x86_fsbase_write_cpu()
x86_fsbase_write_cpu_inactive()
x86_fsbase_write_task()

x86_gsbase_read_cpu()
x86_gsbase_read_task()
x86_gsbase_write_task()

x86_fsgsbase_load() # was: load_fsgs()
x86_fsgsbase_read_task() # was: task_seg_base()

etc.

- Please port patch #7 to latest -tip, it's conflicting with:

e78e5a91456f: x86/vdso: Fix lsl operand order

The resolution is to preserve the modified code.

Also some style nits:

- Please fix the inconsistent capitalization of 'CPU' in various places, comments,
descriptions, etc.

- Comments should start with a capital letter, like sentences do. Right now there's a mixed
style of:

+ /*
+ * If performance here mattered, we could protect the LDT
+ * with RCU. This is a slow path, though, so we can just
+ * take the mutex.
+ */

+ /*
+ * set the selector to 0 as a notion, that the segment base is
+ * overwritten, which will be checked for skipping the segment load
+ * during context switch.
+ */


- Please refer to functions in changelogs and comments with fn_name(), not just fn_name. For
example:

x86/fsgsbase/64: Factor out load FS/GS segments from __switch_to

But there's more examples.

- Please read your own patches once again and fix typos in comments and changelogs, such as:

+ * correctly wrt barrier() and to keep gcc from cleverly


Thanks,

Ingo

\
 
 \ /
  Last update: 2018-09-14 11:28    [W:1.195 / U:0.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site