Re: [PATCH] x86/mm: Rewrite sme_populate_pgd() in a more sensible way

From: Tom Lendacky
Date: Fri Dec 08 2017 - 09:44:09 EST


On 12/4/2017 12:50 PM, Kirill A. Shutemov wrote:
On Mon, Dec 04, 2017 at 12:33:01PM -0600, Tom Lendacky wrote:
On 12/4/2017 10:34 AM, Kirill A. Shutemov wrote:
On Mon, Dec 04, 2017 at 04:00:26PM +0000, Tom Lendacky wrote:
On 12/4/2017 8:57 AM, Kirill A. Shutemov wrote:
On Mon, Dec 04, 2017 at 08:19:11AM -0600, Tom Lendacky wrote:
On 12/4/2017 5:23 AM, Kirill A. Shutemov wrote:
sme_populate_pgd() open-codes a lot of things that are not needed to be
open-coded.

Let's rewrite it in a more stream-lined way.

This would also buy us boot-time switching between support between
paging modes, when rest of the pieces will be upstream.

Hi Kirill,

Unfortunately, some of these can't be changed. The use of p4d_offset(),
pud_offset(), etc., use non-identity mapped virtual addresses which cause
failures at this point of the boot process.

Wat? Virtual address is virtual address. p?d_offset() doesn't care about
what mapping you're using.

Yes it does. For example, pmd_offset() issues a pud_page_addr() call,
which does a __va() returning a non-identity mapped address (0xffff88...).
Only identity mapped virtual addresses have been setup at this point, so
the use of that virtual address panics the kernel.

Stupid me. You are right.

What about something like this:

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index d9a9e9fc75dd..65e0d68f863f 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -12,6 +12,23 @@
#define DISABLE_BRANCH_PROFILING
+/*
+ * Since we're dealing with identity mappings, physical and virtual
+ * addresses are the same, so override these defines which are ultimately
+ * used by the headers in misc.h.
+ */
+#define __pa(x) ((unsigned long)(x))
+#define __va(x) ((void *)((unsigned long)(x)))

No, you can't do this. There are routines in this file that are called
after the kernel has switched to its standard virtual address map where
this definition of __va() will likely cause a failure.

Let's than split it up into separate compilation unit.

+/*
+ * Special hack: we have to be careful, because no indirections are
+ * allowed here, and paravirt_ops is a kind of one. As it will only run in
+ * baremetal anyway, we just keep it from happening. (This list needs to
+ * be extended when new paravirt and debugging variants are added.)
+ */
+#undef CONFIG_PARAVIRT
+#undef CONFIG_PARAVIRT_SPINLOCKS

I'd really, really like to avoid doing something like this.

Any other proposals?

Current code is way too hairy and hard to modify.

I'd like to hear some other opinions, but if there are none, I don't see
an issue with splitting this up, I guess. I just dislike doing some of
these hacks, but if it makes things cleaner and simpler to understand,
then I guess I can't really object.

Thanks,
Tom