Received-SPF: pass (google.com: domain of linux-kernel+bounces-34725-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240108113950.360438-1-jackmanb@google.com>
In-Reply-To: <20240108113950.360438-1-jackmanb@google.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Mon, 22 Jan 2024 21:33:42 -0800
Message-ID: <CAJD7tkZzYp56d=wms7sgji1A=84Ax2Uo+=xvs7u1PtZ+mLvFhQ@mail.gmail.com>
Subject: Re: [PATCH v3 RESEND] x86/entry: Avoid redundant CR3 write on
 paranoid returns
To: Brendan Jackman <jackmanb@google.com>
Cc: luto@kernel.org, tglx@linutronix.de, peterz@infradead.org, 
	dave.hansen@linux.intel.com, mingo@redhat.com, bp@alien8.de, x86@kernel.org, 
	hpa@zytor.com, linux-kernel@vger.kernel.org, laijs@linux.alibaba.com, 
	reijiw@google.com, oweisse@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 8, 2024 at 3:39=E2=80=AFAM Brendan Jackman <jackmanb@google.com=
> wrote:
>
> From: Lai Jiangshan <laijs@linux.alibaba.com>
>
> This path gets used called from:
>
> 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, so this commit avoids redundant writes.
>
> I said "most of the time" because the interrupt might have come during
> kernel entry before the user->kernel CR3 switch or the during exit after
> the kernel->user switch. In the former case skipping the restore might
> actually be be fine, but definitely not the latter. So we do still need
> to check the saved CR3 and restore it if it's a user CR3.
>
> Note this code is ONLY used for returning _to kernel code_. So the only
> times where the CR3 write is necessary are in those rather special cases
> mentioned above where we are in kernel _code_ but a userspace CR3.
>
> While changing this logic the macro is given a new name to clarify its
> usage, and a comment that was describing its behaviour at the call site
> is removed.  We can also simplify the code around the SET_NOFLUSH_BIT
> invocation as we no longer need to branch to it from above.
>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> [Rewrote commit message; responded to review comments]
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> Change-Id: I6e56978c4753fb943a7897ff101f519514fa0827

The Change-Id line here needs to be deleted. Otherwise, it seems like
this patch keeps falling through the cracks :)

Is there anything that needs to be done here?

> ---
>
> Notes:
>     v1: https://lore.kernel.org/lkml/20230817121513.1382800-1-jackmanb@go=
ogle.com/
>
>     v1->v2: Rewrote some comments, added a proper commit message, cleaned=
 up
>         the code per tglx's suggestion.
>
>         I've kept Lai as the Author. If you prefer for the blame to
>         record the last person that touched it then that's also fine
>         though, I can credit Lai as Co-developed-by.
>
>     v2: https://lore.kernel.org/lkml/20230920150443.1789000-1-jackmanb@go=
ogle.com/
>
>     v2->v3: Clarified the commit message per Dave's suggestion and rename=
d the
>         macro. I did not carry PeterZ's ack since I have made some change=
s.
>
>     original v3 (no responses):
>         https://lore.kernel.org/lkml/20231108171656.3444702-1-jackmanb@go=
ogle.com/