[PATCH v2 0/4] x86/pv-shim: configuring / building re-arrangements

[PATCH v2 0/4] x86/pv-shim: configuring / building re-arrangements

[Jan Beulich]

The 2nd patch, while originally having been the main piece here, and while
fixing a bug kind of as a side effect, is partly RFC; see there. The last
two changes are really minor adjustments for aspects noticed while putting
together the main change.

1: provide an inverted Kconfig control for shim-exclusive mode
2: common: reduce PV shim footprint
2: don't even allow enabling GRANT_TABLE
3: suppress core-parking logic

The main change in v2 is the addition of patch 1, as requested by Andrew.

Jan

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

[Jan Beulich]

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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.

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.
+
+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.
 

[PATCH v2 2/4] common: reduce PV shim footprint

[Jan Beulich]

Having CONFIG_PV_SHIM conditionals in common code isn't really nice.
Utilize that we're no longer invoking hypercall handlers via indirect
calls through a table of function vectors. With the use of direct calls
from the macros defined by hypercall-defs.h, we can simply define
overriding macros for event channel and grant table ops handling. All
this requires is arrangement for careful double inclusion of
asm/hypercall.h out of xen/hypercall.h. Such double inclusion is
required because hypercall-defs.h expects certain definitions to be in
place, while the new handling (placed in pv/shim.h, which is now
included from asm/hypercall.h despite the apparent cyclic dependency)
requires prototypes from hypercall-defs.h to be available already.

Note that this makes it necessary to further constrain the stubbing of
pv_shim from common/sched/core.c, and allows removing the inclusion of
asm/guest.h there as well. Since this is actually part of the overall
goal, leverage the mechanism to also get rid of the similar construct in
xsm/flask/hooks.c, including xen/hypercall.h instead.

Note further that kind of as a side effect this fixes grant table
handling for 32-bit shim guests when GRANT_TABLE=y, as the non-stub
compat_grant_table_op() did not redirect to pv_shim_grant_table_op().

A downside of this is that now do_{event_channel,grant_table}_op() are
built in full again when PV_SHIM_EXCLUSIVE=y, despite all the code
actually being dead in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Sadly I had to restore the two "#define pv_shim false", for Arm to
     continue to build. Originally I was hoping to get rid of that
     #ifdef-ary altogether. Would it be acceptable to put a single,
     central #define in e.g. xen/sched.h or xen/hypercall.h?

--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -6,14 +6,23 @@
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
 
-#ifndef __ASM_X86_HYPERCALL_H__
-#define __ASM_X86_HYPERCALL_H__
-
 #include <xen/types.h>
+#include <public/xen.h>
 #include <public/physdev.h>
-#include <public/event_channel.h>
 #include <public/arch-x86/xen-mca.h> /* for do_mca */
+
+#ifdef CONFIG_COMPAT
+#include <compat/arch-x86/xen.h>
+#include <compat/physdev.h>
+#include <compat/platform.h>
+#endif
+
+#if !defined(__ASM_X86_HYPERCALL_H__) && \
+    (!defined(CONFIG_PV_SHIM) || defined(hypercall_args_pv64))
+#define __ASM_X86_HYPERCALL_H__
+
 #include <asm/paging.h>
+#include <asm/pv/shim.h>
 
 #define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1
 
@@ -33,10 +42,6 @@ void pv_ring3_init_hypercall_page(void *
 
 #ifdef CONFIG_COMPAT
 
-#include <compat/arch-x86/xen.h>
-#include <compat/physdev.h>
-#include <compat/platform.h>
-
 extern int
 compat_common_vcpu_op(
     int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
--- a/xen/arch/x86/include/asm/pv/shim.h
+++ b/xen/arch/x86/include/asm/pv/shim.h
@@ -49,6 +49,22 @@ const struct platform_bad_page *pv_shim_
 typeof(do_event_channel_op) pv_shim_event_channel_op;
 typeof(do_grant_table_op) pv_shim_grant_table_op;
 
+#ifdef CONFIG_PV_SHIM_EXCLUSIVE
+#define REVECTOR(pfx, op, args...) pv_shim_ ## op(args)
+#else
+#define REVECTOR(pfx, op, args...) ({ \
+    likely(!pv_shim) \
+    ? pfx ## _ ## op(args) \
+    : pv_shim_ ## op(args); \
+})
+#endif
+
+#define do_event_channel_op(args...) REVECTOR(do, event_channel_op, args)
+#define do_grant_table_op(args...) REVECTOR(do, grant_table_op, args)
+#ifdef CONFIG_COMPAT
+#define compat_grant_table_op(args...) REVECTOR(compat, grant_table_op, args)
+#endif
+
 #else
 
 static inline void pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -822,9 +822,9 @@ long pv_shim_grant_table_op(unsigned int
     return rc;
 }
 
-#ifndef CONFIG_GRANT_TABLE
+#if !defined(CONFIG_GRANT_TABLE) && !defined(CONFIG_PV_SHIM_EXCLUSIVE)
 /* Thin wrapper(s) needed. */
