Re: [tip: x86/entry] x86/entry: Avoid redundant CR3 write on paranoid returns

[Brendan Jackman]

[Apologies if you see this as a duplicate, accidentally sent the
original in HTML, please disregard the other one]

Hi Thomas,

I have just noticed that the commit has disappeared from
tip/x86/entry. Is that deliberate?

Thanks,
Brendan


On Wed, 24 Jan 2024 at 19:36, tip-bot2 for Lai Jiangshan
<tip-bot2@xxxxxxxxxxxxx> wrote:
>
> The following commit has been merged into the x86/entry branch of tip:
>
> Commit-ID:     bb998361999e79bc87dae1ebe0f5bf317f632585
> Gitweb:        https://git.kernel.org/tip/bb998361999e79bc87dae1ebe0f5bf317f632585
> Author:        Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> AuthorDate:    Mon, 08 Jan 2024 11:39:50
> Committer:     Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CommitterDate: Wed, 24 Jan 2024 13:57:59 +01:00
>
> x86/entry: Avoid redundant CR3 write on paranoid returns
>
> The CR3 restore happens in:
>
>   1. #NMI return.
>   2. paranoid_exit() (i.e. #MCE, #VC, #DB and #DF return)
>
> Contrary to the implication in commit 21e94459110252 ("x86/mm: Optimize
> RESTORE_CR3"), the kernel never modifies CR3 in any of these exceptions,
> except for switching from user to kernel pagetables under PTI. That
> means that most of the time when returning from an exception that
> interrupted the kernel no CR3 restore is necessary. Writing CR3 is
> expensive on some machines.
>
> Most of the time because the interrupt might have come during kernel entry
> before the user to kernel CR3 switch or the during exit after the kernel to
> user switch. In the former case skipping the restore would be correct, but
> definitely not for the latter.
>
> So check the saved CR3 value and restore it only, if it is a user CR3.
>
> Give the macro a new name to clarify its usage, and remove a comment that
> was describing the original behaviour along with the not longer needed jump
> label.
>
> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20240108113950.360438-1-jackmanb@xxxxxxxxxx
>
> [Rewrote commit message; responded to review comments]
> Change-Id: I6e56978c4753fb943a7897ff101f519514fa0827
> ---
>  arch/x86/entry/calling.h  | 26 ++++++++++----------------
>  arch/x86/entry/entry_64.S |  7 +++----
>  2 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 9f1d947..92dca4a 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -239,17 +239,19 @@ For 32-bit we have the following conventions - kernel is built with
>  .Ldone_\@:
>  .endm
>
> -.macro RESTORE_CR3 scratch_reg:req save_reg:req
> +/* Restore CR3 from a kernel context. May restore a user CR3 value. */
> +.macro PARANOID_RESTORE_CR3 scratch_reg:req save_reg:req
>         ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
>
> -       ALTERNATIVE "jmp .Lwrcr3_\@", "", X86_FEATURE_PCID
> -
>         /*
> -        * KERNEL pages can always resume with NOFLUSH as we do
> -        * explicit flushes.
> +        * If CR3 contained the kernel page tables at the paranoid exception
> +        * entry, then there is nothing to restore as CR3 is not modified while
> +        * handling the exception.
>          */
>         bt      $PTI_USER_PGTABLE_BIT, \save_reg
> -       jnc     .Lnoflush_\@
> +       jnc     .Lend_\@
> +
> +       ALTERNATIVE "jmp .Lwrcr3_\@", "", X86_FEATURE_PCID
>
>         /*
>          * Check if there's a pending flush for the user ASID we're
> @@ -257,20 +259,12 @@ For 32-bit we have the following conventions - kernel is built with
>          */
>         movq    \save_reg, \scratch_reg
>         andq    $(0x7FF), \scratch_reg
> -       bt      \scratch_reg, THIS_CPU_user_pcid_flush_mask
> -       jnc     .Lnoflush_\@
> -
>         btr     \scratch_reg, THIS_CPU_user_pcid_flush_mask
> -       jmp     .Lwrcr3_\@
> +       jc      .Lwrcr3_\@
>
> -.Lnoflush_\@:
>         SET_NOFLUSH_BIT \save_reg
>
>  .Lwrcr3_\@:
> -       /*
> -        * The CR3 write could be avoided when not changing its value,
> -        * but would require a CR3 read *and* a scratch register.
> -        */
>         movq    \save_reg, %cr3
>  .Lend_\@:
>  .endm
> @@ -285,7 +279,7 @@ For 32-bit we have the following conventions - kernel is built with
>  .endm
>  .macro SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg:req save_reg:req
>  .endm
> -.macro RESTORE_CR3 scratch_reg:req save_reg:req
> +.macro PARANOID_RESTORE_CR3 scratch_reg:req save_reg:req
>  .endm
>
>  #endif
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index c40f89a..aedd169 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -968,14 +968,14 @@ SYM_CODE_START_LOCAL(paranoid_exit)
>         IBRS_EXIT save_reg=%r15
>
>         /*
> -        * The order of operations is important. RESTORE_CR3 requires
> +        * The order of operations is important. PARANOID_RESTORE_CR3 requires
>          * kernel GSBASE.
>          *
>          * NB to anyone to try to optimize this code: this code does
>          * not execute at all for exceptions from user mode. Those
>          * exceptions go through error_return instead.
>          */
> -       RESTORE_CR3     scratch_reg=%rax save_reg=%r14
> +       PARANOID_RESTORE_CR3 scratch_reg=%rax save_reg=%r14
>
>         /* Handle the three GSBASE cases */
>         ALTERNATIVE "jmp .Lparanoid_exit_checkgs", "", X86_FEATURE_FSGSBASE
> @@ -1404,8 +1404,7 @@ end_repeat_nmi:
>         /* Always restore stashed SPEC_CTRL value (see paranoid_entry) */
>         IBRS_EXIT save_reg=%r15
>
> -       /* Always restore stashed CR3 value (see paranoid_entry) */
> -       RESTORE_CR3 scratch_reg=%r15 save_reg=%r14
> +       PARANOID_RESTORE_CR3 scratch_reg=%r15 save_reg=%r14
>
>         /*
>          * The above invocation of paranoid_entry stored the GSBASE