diff mbox series

RISC-V: Add CONFIG_{NON,}PORTABLE

Message ID 20220310170845.17614-1-palmer@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series RISC-V: Add CONFIG_{NON,}PORTABLE | expand

Commit Message

Palmer Dabbelt March 10, 2022, 5:08 p.m. UTC
From: Palmer Dabbelt <palmer@rivosinc.com>

The RISC-V port has collected a handful of options that are
fundamentally non-portable.  To prevent users from shooting themselves
in the foot, hide them all behind a config entry that explicitly calls
out that non-portable binaries may be produced.

Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

---
This came up in the context of the no-M patch:
https://lore.kernel.org/lkml/CAK8P3a3mzax-OiaxBcxM_RgKNsd6N8HW0odRmw38u2jKE5aYaQ@mail.gmail.com/

I'm not sure I strictly need both PORTABLE and NONPORTABLE, but it's the
only way I could come up with to force things like EFI.  I'll poke
around Kconfig a bit more, but I figured this is going to lead to a
discussion so it'd be better to just send this crusty version so we at
least have something concrete to talk about.

I've only given this a smoke test (ie, defconfig looks OK).  I'll go
through all the configs if folks think this is the right way to go -- I
figure it's better to have the discussion on a more focused patch than
on that M patch, as this is really an orthogonal issue.

I'm not really sure what the right option is here: I'm not selecting
things like errata and basic drivers, but I could buy the argument that
disabling those results in non-portable systems.  I am selecting EFI,
that might not be strictly required now but it's the direction we're
going so I figure we might as well start now.  I've also hidden 32BIT
behind this, I could see that going either way but my guess is that
users of 32-bit systems won't care about portable binaries.  I'm also
not sure if this should be tied to something like EMBEDDED or EXPERT.

My biggest worry with this is that users might get the feeling that
current kernels will be compatible with new hardware, that's just not
how RISC-V works.  I tried to write the help text indicating that, I'm
not sure I like how it reads so I'll almost certainly take another shot
at it (though suggestions are, of course, welcome).

I'm also a bit worried that vendors might get the feeling we're not
going to support systems that need modifications to these portablity
requirements.  That's also not the case, as there's really no way for
vendors to make sure their systems continue to run portable kernels
aside from just releasing them publicly so we can test them.
---
 arch/riscv/Kconfig | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Arnd Bergmann March 10, 2022, 6:59 p.m. UTC | #1
On Thu, Mar 10, 2022 at 6:08 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> From: Palmer Dabbelt <palmer@rivosinc.com>
>
> The RISC-V port has collected a handful of options that are
> fundamentally non-portable.  To prevent users from shooting themselves
> in the foot, hide them all behind a config entry that explicitly calls
> out that non-portable binaries may be produced.
>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
> This came up in the context of the no-M patch:
> https://lore.kernel.org/lkml/CAK8P3a3mzax-OiaxBcxM_RgKNsd6N8HW0odRmw38u2jKE5aYaQ@mail.gmail.com/
>
> I'm not sure I strictly need both PORTABLE and NONPORTABLE, but it's the
> only way I could come up with to force things like EFI.  I'll poke
> around Kconfig a bit more, but I figured this is going to lead to a
> discussion so it'd be better to just send this crusty version so we at
> least have something concrete to talk about.
>
> I've only given this a smoke test (ie, defconfig looks OK).  I'll go
> through all the configs if folks think this is the right way to go -- I
> figure it's better to have the discussion on a more focused patch than
> on that M patch, as this is really an orthogonal issue.
>
> I'm not really sure what the right option is here: I'm not selecting
> things like errata and basic drivers, but I could buy the argument that
> disabling those results in non-portable systems.  I am selecting EFI,
> that might not be strictly required now but it's the direction we're
> going so I figure we might as well start now.  I've also hidden 32BIT
> behind this, I could see that going either way but my guess is that
> users of 32-bit systems won't care about portable binaries.

These are all things I would have suggested as well, sounds good.

>  I'm also
> not sure if this should be tied to something like EMBEDDED or EXPERT.

CONFIG_EMBEDDED is weird, it's better to avoid interacting with
it, because it tends to not do what one expects. Hiding it behind
CONFIG_EXPERT would make sense, but it's also a rather strong
guard, so it's probably better to leave it the way you have it.

