Re: [PATCH v2 1/4] x86: provide an inverted Kconfig control for shim-exclusive mode

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Mar 2023, Jan Beulich wrote:
> While we want certain things turned off in shim-exclusive mode, doing
> so via "depends on !PV_SHIM_EXCLUSIVE" badly affects allyesconfig: Since
> that will turn on PV_SHIM_EXCLUSIVE, other options will be turned off as
> a result. Yet allyesconfig wants to enable as much of the functionality
> as possible.
> 
> Retain PV_SHIM_EXCLUSIVE as a prompt-less option such that first of all
> C code using it can remain as is. This isn't just for less code churn,
> but also because I think that symbol is more logical to use in many
> (all?) places.
> 
> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> ---
> The new Kconfig control's name is up for improvement suggestions, but I
> think it's already better than the originally thought of
> FULL_HYPERVISOR.

I think the approach in general is OK, maybe we can improve the naming
further. What about one of the following?

NO_PV_SHIM_EXCLUSIVE
PV_SHIM_NOT_EXCLUSIVE
ADD_PV_SHIM
PV_SHIM_AND_HYPERVISOR

This is because I think the option should be tied to PV_SHIM. Keep in
mind that users are supposed to be able to use "make menuconfig" and
pick good options based on the menu. An option called UNCONSTRAINED or
FULL_HYPERVISOR or any other name that has nothing to do with PV_SHIM is
very confusing to me.


> Secondary Kconfig changes could be omitted; in all of the cases I wasn't
> really sure whether do the adjustments. I think to avoid setting a bad
> precedent we want to avoid "depends on !..." (and hence also the
> functionally equivalent "if !..."), but any default settings or prompt
> controls could also be left as they were (or could be done the other way
> around in subsequent patches).
> 
> The Requested-by: isn't for what exactly is done here, but for the
> underlying principle of avoiding the negative dependencies we've grown.
> 
> Outside of Arm-specific code we have two more negative "depends on":
> COVERAGE requires !LIVEPATCH and SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> requires !ENFORCE_UNIQUE_SYMBOLS. The latter could apparently be
> switched to a choice (enforce, warn, don't warn), but then I'm not sure
> how well choices play with allyesconfig (I guess the default setting is
> used).
> ---
> v2: New.
> 
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -103,7 +103,7 @@ config PV_LINEAR_PT
>  
>  config HVM
>       bool "HVM support"
> -     depends on !PV_SHIM_EXCLUSIVE
> +     depends on UNCONSTRAINED
>       default !PV_SHIM
>       select COMPAT
>       select IOREQ_SERVER
> @@ -145,7 +145,7 @@ config XEN_IBT
>  
>  config SHADOW_PAGING
>       bool "Shadow Paging"
> -     default !PV_SHIM_EXCLUSIVE
> +     default UNCONSTRAINED
>       depends on PV || HVM
>       ---help---
>  
> @@ -196,7 +196,7 @@ config HVM_FEP
>  config TBOOT
>       bool "Xen tboot support (UNSUPPORTED)"
>       depends on UNSUPPORTED
> -     default !PV_SHIM_EXCLUSIVE
> +     default UNCONSTRAINED
>       select CRYPTO
>       ---help---
>         Allows support for Trusted Boot using the Intel(R) Trusted Execution
> @@ -276,17 +276,19 @@ config PV_SHIM
>  
>         If unsure, say Y.
>  
> -config PV_SHIM_EXCLUSIVE
> -     bool "PV Shim Exclusive"
> -     depends on PV_SHIM
> -     ---help---
> -       Build Xen in a way which unconditionally assumes PV_SHIM mode.  This
> -       option is only intended for use when building a dedicated PV Shim
> -       firmware, and will not function correctly in other scenarios.
> +config UNCONSTRAINED
> +     bool "do NOT build a functionality restricted hypervisor" if PV_SHIM
> +     default y
> +     help
> +       Do NOT build Xen in a way which unconditionally assumes PV_SHIM mode.
>  
> -       If unsure, say N.
> +       If unsure, say Y.

Yes, the option should not be visible and default y unless PV_SHIM is
selected, like you did.

I think this patch is fine overall, I only suggest a renaming of
UNCONSTRAINED to something to ties it to PV_SHIM.


> +config PV_SHIM_EXCLUSIVE
> +     def_bool y
> +     depends on !UNCONSTRAINED
>  
> -if !PV_SHIM_EXCLUSIVE
> +if UNCONSTRAINED
>  
>  config HYPERV_GUEST
>       bool "Hyper-V Guest"
> --- a/xen/arch/x86/configs/pvshim_defconfig
> +++ b/xen/arch/x86/configs/pvshim_defconfig
> @@ -3,7 +3,7 @@ CONFIG_PV=y
>  CONFIG_XEN_GUEST=y
>  CONFIG_PVH_GUEST=y
>  CONFIG_PV_SHIM=y
> -CONFIG_PV_SHIM_EXCLUSIVE=y
> +# CONFIG_UNCONSTRAINED is not set
>  CONFIG_NR_CPUS=32
>  CONFIG_EXPERT=y
>  # Disable features not used by the PV shim
> --- a/xen/drivers/video/Kconfig
> +++ b/xen/drivers/video/Kconfig
> @@ -3,10 +3,10 @@ config VIDEO
>       bool
>  
>  config VGA
> -     bool "VGA support" if !PV_SHIM_EXCLUSIVE
> +     bool "VGA support" if UNCONSTRAINED
>       select VIDEO
>       depends on X86
> -     default y if !PV_SHIM_EXCLUSIVE
> +     default y if UNCONSTRAINED
>       ---help---
>         Enable VGA output for the Xen hypervisor.
>  
> 
>