-long do_grant_table_op(
+long (do_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     if ( !pv_shim )
@@ -834,7 +834,7 @@ long do_grant_table_op(
 }
 
 #ifdef CONFIG_PV32
-int compat_grant_table_op(
+int (compat_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     if ( !pv_shim )
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -56,7 +56,7 @@ CHECK_gnttab_swap_grant_ref;
 CHECK_gnttab_cache_flush;
 #undef xen_gnttab_cache_flush
 
-int compat_grant_table_op(
+int (compat_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) cmp_uop, unsigned int count)
 {
     int rc = 0;
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -32,10 +32,6 @@
 #include <public/event_channel.h>
 #include <xsm/xsm.h>
 
-#ifdef CONFIG_PV_SHIM
-#include <asm/guest.h>
-#endif
-
 #define ERROR_EXIT(_errno)                                          \
     do {                                                            \
         gdprintk(XENLOG_WARNING,                                    \
@@ -1222,15 +1218,10 @@ static int evtchn_set_priority(const str
     return ret;
 }
 
-long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long (do_event_channel_op)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int rc;
 
-#ifdef CONFIG_PV_SHIM
-    if ( unlikely(pv_shim) )
-        return pv_shim_event_channel_op(cmd, arg);
-#endif
-
     switch ( cmd )
     {
     case EVTCHNOP_alloc_unbound: {
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -45,10 +45,6 @@
 #include <asm/flushtlb.h>
 #include <asm/guest_atomics.h>
 
-#ifdef CONFIG_PV_SHIM
-#include <asm/guest.h>
-#endif
-
 /* Per-domain grant information. */
 struct grant_table {
     /*
@@ -3563,17 +3559,12 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
     return 0;
 }
 
-long do_grant_table_op(
+long (do_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     long rc;
     unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0;
 
-#ifdef CONFIG_PV_SHIM
-    if ( unlikely(pv_shim) )
-        return pv_shim_grant_table_op(cmd, uop, count);
-#endif
-
     if ( (int)count < 0 )
         return -EINVAL;
 
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -40,9 +40,7 @@
 
 #include "private.h"
 
-#ifdef CONFIG_XEN_GUEST
-#include <asm/guest.h>
-#else
+#ifndef CONFIG_X86
 #define pv_shim false
 #endif
 
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -24,6 +24,9 @@
 /* Needs to be after asm/hypercall.h. */
 #include <xen/hypercall-defs.h>
 
+/* Include a 2nd time, for x86'es PV shim. */
+#include <asm/hypercall.h>
+
 extern long
 arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -19,6 +19,7 @@
 #include <xen/cpumask.h>
 #include <xen/errno.h>
 #include <xen/guest_access.h>
+#include <xen/hypercall.h>
 #include <xen/xenoprof.h>
 #include <xen/iommu.h>
 #ifdef CONFIG_HAS_PCI_MSI
@@ -38,9 +39,7 @@
 #include <conditional.h>
 #include "private.h"
 
-#ifdef CONFIG_X86
-#include <asm/pv/shim.h>
-#else
+#ifndef CONFIG_X86
 #define pv_shim false
 #endif
 

[PATCH v2 3/4] x86/pv-shim: don't even allow enabling GRANT_TABLE

[Jan Beulich]

Grant table code is unused in shim mode, so there's no point in
building it in the first place for shim-exclusive mode.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use UNCONSTRAINED.

--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -9,7 +9,6 @@ CONFIG_EXPERT=y
 # Disable features not used by the PV shim
 # CONFIG_XEN_SHSTK is not set
 # CONFIG_XEN_IBT is not set
-# CONFIG_GRANT_TABLE is not set
 # CONFIG_HYPFS is not set
 # CONFIG_BIGMEM is not set
 # CONFIG_KEXEC is not set
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -15,6 +15,7 @@ config CORE_PARKING
 config GRANT_TABLE
 	bool "Grant table support" if EXPERT
 	default y
+	depends on UNCONSTRAINED
 	---help---
 	  Grant table provides a generic mechanism to memory sharing
 	  between domains. This shared memory interface underpins the

[PATCH v2 4/4] x86/pv-shim: suppress core-parking logic

[Jan Beulich]

This is all dead code in shim-exclusive mode, so there's no point in
building it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Depends on "core-parking: fix build with gcc12 and NR_CPUS=1".
---
v2: Use UNCONSTRAINED.

--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -10,7 +10,7 @@ config COMPAT
 
 config CORE_PARKING
 	bool
-	depends on NR_CPUS > 1
+	depends on NR_CPUS > 1 && UNCONSTRAINED
 
 config GRANT_TABLE
 	bool "Grant table support" if EXPERT

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

[Stefano Stabellini]

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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> ---
> 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.
>  
> 
> 

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

[Jan Beulich]

On 17.01.2025 01:31, Stefano Stabellini wrote:
> 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@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>> 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.

Hmm. That was actually something I was specifically trying to avoid. Imo
the connection to the shim only wants making in the help text. And I fear
I view all your naming suggestions as hard to grok.

Jan

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

[Roger Pau Monné]

On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> 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@citrix.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > ---
> > 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

IMO negated options are confusing, and if possible I think we should
avoid using them unless strictly necessary.

I for example always considered extremely confusing that previous to
having CONFIG_DEBUG Xen used NDEBUG (so no debug) as a way to signal
debug vs non-debug builds.

Thanks, Roger.

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

[Alejandro Vallejo]

On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> > 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@citrix.com>
> > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >
> > > ---
> > > 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
>
> IMO negated options are confusing, and if possible I think we should
> avoid using them unless strictly necessary.

The problem is that the option is negative in nature. It's asking for
hypervisor without _something_. I do agree with Stefano that shim would be
better in the name. Otherwise readers are forced to play divination tricks.

How about something like:

    SHIMLESS_HYPERVISOR

That's arguably not negated, preserves "shim" in the name and has the correct
polarity for allyesconfig to yield the right thing.

>
> I for example always considered extremely confusing that previous to
> having CONFIG_DEBUG Xen used NDEBUG (so no debug) as a way to signal
> debug vs non-debug builds.
>
> Thanks, Roger.

Cheers,
Alejandro

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

[Jan Beulich]

On 17.01.2025 13:24, Alejandro Vallejo wrote:
> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
>>> 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@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> ---
>>>> 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
>>
>> IMO negated options are confusing, and if possible I think we should
>> avoid using them unless strictly necessary.
> 
> The problem is that the option is negative in nature. It's asking for
> hypervisor without _something_. I do agree with Stefano that shim would be
> better in the name. Otherwise readers are forced to play divination tricks.
> 
> How about something like:
> 
>     SHIMLESS_HYPERVISOR
> 
> That's arguably not negated, preserves "shim" in the name and has the correct
> polarity for allyesconfig to yield the right thing.

Except that a hypervisor with this option enabled isn't shim-less, but permits
working in shim as well as in non-shim mode.

Jan

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

On Fri, 17 Jan 2025, Jan Beulich wrote:
> On 17.01.2025 13:24, Alejandro Vallejo wrote:
> > On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
> >> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> >>> 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@citrix.com>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> ---
> >>>> 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
> >>
> >> IMO negated options are confusing, and if possible I think we should
> >> avoid using them unless strictly necessary.
> > 
> > The problem is that the option is negative in nature. It's asking for
> > hypervisor without _something_. I do agree with Stefano that shim would be
> > better in the name. Otherwise readers are forced to play divination tricks.
> > 
> > How about something like:
> > 
> >     SHIMLESS_HYPERVISOR
> > 
> > That's arguably not negated, preserves "shim" in the name and has the correct
> > polarity for allyesconfig to yield the right thing.
> 
> Except that a hypervisor with this option enabled isn't shim-less, but permits
> working in shim as well as in non-shim mode.

First, let's recognize that we have two opposing requirements. One
requirement is to make it as easy as possible to configure for the user.
Ideally without using negative CONFIG options, such as
NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
I think.

On the other hand, we have the requirement that we don't want
allyesconfig to end up disabling a bunch of CONFIG options. Now this
requirement can be satisfied easily by adding something like
NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
requirement.

So we need a compromise, something that doesn't end up disabling other
CONFIG options, to make allyesconfig happy, but also not too confusing
for the user (which is a matter of personal opinion).

In short, expect that people will have different opinions on this and
will find different compromises better or worse. For one, I prefer to
compromise on "no negative CONFIG options" and use
PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
completely generic FULL_HYPERVISOR option, which means nothing to me.

I cannot see a way to have a good and clear non-negated CONFIG option,
and also satisfy the allyesconfig requirement. So I prefer to compromise
on the "non-negated" part.

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

[Andrew Cooper]

On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
> On Fri, 17 Jan 2025, Jan Beulich wrote:
>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
>>>>> 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@citrix.com>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> ---
>>>>>> 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
>>>> IMO negated options are confusing, and if possible I think we should
>>>> avoid using them unless strictly necessary.
>>> The problem is that the option is negative in nature. It's asking for
>>> hypervisor without _something_. I do agree with Stefano that shim would be
>>> better in the name. Otherwise readers are forced to play divination tricks.
>>>
>>> How about something like:
>>>
>>>     SHIMLESS_HYPERVISOR
>>>
>>> That's arguably not negated, preserves "shim" in the name and has the correct
>>> polarity for allyesconfig to yield the right thing.
>> Except that a hypervisor with this option enabled isn't shim-less, but permits
>> working in shim as well as in non-shim mode.
> First, let's recognize that we have two opposing requirements. One
> requirement is to make it as easy as possible to configure for the user.
> Ideally without using negative CONFIG options, such as
> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
> I think.
>
> On the other hand, we have the requirement that we don't want
> allyesconfig to end up disabling a bunch of CONFIG options. Now this
> requirement can be satisfied easily by adding something like
> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
> requirement.
>
> So we need a compromise, something that doesn't end up disabling other
> CONFIG options, to make allyesconfig happy, but also not too confusing
> for the user (which is a matter of personal opinion).
>
> In short, expect that people will have different opinions on this and
> will find different compromises better or worse. For one, I prefer to
> compromise on "no negative CONFIG options" and use
> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
> completely generic FULL_HYPERVISOR option, which means nothing to me.
>
> I cannot see a way to have a good and clear non-negated CONFIG option,
> and also satisfy the allyesconfig requirement. So I prefer to compromise
> on the "non-negated" part.

Debating the naming is missing the point.


The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
that Kconfig is not capable of expressing.  Specifically, what is broken
is having "lower level" options inhibit unrelated "higher level" options
when the graph gets rescanned[1].  That's why we're in the laughable
position of `make allyesconfig` turning off CONFIG_HVM.

Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
"reset me back to a PV Shim".

Kconfig spells this "make $foo_defconfig" for an appropriately given foo.


There should be:

1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
making the boolean be a compile time constant.

2) a pvshim_defconfig target which expresses what a pvshim ought to look
like.

Trying to fight against the behaviour of Kconfig is not a good use of
anyone's time.

~Andrew

[1] default to unrelated symbols is also broken for a related reason. 
The result you get is sensitive to the order of processing of symbols.

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

[Jan Beulich]

On 18.01.2025 00:41, Andrew Cooper wrote:
> On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
>> On Fri, 17 Jan 2025, Jan Beulich wrote:
>>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
>>>>>> 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@citrix.com>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>
>>>>>>> ---
>>>>>>> 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
>>>>> IMO negated options are confusing, and if possible I think we should
>>>>> avoid using them unless strictly necessary.
>>>> The problem is that the option is negative in nature. It's asking for
>>>> hypervisor without _something_. I do agree with Stefano that shim would be
>>>> better in the name. Otherwise readers are forced to play divination tricks.
>>>>
>>>> How about something like:
>>>>
>>>>     SHIMLESS_HYPERVISOR
>>>>
>>>> That's arguably not negated, preserves "shim" in the name and has the correct
>>>> polarity for allyesconfig to yield the right thing.
>>> Except that a hypervisor with this option enabled isn't shim-less, but permits
>>> working in shim as well as in non-shim mode.
>> First, let's recognize that we have two opposing requirements. One
>> requirement is to make it as easy as possible to configure for the user.
>> Ideally without using negative CONFIG options, such as
>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
>> I think.
>>
>> On the other hand, we have the requirement that we don't want
>> allyesconfig to end up disabling a bunch of CONFIG options. Now this
>> requirement can be satisfied easily by adding something like
>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
>> requirement.
>>
>> So we need a compromise, something that doesn't end up disabling other
>> CONFIG options, to make allyesconfig happy, but also not too confusing
>> for the user (which is a matter of personal opinion).
>>
>> In short, expect that people will have different opinions on this and
>> will find different compromises better or worse. For one, I prefer to
>> compromise on "no negative CONFIG options" and use
>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
>> completely generic FULL_HYPERVISOR option, which means nothing to me.
>>
>> I cannot see a way to have a good and clear non-negated CONFIG option,
>> and also satisfy the allyesconfig requirement. So I prefer to compromise
>> on the "non-negated" part.
> 
> Debating the naming is missing the point.
> 
> 
> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
> that Kconfig is not capable of expressing.  Specifically, what is broken
> is having "lower level" options inhibit unrelated "higher level" options
> when the graph gets rescanned[1].  That's why we're in the laughable
> position of `make allyesconfig` turning off CONFIG_HVM.
> 
> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
> "reset me back to a PV Shim".

Isn't this an independent goal? Or is this a statement on what you see
my change (kind of) doing, indicating you consider this wrong?

> Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
> 
> 
> There should be:
> 
> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
> making the boolean be a compile time constant.

I fear it remains unclear to me what exactly you mean here. It feels like
you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
dropped, without replacement. That seems wrong to me, though. In
particular I'm of the opinion that besides using "make pvshim_defconfig"
people ought to also have the option to properly configure a shim build
from scratch (or from any random .config they hold in hands), requiring
respective "depends on" and/or "select" / "imply" to be in place. Or else
they may end up with a lot of dead code. (Just consider e.g. HVM=n: We
also don't permit HVM-only stuff to be enabled in that case, as any of
that would again be dead code then.)

> 2) a pvshim_defconfig target which expresses what a pvshim ought to look
> like.

We have this file already. I consider it debatable though whether this file
should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
name as either "works just as shim" or "can also work as shim".

> Trying to fight against the behaviour of Kconfig is not a good use of
> anyone's time.
> 
> ~Andrew
> 
> [1] default to unrelated symbols is also broken for a related reason. 
> The result you get is sensitive to the order of processing of symbols.

Is it? It has been my understanding that defaults get re-evaluated as user
input is processed.

Jan

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6510 bytes --]

On Mon, 20 Jan 2025, Jan Beulich wrote:
> On 18.01.2025 00:41, Andrew Cooper wrote:
> > On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
> >> On Fri, 17 Jan 2025, Jan Beulich wrote:
> >>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
> >>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
> >>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> >>>>>> 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@citrix.com>
> >>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>>>
> >>>>>>> ---
> >>>>>>> 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
> >>>>> IMO negated options are confusing, and if possible I think we should
> >>>>> avoid using them unless strictly necessary.
> >>>> The problem is that the option is negative in nature. It's asking for
> >>>> hypervisor without _something_. I do agree with Stefano that shim would be
> >>>> better in the name. Otherwise readers are forced to play divination tricks.
> >>>>
> >>>> How about something like:
> >>>>
> >>>>     SHIMLESS_HYPERVISOR
> >>>>
> >>>> That's arguably not negated, preserves "shim" in the name and has the correct
> >>>> polarity for allyesconfig to yield the right thing.
> >>> Except that a hypervisor with this option enabled isn't shim-less, but permits
> >>> working in shim as well as in non-shim mode.
> >> First, let's recognize that we have two opposing requirements. One
> >> requirement is to make it as easy as possible to configure for the user.
> >> Ideally without using negative CONFIG options, such as
> >> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
> >> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
> >> I think.
> >>
> >> On the other hand, we have the requirement that we don't want
> >> allyesconfig to end up disabling a bunch of CONFIG options. Now this
> >> requirement can be satisfied easily by adding something like
> >> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
> >> requirement.
> >>
> >> So we need a compromise, something that doesn't end up disabling other
> >> CONFIG options, to make allyesconfig happy, but also not too confusing
> >> for the user (which is a matter of personal opinion).
> >>
> >> In short, expect that people will have different opinions on this and
> >> will find different compromises better or worse. For one, I prefer to
> >> compromise on "no negative CONFIG options" and use
> >> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
> >> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
> >> completely generic FULL_HYPERVISOR option, which means nothing to me.
> >>
> >> I cannot see a way to have a good and clear non-negated CONFIG option,
> >> and also satisfy the allyesconfig requirement. So I prefer to compromise
> >> on the "non-negated" part.
> > 
> > Debating the naming is missing the point.
> > 
> > 
> > The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
> > that Kconfig is not capable of expressing.  Specifically, what is broken
> > is having "lower level" options inhibit unrelated "higher level" options
> > when the graph gets rescanned[1].  That's why we're in the laughable
> > position of `make allyesconfig` turning off CONFIG_HVM.
> > 
> > Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
> > "reset me back to a PV Shim".
> 
> Isn't this an independent goal? Or is this a statement on what you see
> my change (kind of) doing, indicating you consider this wrong?

The way I understood it, it is the latter


> > Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
> > 
> > 
> > There should be:
> > 
> > 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
> > making the boolean be a compile time constant.
> 
> I fear it remains unclear to me what exactly you mean here. It feels like
> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
> dropped, without replacement. That seems wrong to me, though. In
> particular I'm of the opinion that besides using "make pvshim_defconfig"
> people ought to also have the option to properly configure a shim build
> from scratch (or from any random .config they hold in hands), requiring
> respective "depends on" and/or "select" / "imply" to be in place.

That should be done with "make pvshim_defconfig"


> Or else they may end up with a lot of dead code. (Just consider e.g.
> HVM=n: We also don't permit HVM-only stuff to be enabled in that case,
> as any of that would again be dead code then.)

It will always be possible to come up with poor configurations. I do not
believe it is necessarily our responsibility to go out of our way to
prevent them.


> > 2) a pvshim_defconfig target which expresses what a pvshim ought to look
> > like.
> 
> We have this file already. I consider it debatable though whether this file
> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
> name as either "works just as shim" or "can also work as shim".

If I understood it right, I like Andrew's suggestion. He is suggesting
to do the following:

- turning PV_SHIM_EXCLUSIVE into something that does nothing
- adding "make pvshim_defconfig"

So that:

- people use "make pvshim_defconfig" to get what today is enabled by
  PV_SHIM_EXCLUSIVE
- but "make allyesconfig" doesn't end up disabling things
- the Kconfig menu makes sense because PV_SHIM_EXCLUSIVE goes away and
  it is not replaced by anything

If I got it right, I am in favor.

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

[Jan Beulich]

On 21.01.2025 00:27, Stefano Stabellini wrote:
> On Mon, 20 Jan 2025, Jan Beulich wrote:
>> On 18.01.2025 00:41, Andrew Cooper wrote:
>>> On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
>>>> On Fri, 17 Jan 2025, Jan Beulich wrote:
>>>>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
>>>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
>>>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
>>>>>>>> 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@citrix.com>
>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> 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
>>>>>>> IMO negated options are confusing, and if possible I think we should
>>>>>>> avoid using them unless strictly necessary.
>>>>>> The problem is that the option is negative in nature. It's asking for
>>>>>> hypervisor without _something_. I do agree with Stefano that shim would be
>>>>>> better in the name. Otherwise readers are forced to play divination tricks.
>>>>>>
>>>>>> How about something like:
>>>>>>
>>>>>>     SHIMLESS_HYPERVISOR
>>>>>>
>>>>>> That's arguably not negated, preserves "shim" in the name and has the correct
>>>>>> polarity for allyesconfig to yield the right thing.
>>>>> Except that a hypervisor with this option enabled isn't shim-less, but permits
>>>>> working in shim as well as in non-shim mode.
>>>> First, let's recognize that we have two opposing requirements. One
>>>> requirement is to make it as easy as possible to configure for the user.
>>>> Ideally without using negative CONFIG options, such as
>>>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
>>>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
>>>> I think.
>>>>
>>>> On the other hand, we have the requirement that we don't want
>>>> allyesconfig to end up disabling a bunch of CONFIG options. Now this
>>>> requirement can be satisfied easily by adding something like
>>>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
>>>> requirement.
>>>>
>>>> So we need a compromise, something that doesn't end up disabling other
>>>> CONFIG options, to make allyesconfig happy, but also not too confusing
>>>> for the user (which is a matter of personal opinion).
>>>>
>>>> In short, expect that people will have different opinions on this and
>>>> will find different compromises better or worse. For one, I prefer to
>>>> compromise on "no negative CONFIG options" and use
>>>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
>>>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
>>>> completely generic FULL_HYPERVISOR option, which means nothing to me.
>>>>
>>>> I cannot see a way to have a good and clear non-negated CONFIG option,
>>>> and also satisfy the allyesconfig requirement. So I prefer to compromise
>>>> on the "non-negated" part.
>>>
>>> Debating the naming is missing the point.
>>>
>>>
>>> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
>>> that Kconfig is not capable of expressing.  Specifically, what is broken
>>> is having "lower level" options inhibit unrelated "higher level" options
>>> when the graph gets rescanned[1].  That's why we're in the laughable
>>> position of `make allyesconfig` turning off CONFIG_HVM.
>>>
>>> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
>>> "reset me back to a PV Shim".
>>
>> Isn't this an independent goal? Or is this a statement on what you see
>> my change (kind of) doing, indicating you consider this wrong?
> 
> The way I understood it, it is the latter
> 
> 
>>> Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
>>>
>>>
>>> There should be:
>>>
>>> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
>>> making the boolean be a compile time constant.
>>
>> I fear it remains unclear to me what exactly you mean here. It feels like
>> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
>> dropped, without replacement. That seems wrong to me, though. In
>> particular I'm of the opinion that besides using "make pvshim_defconfig"
>> people ought to also have the option to properly configure a shim build
>> from scratch (or from any random .config they hold in hands), requiring
>> respective "depends on" and/or "select" / "imply" to be in place.
> 
> That should be done with "make pvshim_defconfig"

Why? Specifically, why should people use only one entirely nailed down
configuration for a shim. Like a "normal" hypervisor, there are aspects
which very well can be left to the person doing the configuration.

>> Or else they may end up with a lot of dead code. (Just consider e.g.
>> HVM=n: We also don't permit HVM-only stuff to be enabled in that case,
>> as any of that would again be dead code then.)
> 
> It will always be possible to come up with poor configurations. I do not
> believe it is necessarily our responsibility to go out of our way to
> prevent them.

Well - if so, why would we do this in some cases but not in others?
You may recall that I'm a proponent of consistency and predictability.

>>> 2) a pvshim_defconfig target which expresses what a pvshim ought to look
>>> like.
>>
>> We have this file already. I consider it debatable though whether this file
>> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
>> name as either "works just as shim" or "can also work as shim".
> 
> If I understood it right, I like Andrew's suggestion. He is suggesting
> to do the following:
> 
> - turning PV_SHIM_EXCLUSIVE into something that does nothing

FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
"nothing other than making the boolean be a compile time constant".

> - adding "make pvshim_defconfig"

Why do you say "adding"? We have this already.

Jan

> So that:
> 
> - people use "make pvshim_defconfig" to get what today is enabled by
>   PV_SHIM_EXCLUSIVE
> - but "make allyesconfig" doesn't end up disabling things
> - the Kconfig menu makes sense because PV_SHIM_EXCLUSIVE goes away and
>   it is not replaced by anything
> 
> If I got it right, I am in favor.

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

[Roger Pau Monné]

On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
> On 21.01.2025 00:27, Stefano Stabellini wrote:
> > On Mon, 20 Jan 2025, Jan Beulich wrote:
> >> On 18.01.2025 00:41, Andrew Cooper wrote:
> >>> On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
> >>>> On Fri, 17 Jan 2025, Jan Beulich wrote:
> >>>>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
> >>>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
> >>>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> >>>>>>>> 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@citrix.com>
> >>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>
> >>>>>>>>> ---
> >>>>>>>>> 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
> >>>>>>> IMO negated options are confusing, and if possible I think we should
> >>>>>>> avoid using them unless strictly necessary.
> >>>>>> The problem is that the option is negative in nature. It's asking for
> >>>>>> hypervisor without _something_. I do agree with Stefano that shim would be
> >>>>>> better in the name. Otherwise readers are forced to play divination tricks.
> >>>>>>
> >>>>>> How about something like:
> >>>>>>
> >>>>>>     SHIMLESS_HYPERVISOR
> >>>>>>
> >>>>>> That's arguably not negated, preserves "shim" in the name and has the correct
> >>>>>> polarity for allyesconfig to yield the right thing.
> >>>>> Except that a hypervisor with this option enabled isn't shim-less, but permits
> >>>>> working in shim as well as in non-shim mode.
> >>>> First, let's recognize that we have two opposing requirements. One
> >>>> requirement is to make it as easy as possible to configure for the user.
> >>>> Ideally without using negative CONFIG options, such as
> >>>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
> >>>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
> >>>> I think.
> >>>>
> >>>> On the other hand, we have the requirement that we don't want
> >>>> allyesconfig to end up disabling a bunch of CONFIG options. Now this
> >>>> requirement can be satisfied easily by adding something like
> >>>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
> >>>> requirement.
> >>>>
> >>>> So we need a compromise, something that doesn't end up disabling other
> >>>> CONFIG options, to make allyesconfig happy, but also not too confusing
> >>>> for the user (which is a matter of personal opinion).
> >>>>
> >>>> In short, expect that people will have different opinions on this and
> >>>> will find different compromises better or worse. For one, I prefer to
> >>>> compromise on "no negative CONFIG options" and use
> >>>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
> >>>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
> >>>> completely generic FULL_HYPERVISOR option, which means nothing to me.
> >>>>
> >>>> I cannot see a way to have a good and clear non-negated CONFIG option,
> >>>> and also satisfy the allyesconfig requirement. So I prefer to compromise
> >>>> on the "non-negated" part.
> >>>
> >>> Debating the naming is missing the point.
> >>>
> >>>
> >>> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
> >>> that Kconfig is not capable of expressing.  Specifically, what is broken
> >>> is having "lower level" options inhibit unrelated "higher level" options
> >>> when the graph gets rescanned[1].  That's why we're in the laughable
> >>> position of `make allyesconfig` turning off CONFIG_HVM.
> >>>
> >>> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
> >>> "reset me back to a PV Shim".
> >>
> >> Isn't this an independent goal? Or is this a statement on what you see
> >> my change (kind of) doing, indicating you consider this wrong?
> > 
> > The way I understood it, it is the latter
> > 
> > 
> >>> Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
> >>>
> >>>
> >>> There should be:
> >>>
> >>> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
> >>> making the boolean be a compile time constant.
> >>
> >> I fear it remains unclear to me what exactly you mean here. It feels like
> >> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
> >> dropped, without replacement. That seems wrong to me, though. In
> >> particular I'm of the opinion that besides using "make pvshim_defconfig"
> >> people ought to also have the option to properly configure a shim build
> >> from scratch (or from any random .config they hold in hands), requiring
> >> respective "depends on" and/or "select" / "imply" to be in place.
> > 
> > That should be done with "make pvshim_defconfig"
> 
> Why? Specifically, why should people use only one entirely nailed down
> configuration for a shim. Like a "normal" hypervisor, there are aspects
> which very well can be left to the person doing the configuration.

But nothing prevents a user from starting from a shim defconfig, and
then tweaking it as desired:

$ make pvshim_defconfig
$ make menuconfig

Or there's something I'm missing here?

> >> Or else they may end up with a lot of dead code. (Just consider e.g.
> >> HVM=n: We also don't permit HVM-only stuff to be enabled in that case,
> >> as any of that would again be dead code then.)
> > 
> > It will always be possible to come up with poor configurations. I do not
> > believe it is necessarily our responsibility to go out of our way to
> > prevent them.
> 
> Well - if so, why would we do this in some cases but not in others?
> You may recall that I'm a proponent of consistency and predictability.
> 
> >>> 2) a pvshim_defconfig target which expresses what a pvshim ought to look
> >>> like.
> >>
> >> We have this file already. I consider it debatable though whether this file
> >> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
> >> name as either "works just as shim" or "can also work as shim".
> > 
> > If I understood it right, I like Andrew's suggestion. He is suggesting
> > to do the following:
> > 
> > - turning PV_SHIM_EXCLUSIVE into something that does nothing
> 
> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
> "nothing other than making the boolean be a compile time constant".

Won't making the boolean a compile time constant would also result in
DCO kicking in and removing a fair amount of code?  So even if you
have enabled everything in Kconfig, the resulting hypervisor would
only be suitable to be used as a shim?

Thanks, Roger.

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

[Jan Beulich]

On 21.01.2025 09:52, Roger Pau Monné wrote:
> On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
>> On 21.01.2025 00:27, Stefano Stabellini wrote:
>>> On Mon, 20 Jan 2025, Jan Beulich wrote:
>>>> On 18.01.2025 00:41, Andrew Cooper wrote:
>>>>> On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
>>>>>> On Fri, 17 Jan 2025, Jan Beulich wrote:
>>>>>>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
>>>>>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
>>>>>>>>>> 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@citrix.com>
>>>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> 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
>>>>>>>>> IMO negated options are confusing, and if possible I think we should
>>>>>>>>> avoid using them unless strictly necessary.
>>>>>>>> The problem is that the option is negative in nature. It's asking for
>>>>>>>> hypervisor without _something_. I do agree with Stefano that shim would be
>>>>>>>> better in the name. Otherwise readers are forced to play divination tricks.
>>>>>>>>
>>>>>>>> How about something like:
>>>>>>>>
>>>>>>>>     SHIMLESS_HYPERVISOR
>>>>>>>>
>>>>>>>> That's arguably not negated, preserves "shim" in the name and has the correct
>>>>>>>> polarity for allyesconfig to yield the right thing.
>>>>>>> Except that a hypervisor with this option enabled isn't shim-less, but permits
>>>>>>> working in shim as well as in non-shim mode.
>>>>>> First, let's recognize that we have two opposing requirements. One
>>>>>> requirement is to make it as easy as possible to configure for the user.
>>>>>> Ideally without using negative CONFIG options, such as
>>>>>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
>>>>>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
>>>>>> I think.
>>>>>>
>>>>>> On the other hand, we have the requirement that we don't want
>>>>>> allyesconfig to end up disabling a bunch of CONFIG options. Now this
>>>>>> requirement can be satisfied easily by adding something like
>>>>>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
>>>>>> requirement.
>>>>>>
>>>>>> So we need a compromise, something that doesn't end up disabling other
>>>>>> CONFIG options, to make allyesconfig happy, but also not too confusing
>>>>>> for the user (which is a matter of personal opinion).
>>>>>>
>>>>>> In short, expect that people will have different opinions on this and
>>>>>> will find different compromises better or worse. For one, I prefer to
>>>>>> compromise on "no negative CONFIG options" and use
>>>>>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
>>>>>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
>>>>>> completely generic FULL_HYPERVISOR option, which means nothing to me.
>>>>>>
>>>>>> I cannot see a way to have a good and clear non-negated CONFIG option,
>>>>>> and also satisfy the allyesconfig requirement. So I prefer to compromise
>>>>>> on the "non-negated" part.
>>>>>
>>>>> Debating the naming is missing the point.
>>>>>
>>>>>
>>>>> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
>>>>> that Kconfig is not capable of expressing.  Specifically, what is broken
>>>>> is having "lower level" options inhibit unrelated "higher level" options
>>>>> when the graph gets rescanned[1].  That's why we're in the laughable
>>>>> position of `make allyesconfig` turning off CONFIG_HVM.
>>>>>
>>>>> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
>>>>> "reset me back to a PV Shim".
>>>>
>>>> Isn't this an independent goal? Or is this a statement on what you see
>>>> my change (kind of) doing, indicating you consider this wrong?
>>>
>>> The way I understood it, it is the latter
>>>
>>>
>>>>> Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
>>>>>
>>>>>
>>>>> There should be:
>>>>>
>>>>> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
>>>>> making the boolean be a compile time constant.
>>>>
>>>> I fear it remains unclear to me what exactly you mean here. It feels like
>>>> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
>>>> dropped, without replacement. That seems wrong to me, though. In
>>>> particular I'm of the opinion that besides using "make pvshim_defconfig"
>>>> people ought to also have the option to properly configure a shim build
>>>> from scratch (or from any random .config they hold in hands), requiring
>>>> respective "depends on" and/or "select" / "imply" to be in place.
>>>
>>> That should be done with "make pvshim_defconfig"
>>
>> Why? Specifically, why should people use only one entirely nailed down
>> configuration for a shim. Like a "normal" hypervisor, there are aspects
>> which very well can be left to the person doing the configuration.
> 
> But nothing prevents a user from starting from a shim defconfig, and
> then tweaking it as desired:
> 
> $ make pvshim_defconfig
> $ make menuconfig
> 
> Or there's something I'm missing here?

Well, no, you don't. But if the above is okay, why would not starting from
pvshim_defconfig not also be okay? Plus whichever way tweaks are done,
sensible dependencies should still be enforced imo.

>>>> Or else they may end up with a lot of dead code. (Just consider e.g.
>>>> HVM=n: We also don't permit HVM-only stuff to be enabled in that case,
>>>> as any of that would again be dead code then.)
>>>
>>> It will always be possible to come up with poor configurations. I do not
>>> believe it is necessarily our responsibility to go out of our way to
>>> prevent them.
>>
>> Well - if so, why would we do this in some cases but not in others?
>> You may recall that I'm a proponent of consistency and predictability.
>>
>>>>> 2) a pvshim_defconfig target which expresses what a pvshim ought to look
>>>>> like.
>>>>
>>>> We have this file already. I consider it debatable though whether this file
>>>> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
>>>> name as either "works just as shim" or "can also work as shim".
>>>
>>> If I understood it right, I like Andrew's suggestion. He is suggesting
>>> to do the following:
>>>
>>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
>>
>> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
>> "nothing other than making the boolean be a compile time constant".
> 
> Won't making the boolean a compile time constant would also result in
> DCO kicking in and removing a fair amount of code?  So even if you
> have enabled everything in Kconfig, the resulting hypervisor would
> only be suitable to be used as a shim?

Of course.

Jan

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

[Roger Pau Monné]

On Tue, Jan 21, 2025 at 11:35:42AM +0100, Jan Beulich wrote:
> On 21.01.2025 09:52, Roger Pau Monné wrote:
> > On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
> >> On 21.01.2025 00:27, Stefano Stabellini wrote:
> >>> On Mon, 20 Jan 2025, Jan Beulich wrote:
> >>>> On 18.01.2025 00:41, Andrew Cooper wrote:
> >>>>> On 17/01/2025 10:43 pm, Stefano Stabellini wrote:
> >>>>>> On Fri, 17 Jan 2025, Jan Beulich wrote:
> >>>>>>> On 17.01.2025 13:24, Alejandro Vallejo wrote:
> >>>>>>>> On Fri Jan 17, 2025 at 10:31 AM GMT, Roger Pau Monné wrote:
> >>>>>>>>> On Thu, Jan 16, 2025 at 04:31:46PM -0800, Stefano Stabellini wrote:
> >>>>>>>>>> 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@citrix.com>
> >>>>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>>>>>>>
> >>>>>>>>>>> ---
> >>>>>>>>>>> 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
> >>>>>>>>> IMO negated options are confusing, and if possible I think we should
> >>>>>>>>> avoid using them unless strictly necessary.
> >>>>>>>> The problem is that the option is negative in nature. It's asking for
> >>>>>>>> hypervisor without _something_. I do agree with Stefano that shim would be
> >>>>>>>> better in the name. Otherwise readers are forced to play divination tricks.
> >>>>>>>>
> >>>>>>>> How about something like:
> >>>>>>>>
> >>>>>>>>     SHIMLESS_HYPERVISOR
> >>>>>>>>
> >>>>>>>> That's arguably not negated, preserves "shim" in the name and has the correct
> >>>>>>>> polarity for allyesconfig to yield the right thing.
> >>>>>>> Except that a hypervisor with this option enabled isn't shim-less, but permits
> >>>>>>> working in shim as well as in non-shim mode.
> >>>>>> First, let's recognize that we have two opposing requirements. One
> >>>>>> requirement is to make it as easy as possible to configure for the user.
> >>>>>> Ideally without using negative CONFIG options, such as
> >>>>>> NO_PV_SHIM_EXCLUSIVE. From the user point of view, honestly,
> >>>>>> PV_SHIM_EXCLUSIVE is a pretty good name. Better than all of the others,
> >>>>>> I think.
> >>>>>>
> >>>>>> On the other hand, we have the requirement that we don't want
> >>>>>> allyesconfig to end up disabling a bunch of CONFIG options. Now this
> >>>>>> requirement can be satisfied easily by adding something like
> >>>>>> NO_PV_SHIM_EXCLUSIVE. However, it would go somewhat against the previous
> >>>>>> requirement.
> >>>>>>
> >>>>>> So we need a compromise, something that doesn't end up disabling other
> >>>>>> CONFIG options, to make allyesconfig happy, but also not too confusing
> >>>>>> for the user (which is a matter of personal opinion).
> >>>>>>
> >>>>>> In short, expect that people will have different opinions on this and
> >>>>>> will find different compromises better or worse. For one, I prefer to
> >>>>>> compromise on "no negative CONFIG options" and use
> >>>>>> PV_SHIM_NOT_EXCLUSIVE. Because it serves the allyesconfig goal, and
> >>>>>> while it is not as clear as PV_SHIM_EXCLUSIVE, is still better than a
> >>>>>> completely generic FULL_HYPERVISOR option, which means nothing to me.
> >>>>>>
> >>>>>> I cannot see a way to have a good and clear non-negated CONFIG option,
> >>>>>> and also satisfy the allyesconfig requirement. So I prefer to compromise
> >>>>>> on the "non-negated" part.
> >>>>>
> >>>>> Debating the naming is missing the point.
> >>>>>
> >>>>>
> >>>>> The problem here is the wish to have PV_SHIM_EXCLUSIVE behave in a way
> >>>>> that Kconfig is not capable of expressing.  Specifically, what is broken
> >>>>> is having "lower level" options inhibit unrelated "higher level" options
> >>>>> when the graph gets rescanned[1].  That's why we're in the laughable
> >>>>> position of `make allyesconfig` turning off CONFIG_HVM.
> >>>>>
> >>>>> Jan, you want "echo PV_SHIM_EXCLUSIVE=y >> .config && make" to mean
> >>>>> "reset me back to a PV Shim".
> >>>>
> >>>> Isn't this an independent goal? Or is this a statement on what you see
> >>>> my change (kind of) doing, indicating you consider this wrong?
> >>>
> >>> The way I understood it, it is the latter
> >>>
> >>>
> >>>>> Kconfig spells this "make $foo_defconfig" for an appropriately given foo.
> >>>>>
> >>>>>
> >>>>> There should be:
> >>>>>
> >>>>> 1) an option called PV_SHIM_EXCLUSIVE which does *nothing* other than
> >>>>> making the boolean be a compile time constant.
> >>>>
> >>>> I fear it remains unclear to me what exactly you mean here. It feels like
> >>>> you may be suggesting that all other uses of PV_SHIM_EXCLUSIVE should be
> >>>> dropped, without replacement. That seems wrong to me, though. In
> >>>> particular I'm of the opinion that besides using "make pvshim_defconfig"
> >>>> people ought to also have the option to properly configure a shim build
> >>>> from scratch (or from any random .config they hold in hands), requiring
> >>>> respective "depends on" and/or "select" / "imply" to be in place.
> >>>
> >>> That should be done with "make pvshim_defconfig"
> >>
> >> Why? Specifically, why should people use only one entirely nailed down
> >> configuration for a shim. Like a "normal" hypervisor, there are aspects
> >> which very well can be left to the person doing the configuration.
> > 
> > But nothing prevents a user from starting from a shim defconfig, and
> > then tweaking it as desired:
> > 
> > $ make pvshim_defconfig
> > $ make menuconfig
> > 
> > Or there's something I'm missing here?
> 
> Well, no, you don't. But if the above is okay, why would not starting from
> pvshim_defconfig not also be okay? Plus whichever way tweaks are done,
> sensible dependencies should still be enforced imo.

Not starting from pvshim_defconfig should always be OK, as the
defconfig file is just a set of options that the user can otherwise
enable manually.

There are two different things that PV_SHIM_EXCLUSIVE accomplishes:
 - Use to remove code blocks or change defines:  for example
   short-circuiting PG_log_dirty to 0.  This should likely be done
   using a different more fine grained set of Kconfig options.
 - Convert pv_shim to a compile time constant: this is the tricky part
   IMO, as such conversion will force DCO and thus make the resulting
   Xen binary no longer what a user would expect when using
   allyesconfig.

> >>>> Or else they may end up with a lot of dead code. (Just consider e.g.
> >>>> HVM=n: We also don't permit HVM-only stuff to be enabled in that case,
> >>>> as any of that would again be dead code then.)
> >>>
> >>> It will always be possible to come up with poor configurations. I do not
> >>> believe it is necessarily our responsibility to go out of our way to
> >>> prevent them.
> >>
> >> Well - if so, why would we do this in some cases but not in others?
> >> You may recall that I'm a proponent of consistency and predictability.
> >>
> >>>>> 2) a pvshim_defconfig target which expresses what a pvshim ought to look
> >>>>> like.
> >>>>
> >>>> We have this file already. I consider it debatable though whether this file
> >>>> should really force PV_SHIM_EXCLUSIVE=y. People may read "pvshim" in the
> >>>> name as either "works just as shim" or "can also work as shim".
> >>>
> >>> If I understood it right, I like Andrew's suggestion. He is suggesting
> >>> to do the following:
> >>>
> >>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
> >>
> >> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
> >> "nothing other than making the boolean be a compile time constant".
> > 
> > Won't making the boolean a compile time constant would also result in
> > DCO kicking in and removing a fair amount of code?  So even if you
> > have enabled everything in Kconfig, the resulting hypervisor would
> > only be suitable to be used as a shim?
> 
> Of course.

Then what's the point of this approach?  Options will be enabled in
Kconfig, but the resulting hypervisor build when using allyesconfig
would have a lot of them short-circuited, making it even worse than
currently?  As options will get effectively build-time disabled due
to DCO while enabled in Kconfig.

Overall I think PV_SHIM_EXCLUSIVE should be excluded from
allyesconfig, even with Andrew's proposed change.  Otherwise the
purpose of allyesconfig is defeated if enabling PV_SHIM_EXCLUSIVE
makes the resulting hypervisor build PV shim only.  IIRC we can
provide a default alllyes.config with CONFIG_PV_SHIM_EXCLUSIVE=n.

Thanks, Roger.

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

[Jan Beulich]

On 21.01.2025 19:02, Roger Pau Monné wrote:
> On Tue, Jan 21, 2025 at 11:35:42AM +0100, Jan Beulich wrote:
>> On 21.01.2025 09:52, Roger Pau Monné wrote:
>>> On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
>>>> On 21.01.2025 00:27, Stefano Stabellini wrote:
>>>>> If I understood it right, I like Andrew's suggestion. He is suggesting
>>>>> to do the following:
>>>>>
>>>>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
>>>>
>>>> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
>>>> "nothing other than making the boolean be a compile time constant".
>>>
>>> Won't making the boolean a compile time constant would also result in
>>> DCO kicking in and removing a fair amount of code?  So even if you
>>> have enabled everything in Kconfig, the resulting hypervisor would
>>> only be suitable to be used as a shim?
>>
>> Of course.
> 
> Then what's the point of this approach?  Options will be enabled in
> Kconfig, but the resulting hypervisor build when using allyesconfig
> would have a lot of them short-circuited, making it even worse than
> currently?  As options will get effectively build-time disabled due
> to DCO while enabled in Kconfig.

Well, I have to direct this question to Andrew. It is specifically
what I'm trying to address with UNCONSTRAINED.

> Overall I think PV_SHIM_EXCLUSIVE should be excluded from
> allyesconfig, even with Andrew's proposed change.  Otherwise the
> purpose of allyesconfig is defeated if enabling PV_SHIM_EXCLUSIVE
> makes the resulting hypervisor build PV shim only.  IIRC we can
> provide a default alllyes.config with CONFIG_PV_SHIM_EXCLUSIVE=n.

Hmm, I wasn't aware of the option of using allyes.config. That might be
the route to go, albeit it looks like people using the allyesconfig
target then need to remember to set KCONFIG_ALLCONFIG in the environment
(to <empty> or 1), or to explicitly specify a file name that way. (This
of course ought to be easy enough to arrange for in our automation.)

Jan

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

[Roger Pau Monné]

On Wed, Jan 22, 2025 at 09:43:53AM +0100, Jan Beulich wrote:
> On 21.01.2025 19:02, Roger Pau Monné wrote:
> > On Tue, Jan 21, 2025 at 11:35:42AM +0100, Jan Beulich wrote:
> >> On 21.01.2025 09:52, Roger Pau Monné wrote:
> >>> On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
> >>>> On 21.01.2025 00:27, Stefano Stabellini wrote:
> >>>>> If I understood it right, I like Andrew's suggestion. He is suggesting
> >>>>> to do the following:
> >>>>>
> >>>>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
> >>>>
> >>>> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
> >>>> "nothing other than making the boolean be a compile time constant".
> >>>
> >>> Won't making the boolean a compile time constant would also result in
> >>> DCO kicking in and removing a fair amount of code?  So even if you
> >>> have enabled everything in Kconfig, the resulting hypervisor would
> >>> only be suitable to be used as a shim?
> >>
> >> Of course.
> > 
> > Then what's the point of this approach?  Options will be enabled in
> > Kconfig, but the resulting hypervisor build when using allyesconfig
> > would have a lot of them short-circuited, making it even worse than
> > currently?  As options will get effectively build-time disabled due
> > to DCO while enabled in Kconfig.
> 
> Well, I have to direct this question to Andrew. It is specifically
> what I'm trying to address with UNCONSTRAINED.
> 
> > Overall I think PV_SHIM_EXCLUSIVE should be excluded from
> > allyesconfig, even with Andrew's proposed change.  Otherwise the
> > purpose of allyesconfig is defeated if enabling PV_SHIM_EXCLUSIVE
> > makes the resulting hypervisor build PV shim only.  IIRC we can
> > provide a default alllyes.config with CONFIG_PV_SHIM_EXCLUSIVE=n.
> 
> Hmm, I wasn't aware of the option of using allyes.config. That might be
> the route to go, albeit it looks like people using the allyesconfig
> target then need to remember to set KCONFIG_ALLCONFIG in the environment
> (to <empty> or 1), or to explicitly specify a file name that way. (This
> of course ought to be easy enough to arrange for in our automation.)

My knowledge of Kconfig is very limited, but isn't there a default
path for such file to be picked up by Kconfig?  I see we already have
a xen/tools/kconfig/allrandom.config, I was expecting it would be a
matter of dropping an allyes.config in that directory, but I haven't
tried.

Thanks, Roger.

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

[Jan Beulich]

On 22.01.2025 10:49, Roger Pau Monné wrote:
> On Wed, Jan 22, 2025 at 09:43:53AM +0100, Jan Beulich wrote:
>> On 21.01.2025 19:02, Roger Pau Monné wrote:
>>> On Tue, Jan 21, 2025 at 11:35:42AM +0100, Jan Beulich wrote:
>>>> On 21.01.2025 09:52, Roger Pau Monné wrote:
>>>>> On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
>>>>>> On 21.01.2025 00:27, Stefano Stabellini wrote:
>>>>>>> If I understood it right, I like Andrew's suggestion. He is suggesting
>>>>>>> to do the following:
>>>>>>>
>>>>>>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
>>>>>>
>>>>>> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
>>>>>> "nothing other than making the boolean be a compile time constant".
>>>>>
>>>>> Won't making the boolean a compile time constant would also result in
>>>>> DCO kicking in and removing a fair amount of code?  So even if you
>>>>> have enabled everything in Kconfig, the resulting hypervisor would
>>>>> only be suitable to be used as a shim?
>>>>
>>>> Of course.
>>>
>>> Then what's the point of this approach?  Options will be enabled in
>>> Kconfig, but the resulting hypervisor build when using allyesconfig
>>> would have a lot of them short-circuited, making it even worse than
>>> currently?  As options will get effectively build-time disabled due
>>> to DCO while enabled in Kconfig.
>>
>> Well, I have to direct this question to Andrew. It is specifically
>> what I'm trying to address with UNCONSTRAINED.
>>
>>> Overall I think PV_SHIM_EXCLUSIVE should be excluded from
>>> allyesconfig, even with Andrew's proposed change.  Otherwise the
>>> purpose of allyesconfig is defeated if enabling PV_SHIM_EXCLUSIVE
>>> makes the resulting hypervisor build PV shim only.  IIRC we can
>>> provide a default alllyes.config with CONFIG_PV_SHIM_EXCLUSIVE=n.
>>
>> Hmm, I wasn't aware of the option of using allyes.config. That might be
>> the route to go, albeit it looks like people using the allyesconfig
>> target then need to remember to set KCONFIG_ALLCONFIG in the environment
>> (to <empty> or 1), or to explicitly specify a file name that way. (This
>> of course ought to be easy enough to arrange for in our automation.)
> 
> My knowledge of Kconfig is very limited, but isn't there a default
> path for such file to be picked up by Kconfig?  I see we already have
> a xen/tools/kconfig/allrandom.config, I was expecting it would be a
> matter of dropping an allyes.config in that directory, but I haven't
> tried.

Well, I simply looked at the kconfig sources, and my reading of it is
that it won't even try to open allyes.config when the envvar is absent.

Jan

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On Wed, 22 Jan 2025, Jan Beulich wrote:
> On 22.01.2025 10:49, Roger Pau Monné wrote:
> > On Wed, Jan 22, 2025 at 09:43:53AM +0100, Jan Beulich wrote:
> >> On 21.01.2025 19:02, Roger Pau Monné wrote:
> >>> On Tue, Jan 21, 2025 at 11:35:42AM +0100, Jan Beulich wrote:
> >>>> On 21.01.2025 09:52, Roger Pau Monné wrote:
> >>>>> On Tue, Jan 21, 2025 at 09:13:38AM +0100, Jan Beulich wrote:
> >>>>>> On 21.01.2025 00:27, Stefano Stabellini wrote:
> >>>>>>> If I understood it right, I like Andrew's suggestion. He is suggesting
> >>>>>>> to do the following:
> >>>>>>>
> >>>>>>> - turning PV_SHIM_EXCLUSIVE into something that does nothing
> >>>>>>
> >>>>>> FTAOD - you mean Kconfig-wise? Andrew clearly didn't say "nothing", but
> >>>>>> "nothing other than making the boolean be a compile time constant".
> >>>>>
> >>>>> Won't making the boolean a compile time constant would also result in
> >>>>> DCO kicking in and removing a fair amount of code?  So even if you
> >>>>> have enabled everything in Kconfig, the resulting hypervisor would
> >>>>> only be suitable to be used as a shim?
> >>>>
> >>>> Of course.
> >>>
> >>> Then what's the point of this approach?  Options will be enabled in
> >>> Kconfig, but the resulting hypervisor build when using allyesconfig
> >>> would have a lot of them short-circuited, making it even worse than
> >>> currently?  As options will get effectively build-time disabled due
> >>> to DCO while enabled in Kconfig.
> >>
> >> Well, I have to direct this question to Andrew. It is specifically
> >> what I'm trying to address with UNCONSTRAINED.
> >>
> >>> Overall I think PV_SHIM_EXCLUSIVE should be excluded from
> >>> allyesconfig, even with Andrew's proposed change.  Otherwise the
> >>> purpose of allyesconfig is defeated if enabling PV_SHIM_EXCLUSIVE
> >>> makes the resulting hypervisor build PV shim only.  IIRC we can
> >>> provide a default alllyes.config with CONFIG_PV_SHIM_EXCLUSIVE=n.
> >>
> >> Hmm, I wasn't aware of the option of using allyes.config. That might be
> >> the route to go, albeit it looks like people using the allyesconfig
> >> target then need to remember to set KCONFIG_ALLCONFIG in the environment
> >> (to <empty> or 1), or to explicitly specify a file name that way. (This
> >> of course ought to be easy enough to arrange for in our automation.)
> > 
> > My knowledge of Kconfig is very limited, but isn't there a default
> > path for such file to be picked up by Kconfig?  I see we already have
> > a xen/tools/kconfig/allrandom.config, I was expecting it would be a
> > matter of dropping an allyes.config in that directory, but I haven't
> > tried.
> 
> Well, I simply looked at the kconfig sources, and my reading of it is
> that it won't even try to open allyes.config when the envvar is absent.

Reading the thread, I think that:

- using allyes.config as Roger suggested
- arranging for KCONFIG_ALLCONFIG to be set as needed
- leaving PV_SHIM_EXCLUSIVE as is

is the simplest way forward

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

[Stefano Stabellini]

Hi all,

The topic was discussed today during the committers and core maintainers
call and the decision was to remove the PV_SHIM_EXCLUSIVE Kconfig
option.

Cheers,

Stefano