> My biggest worry with this is that users might get the feeling that
> current kernels will be compatible with new hardware, that's just not
> how RISC-V works.  I tried to write the help text indicating that, I'm
> not sure I like how it reads so I'll almost certainly take another shot
> at it (though suggestions are, of course, welcome).
>
> I'm also a bit worried that vendors might get the feeling we're not
> going to support systems that need modifications to these portablity
> requirements.  That's also not the case, as there's really no way for
> vendors to make sure their systems continue to run portable kernels
> aside from just releasing them publicly so we can test them.
> ---
>  arch/riscv/Kconfig | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5adcbd9b5e88..de0916d7aca7 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -9,6 +9,7 @@ config 64BIT
>
>  config 32BIT
>         bool
> +       depends on NONPORTABLE

This 'depends on is in the wrong place, you need to move it below
ARCH_RV32I, or possibly rework that "Base ISA" choice statement
into something else.

>  config RISCV
>         def_bool y
> @@ -485,6 +486,7 @@ config STACKPROTECTOR_PER_TASK
>
>  config PHYS_RAM_BASE_FIXED
>         bool "Explicitly specified physical RAM address"
> +       depends on NONPORTABLE
>         default n
>
>  config PHYS_RAM_BASE
> @@ -498,7 +500,7 @@ config PHYS_RAM_BASE
>
>  config XIP_KERNEL
>         bool "Kernel Execute-In-Place from ROM"
> -       depends on MMU && SPARSEMEM
> +       depends on MMU && SPARSEMEM && NONPORTABLE
>         # This prevents XIP from being enabled by all{yes,mod}config, which
>         # fail to build since XIP doesn't support large kernels.
>         depends on !COMPILE_TEST
> @@ -538,9 +540,31 @@ endmenu
>
>  config BUILTIN_DTB
>         bool
> -       depends on OF
> +       depends on OF && NONPORTABLE
>         default y if XIP_KERNEL
>
> +config NONPORTABLE
> +       bool "Allow configurations that result in non-portable kernels"
> +       help
> +         RISC-V kernel binaries are compatibile between all known systems

typo: compatible

> +         whenever possible, but there are some use cases that can only be
> +         satisfied by configurations that result in kernel binaries that are
> +         not portable between systems.
> +
> +         Selecting N does not guarntee kernels will be portable to all knows

typo: guarantee

> +         systems.  Selecting any of the options guarded by NONPORTABLE will
> +         result in kernel binaries that are unlikely to be portable between
> +         systems.
> +
> +         If unsure, say N.
> +
> +config PORTABLE
> +       bool
> +       default !NONPORTABLE
> +       select EFI
> +       select OF
> +       select MMU

A nice trick I would use here is to make PORTABLE/NONPORTABLE
into a 'choice' statement that defaults to PORTABLE. That way, both
allnoconfig and allmodconfig/allyesconfig end up testing the portable case.

Ideally both allnoconfig and allmodconfig would be able to boot the
same (virtual) machine, but getting to that point likely requires addressing
further issues.

If someone can come up with a better naming system, the
portable/nonportable choice could even be integrated into the
"Base ISA" choice, giving the user a list of the possible targets,
like:

choice
      prompt "System type"
      default ARCH_RV64I

config ARCH_RV64I
       bool "Generic portable RV64GC system"
       select 64BIT
       select EFI
       select OF
       select MMU

config ARCH_RV64I_NONPORTABLE
        bool "Custom RV64I machine, nonportable"
        select 64BIT
        select MMU

config ARCH_RV32I_NONPORTABLE
        bool "Custom RV64I machine, nonportable"
        select 64BIT
        select MMU

config ARCH_RV64_NOMMU
        bool "Custom RV64I machine without MMU"
        select 64BIT

endchoice

Not sure if that is more or less confusing than what you have
here, just putting it out there as another way to handle the
top-level platform selection.

         Arnd
