Re: [GIT PULL] Protection Keys (pkeys) support

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> So I finally got around to this one and the objtool pull request, and
> note that there's a conflict in the arch/x86/Kconfig file.
> 
> And I'm not sending this email because the conflict would have been
> hard to resolve - it was completely trivial. But the conflict does
> show that once again people are starting to add the new options to the
> end of the list, even though that list is supposedly sorted.
> 
> HOWEVER.
> 
> I didn't actually fix that up in the merge, because I think that those
> options should be done differently anyway.
> 
> So all of these are under the "X86" config options as "select"
> statements that are true for x86. However, all the new ones (and an
> alarming number of old ones) aren't actually really "these are true
> for x86". No, they are *conditionally* true for x86.
> 
> For example, if we were to sort those thing, the two PKEY-related
> options would have to be split up:
> 
>         select ARCH_USES_HIGH_VMA_FLAGS         if
> X86_INTEL_MEMORY_PROTECTION_KEYS
>         select ARCH_HAS_PKEYS                   if
> X86_INTEL_MEMORY_PROTECTION_KEYS
> 
> which would actually make it really nasty to see that they are related.
> 
> There's also a *lot* of those X86 selects that are "if X86_64". So
> they really aren't x86 options, they are x86-64 options.
> 
> So instead of having a _huge_ list of select statements under the X86
> option, why aren't those split up, and the select statements are
> closer to the thing that actually controls them.
> 
> I realize that for many *common* options that really are "this
> architecture uses the generic XYZ feature", the current "select" model
> is really good. But it's starting to look really quite nasty for some
> of these more specialized options, and I really think it would be
> better to move (for example) the select for ARCH_HAS_PKEYS and
> ARCH_USES_HIGH_VMA_FLAGS to actually be under the
> X86_INTEL_MEMORY_PROTECTION_KEYS config option, rather than try to lie
> and make it look like this is somehow some "x86 feature". It's much
> more specific than that.

Agreed absolutely - I didn't notice that these two new options were misplaced.

That long list under the x86 option is supposed to be there only for 'pure' 
options that only depend on the bitness. The single unified 'table' of options 
replaces former hard to read/maintain duplicates like:

 config X86_32
	...
	select HAVE_CMPXCHG_LOCAL
	...

 [a couple of pages further down]

 config X86_64
	...
	select HAVE_CMPXCHG_LOCAL
	...

Also, the 'if X86_64' part is supposed to be for features where the dependency is 
a 'maintainer decision/option', not an 'architecture necessity'.

So for example:

        select ARCH_HAS_PMEM_API                if X86_64
        select HAVE_BPF_JIT                     if X86_64
        select HAVE_CONTEXT_TRACKING            if X86_64

are 'optional features' that are 64-bit because they are not implemented for 
x86-32 yet.

while bits like:

        select CLONE_BACKWARDS                  if X86_32
        select ARCH_WANT_IPC_PARSE_VERSION      if X86_32
        select ARCH_SUPPORTS_INT128             if X86_64

are more 'fundamental' architecture properties that probably won't change in the 
future.

The list was also supposed to be sorted by name originally, to make the likelihood 
of collisions/conflicts lower...

All of which rules I broke when I applied the pkeys bits :-/

> Anyway, it's all merged in my tree, but is going through the built tests and 
> I'll do a boot test too before pushing out. So no need to do anything wrt these 
> pull requests, this was more of a "Hmm, I really think the x86 Kconfig file is 
> getting pretty nasty".

Yeah, so I think the way I'd solve the pkeys problem is the patch attached below. 
Eventually, if 'protection keys' becomes a generic option configured in mm/Kconfig 
or so, then we could turn this into a pure architecture select - but not now.

Furthermore, I'd reorganize the 'arch settings' section into 4 groups, the 
following way:

1)

#
# Options that are fundamentally only set on x86-32 kernels:
#
config X86_32
	select CLONE_BACKWARDS
	select ARCH_WANT_IPC_PARSE_VERSION
	...

2)

#
# Options that are fundamentally only set on x86-64 kernels:
#
config X86_64
	select ARCH_SUPPORTS_INT128
	...

3)

#
# Bi-arch options and options that are not yet implemented for both
# x86 bit widths, sorted alphabetically:
#
config X86
	select ANON_INODES
	...
        select ARCH_HAS_PMEM_API                if X86_64
        select ARCH_SUPPORTS_NUMA_BALANCING     if X86_64
        select ARCH_USE_BUILTIN_BSWAP

all other options are moved out.

Furthermore, some options have unnecessary dependencies, such as:

        select ACPI_LEGACY_TABLES_LOOKUP        if ACPI
        select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
        select ARCH_MIGHT_HAVE_ACPI_PDC         if ACPI

these could lose the 'if ACPI' part - those options don't make a difference when 
ACPI is disabled.

4)

All other options, such as:

        select COMPAT_OLD_SIGACTION             if IA32_EMULATION
        select X86_DEV_DMA_OPS                  if X86_64
        select X86_FEATURE_NAMES                if PROC_FS

are moved as selects to their respective config entries.

In the end for 'config X86' we'd have a clean list of options that are only 
restricted to any of the bitness variants if that restriction is 'optional', i.e. 
we might reasonably expect the other bitness to be supported in the feature.

Do you agree with such an approach?

Thanks,

	Ingo

=================>
 arch/x86/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 8b680a5cb25b..ada9c64cabca 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -156,8 +156,6 @@ config X86
 	select X86_DEV_DMA_OPS			if X86_64
 	select X86_FEATURE_NAMES		if PROC_FS
 	select HAVE_STACK_VALIDATION		if X86_64
-	select ARCH_USES_HIGH_VMA_FLAGS		if X86_INTEL_MEMORY_PROTECTION_KEYS
-	select ARCH_HAS_PKEYS			if X86_INTEL_MEMORY_PROTECTION_KEYS
 
 config INSTRUCTION_DECODER
 	def_bool y
@@ -1726,6 +1724,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
 	def_bool y
 	# Note: only available in 64-bit mode
 	depends on CPU_SUP_INTEL && X86_64
+	select ARCH_USES_HIGH_VMA_FLAGS
+	select ARCH_HAS_PKEYS
 	---help---
 	  Memory Protection Keys provides a mechanism for enforcing
 	  page-based protections, but without requiring modification of the