Re: [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix

"Aneesh Kumar K.V" [off-list ref] writes:

On 7/6/20 6:11 PM, Michael Ellerman wrote:

quoted

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

The next set of patches adds support for kuap with hash translation.

That's no longer true of this series.

quoted

diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 0d72c0246052..e93b65a0e6e7 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c

@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)
  	return;
  }
  
+#ifdef CONFIG_PPC_KUAP
+void __init setup_kuap(bool disabled)
+{
+	if (disabled || !early_radix_enabled())
+		return;
+
+	if (smp_processor_id() == boot_cpuid) {
+		pr_info("Activating Kernel Userspace Access Prevention\n");
+		cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
+	}
+
+	/* Make sure userspace can't change the AMR */
+	mtspr(SPRN_UAMOR, 0);
+	mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
+	isync();
+}
+#endif

This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
used to.

That risks breaking people's existing .configs, if they have
PPC_MEM_KEYS=n they will now lose KUAP.

And I'm not convinced the two features should be tied together, at least
at the user-visible Kconfig level.

That simplifies the addition of hash kuap a lot. Especially in the 
exception entry and return paths.  I did try to consider them as 
independent options. But then the feature fixup in asm code gets 
unnecessarily complicated. Also the UAMOR handling also get complicated.

Yep. I'm OK if most of the code is enabled for either/both options, but
I think the user-visible options should not depend on each other.

So something like:

config PPC_PKEY
	def_bool y
        depends on PPC_MEM_KEYS || PPC_KUAP

And then the low-level code is guarded by PPC_PKEY.

Or we just say that making MEM_KEYS configurable is not worth the
added complexity and turn it on always.

cheers