Palmer Dabbelt April 14, 2022, 1:40 a.m. UTC | #2
On Thu, 10 Mar 2022 10:59:42 PST (-0800), Arnd Bergmann wrote:
> On Thu, Mar 10, 2022 at 6:08 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> From: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> The RISC-V port has collected a handful of options that are
>> fundamentally non-portable.  To prevent users from shooting themselves
>> in the foot, hide them all behind a config entry that explicitly calls
>> out that non-portable binaries may be produced.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>> This came up in the context of the no-M patch:
>> https://lore.kernel.org/lkml/CAK8P3a3mzax-OiaxBcxM_RgKNsd6N8HW0odRmw38u2jKE5aYaQ@mail.gmail.com/
>>
>> I'm not sure I strictly need both PORTABLE and NONPORTABLE, but it's the
>> only way I could come up with to force things like EFI.  I'll poke
>> around Kconfig a bit more, but I figured this is going to lead to a
>> discussion so it'd be better to just send this crusty version so we at
>> least have something concrete to talk about.
>>
>> I've only given this a smoke test (ie, defconfig looks OK).  I'll go
>> through all the configs if folks think this is the right way to go -- I
>> figure it's better to have the discussion on a more focused patch than
>> on that M patch, as this is really an orthogonal issue.
>>
>> I'm not really sure what the right option is here: I'm not selecting
>> things like errata and basic drivers, but I could buy the argument that
>> disabling those results in non-portable systems.  I am selecting EFI,
>> that might not be strictly required now but it's the direction we're
>> going so I figure we might as well start now.  I've also hidden 32BIT
>> behind this, I could see that going either way but my guess is that
>> users of 32-bit systems won't care about portable binaries.
>
> These are all things I would have suggested as well, sounds good.
>
>>  I'm also
>> not sure if this should be tied to something like EMBEDDED or EXPERT.
>
> CONFIG_EMBEDDED is weird, it's better to avoid interacting with
> it, because it tends to not do what one expects. Hiding it behind
> CONFIG_EXPERT would make sense, but it's also a rather strong
> guard, so it's probably better to leave it the way you have it.
>
>> My biggest worry with this is that users might get the feeling that
>> current kernels will be compatible with new hardware, that's just not
>> how RISC-V works.  I tried to write the help text indicating that, I'm
>> not sure I like how it reads so I'll almost certainly take another shot
>> at it (though suggestions are, of course, welcome).
>>
>> I'm also a bit worried that vendors might get the feeling we're not
>> going to support systems that need modifications to these portablity
>> requirements.  That's also not the case, as there's really no way for
>> vendors to make sure their systems continue to run portable kernels
>> aside from just releasing them publicly so we can test them.
>> ---
>>  arch/riscv/Kconfig | 28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 5adcbd9b5e88..de0916d7aca7 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -9,6 +9,7 @@ config 64BIT
>>
>>  config 32BIT
>>         bool
>> +       depends on NONPORTABLE
>
> This 'depends on is in the wrong place, you need to move it below
> ARCH_RV32I, or possibly rework that "Base ISA" choice statement
> into something else.
>
>>  config RISCV
>>         def_bool y
>> @@ -485,6 +486,7 @@ config STACKPROTECTOR_PER_TASK
>>
>>  config PHYS_RAM_BASE_FIXED
>>         bool "Explicitly specified physical RAM address"
>> +       depends on NONPORTABLE
>>         default n
>>
>>  config PHYS_RAM_BASE
>> @@ -498,7 +500,7 @@ config PHYS_RAM_BASE
>>
>>  config XIP_KERNEL
>>         bool "Kernel Execute-In-Place from ROM"
>> -       depends on MMU && SPARSEMEM
>> +       depends on MMU && SPARSEMEM && NONPORTABLE
>>         # This prevents XIP from being enabled by all{yes,mod}config, which
>>         # fail to build since XIP doesn't support large kernels.
>>         depends on !COMPILE_TEST
>> @@ -538,9 +540,31 @@ endmenu
>>
>>  config BUILTIN_DTB
>>         bool
>> -       depends on OF
>> +       depends on OF && NONPORTABLE
>>         default y if XIP_KERNEL
>>
>> +config NONPORTABLE
>> +       bool "Allow configurations that result in non-portable kernels"
>> +       help
>> +         RISC-V kernel binaries are compatibile between all known systems
>
> typo: compatible
>
>> +         whenever possible, but there are some use cases that can only be
>> +         satisfied by configurations that result in kernel binaries that are
>> +         not portable between systems.
>> +
>> +         Selecting N does not guarntee kernels will be portable to all knows
>
> typo: guarantee
>
>> +         systems.  Selecting any of the options guarded by NONPORTABLE will
>> +         result in kernel binaries that are unlikely to be portable between
>> +         systems.
>> +
>> +         If unsure, say N.
>> +
>> +config PORTABLE
>> +       bool
>> +       default !NONPORTABLE
>> +       select EFI
>> +       select OF
>> +       select MMU
>
> A nice trick I would use here is to make PORTABLE/NONPORTABLE
> into a 'choice' statement that defaults to PORTABLE. That way, both
> allnoconfig and allmodconfig/allyesconfig end up testing the portable case.
>
> Ideally both allnoconfig and allmodconfig would be able to boot the
> same (virtual) machine, but getting to that point likely requires addressing
> further issues.
>
> If someone can come up with a better naming system, the
> portable/nonportable choice could even be integrated into the
> "Base ISA" choice, giving the user a list of the possible targets,
> like:

That's a really nice trick to know.  We're a lot farther from this than 
making an allnoconfig build, at least some of that is the Kconfig.socs 
brokenness.  IMO that's sort of just its own problem, and while it 
should get fixed if I try to roll it up into this one we're going to get 
the simple thing stuck on a bigger thing.

> choice
>       prompt "System type"
>       default ARCH_RV64I
>
> config ARCH_RV64I
>        bool "Generic portable RV64GC system"
>        select 64BIT
>        select EFI
>        select OF
>        select MMU
>
> config ARCH_RV64I_NONPORTABLE
>         bool "Custom RV64I machine, nonportable"
>         select 64BIT
>         select MMU
>
> config ARCH_RV32I_NONPORTABLE
>         bool "Custom RV64I machine, nonportable"
>         select 64BIT
>         select MMU
>
> config ARCH_RV64_NOMMU
>         bool "Custom RV64I machine without MMU"
>         select 64BIT
>
> endchoice
>
> Not sure if that is more or less confusing than what you have
> here, just putting it out there as another way to handle the
> top-level platform selection.
>
>          Arnd
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 5adcbd9b5e88..de0916d7aca7 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -9,6 +9,7 @@  config 64BIT
 
 config 32BIT
 	bool
+	depends on NONPORTABLE
 
 config RISCV
 	def_bool y
@@ -485,6 +486,7 @@  config STACKPROTECTOR_PER_TASK
 
 config PHYS_RAM_BASE_FIXED
 	bool "Explicitly specified physical RAM address"
+	depends on NONPORTABLE
 	default n
 
 config PHYS_RAM_BASE
@@ -498,7 +500,7 @@  config PHYS_RAM_BASE
 
 config XIP_KERNEL
 	bool "Kernel Execute-In-Place from ROM"
-	depends on MMU && SPARSEMEM
+	depends on MMU && SPARSEMEM && NONPORTABLE
 	# This prevents XIP from being enabled by all{yes,mod}config, which
 	# fail to build since XIP doesn't support large kernels.
 	depends on !COMPILE_TEST
@@ -538,9 +540,31 @@  endmenu
 
 config BUILTIN_DTB
 	bool
-	depends on OF
+	depends on OF && NONPORTABLE
 	default y if XIP_KERNEL
 
+config NONPORTABLE
+	bool "Allow configurations that result in non-portable kernels"
+	help
+	  RISC-V kernel binaries are compatibile between all known systems
+	  whenever possible, but there are some use cases that can only be
+	  satisfied by configurations that result in kernel binaries that are
+	  not portable between systems.
+
+	  Selecting N does not guarntee kernels will be portable to all knows
+	  systems.  Selecting any of the options guarded by NONPORTABLE will
+	  result in kernel binaries that are unlikely to be portable between
+	  systems.
+
+	  If unsure, say N.
+
+config PORTABLE
+	bool
+	default !NONPORTABLE
+	select EFI
+	select OF
+	select MMU
+
 menu "Power management options"
 
 source "kernel/power/Kconfig"