[PATCH 0/3] firmware/shim: build adjustments

[PATCH 0/3] firmware/shim: build adjustments

[Jan Beulich]

Originally I meant to finally get v2 of "firmware/shim: honor symlinks
during Xen tree setup" sorted. However, the suggestion to use find's
-L option, while a suitable equivalent of the -xtype primary, has the
same drawback: It doesn't distinguish between relative and absolute
symlinks (and we specifically want to skip relative ones). Locally I'm
using '(' -type f -o -lname '/*' ')' now, but -lname again being non-
standard I didn't think it would even be worth submitting. While
looking into that I did notice a few other anomalies, though, which
this series tries to address.

I notice tools/firmware/xen-dir/ isn't included in "X86 ARCHITECTURE".
I wonder whether that should be added.

1: update linkfarm exclusions
2: drop XEN_CONFIG_EXPERT uses
3: UNSUPPORTED=n

Jan

[PATCH 1/3] firmware/shim: update linkfarm exclusions

[Jan Beulich]

Some intermediate files weren't considered at all at the time. Also
after its introduction, various changes to the build environment have
rendered the exclusion sets stale. For example, we now have some .*.cmd
files in the build tree.  Combine all respective patterns into a single
.* one, seeing that we don't have any actual source files matching this
pattern in the tree. Add other patterns as well as individual files.
Also introduce LINK_EXCLUDE_PATHS to deal with entire directories full
of generated headers as well as a few specific files the names of which
are too generic to list under LINK_EXCLUDES.

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

--- a/tools/firmware/xen-dir/Makefile
+++ b/tools/firmware/xen-dir/Makefile
@@ -15,9 +15,19 @@ DEP_DIRS=$(foreach i, $(LINK_DIRS), $(XE
 DEP_FILES=$(foreach i, $(LINK_FILES), $(XEN_ROOT)/$(i))
 
 # Exclude some intermediate files and final build products
-LINK_EXCLUDES := '*.[isoa]' '.*.d' '.*.d2' '.config'
-LINK_EXCLUDES += '*.map' 'xen' 'xen.gz' 'xen.efi' 'xen-syms'
-LINK_EXCLUDES += '.*.tmp'
+LINK_EXCLUDES := '*.[isoa]' '*.bin' '*.chk' '*.lnk' '*.gz' '.*'
+LINK_EXCLUDES += lexer.lex.? parser.tab.? conf
+LINK_EXCLUDES += asm-offsets.h asm-macros.h compile.h '*-autogen.h'
+LINK_EXCLUDES += mkelf32 mkreloc symbols config_data.S xen.lds efi.lds
+LINK_EXCLUDES += '*.map' xen xen.gz xen.efi xen-syms check.efi
+
+# To exclude full subtrees or individual files of not sufficiently specific
+# names, regular expressions are used:
+LINK_EXCLUDE_PATHS := xen/include/compat/.*
+LINK_EXCLUDE_PATHS += xen/include/config/.*
+LINK_EXCLUDE_PATHS += xen/include/generated/.*
+LINK_EXCLUDE_PATHS += xen/arch/x86/boot/reloc[.]S
+LINK_EXCLUDE_PATHS += xen/arch/x86/boot/cmdline[.]S
 
 # This is all a giant mess and doesn't really work.
 #
@@ -32,9 +42,10 @@ LINK_EXCLUDES += '.*.tmp'
 # support easy development of the shim, but has a side effect of clobbering
 # the already-built shim.
 #
-# $(LINK_EXCLUDES) should be set such that a parallel build of shim and xen/
-# doesn't cause a subsequent `make install` to decide to regenerate the
-# linkfarm.  This means that all final build artefacts must be excluded.
+# $(LINK_EXCLUDES) and $(LINK_EXCLUDE_DIRS) should be set such that a parallel
+# build of shim and xen/ doesn't cause a subsequent `make install` to decide to
+# to regenerate the linkfarm.  This means that all intermediate and final build
+# artefacts must be excluded.
 linkfarm.stamp: $(DEP_DIRS) $(DEP_FILES) FORCE
 	mkdir -p $(D)
 	rm -f linkfarm.stamp.tmp
@@ -46,7 +57,8 @@ linkfarm.stamp: $(DEP_DIRS) $(DEP_FILES)
 			sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p .);) \
 	$(foreach d, $(LINK_DIRS), \
 		(cd $(XEN_ROOT); \
-		 find $(d) ! -type l -type f $(addprefix ! -name ,$(LINK_EXCLUDES))) \
+		 find $(d) ! -type l -type f $(addprefix ! -name ,$(LINK_EXCLUDES)) \
+		 | grep -v $(patsubst %,-e '^%$$',$(LINK_EXCLUDE_PATHS))) \
 		 >> linkfarm.stamp.tmp ; ) \
 	$(foreach f, $(LINK_FILES), \
 		echo $(f) >> linkfarm.stamp.tmp ;)

Re: [PATCH 1/3] firmware/shim: update linkfarm exclusions

[Roger Pau Monné]

On Fri, Apr 30, 2021 at 04:43:59PM +0200, Jan Beulich wrote:
> Some intermediate files weren't considered at all at the time. Also
> after its introduction, various changes to the build environment have
> rendered the exclusion sets stale. For example, we now have some .*.cmd
> files in the build tree.  Combine all respective patterns into a single
> .* one, seeing that we don't have any actual source files matching this
> pattern in the tree. Add other patterns as well as individual files.
> Also introduce LINK_EXCLUDE_PATHS to deal with entire directories full
> of generated headers as well as a few specific files the names of which
> are too generic to list under LINK_EXCLUDES.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> --- a/tools/firmware/xen-dir/Makefile
> +++ b/tools/firmware/xen-dir/Makefile
> @@ -15,9 +15,19 @@ DEP_DIRS=$(foreach i, $(LINK_DIRS), $(XE
>  DEP_FILES=$(foreach i, $(LINK_FILES), $(XEN_ROOT)/$(i))
>  
>  # Exclude some intermediate files and final build products
> -LINK_EXCLUDES := '*.[isoa]' '.*.d' '.*.d2' '.config'
> -LINK_EXCLUDES += '*.map' 'xen' 'xen.gz' 'xen.efi' 'xen-syms'
> -LINK_EXCLUDES += '.*.tmp'
> +LINK_EXCLUDES := '*.[isoa]' '*.bin' '*.chk' '*.lnk' '*.gz' '.*'
> +LINK_EXCLUDES += lexer.lex.? parser.tab.? conf
> +LINK_EXCLUDES += asm-offsets.h asm-macros.h compile.h '*-autogen.h'
> +LINK_EXCLUDES += mkelf32 mkreloc symbols config_data.S xen.lds efi.lds
> +LINK_EXCLUDES += '*.map' xen xen.gz xen.efi xen-syms check.efi
> +
> +# To exclude full subtrees or individual files of not sufficiently specific
> +# names, regular expressions are used:
> +LINK_EXCLUDE_PATHS := xen/include/compat/.*
> +LINK_EXCLUDE_PATHS += xen/include/config/.*
> +LINK_EXCLUDE_PATHS += xen/include/generated/.*
> +LINK_EXCLUDE_PATHS += xen/arch/x86/boot/reloc[.]S
> +LINK_EXCLUDE_PATHS += xen/arch/x86/boot/cmdline[.]S
>  
>  # This is all a giant mess and doesn't really work.
>  #
> @@ -32,9 +42,10 @@ LINK_EXCLUDES += '.*.tmp'
>  # support easy development of the shim, but has a side effect of clobbering
>  # the already-built shim.
>  #
> -# $(LINK_EXCLUDES) should be set such that a parallel build of shim and xen/
> -# doesn't cause a subsequent `make install` to decide to regenerate the
> -# linkfarm.  This means that all final build artefacts must be excluded.
> +# $(LINK_EXCLUDES) and $(LINK_EXCLUDE_DIRS) should be set such that a parallel
> +# build of shim and xen/ doesn't cause a subsequent `make install` to decide to
> +# to regenerate the linkfarm.  This means that all intermediate and final build
     ^ duplicated 'to'

Thanks, Roger.

[PATCH 2/3] firmware/shim: drop XEN_CONFIG_EXPERT uses

[Jan Beulich]

As of commit d155e4aef35c ("xen: Allow EXPERT mode to be selected from
the menuconfig directly") EXPERT is a regular config option.

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

--- a/tools/firmware/xen-dir/Makefile
+++ b/tools/firmware/xen-dir/Makefile
@@ -74,10 +74,10 @@ $(D): linkfarm.stamp
 	$(MAKE) -C $(D)/xen distclean
 
 $(D)/xen/.config: $(D)
-	$(MAKE) -C $(@D) KBUILD_DEFCONFIG=pvshim_defconfig XEN_CONFIG_EXPERT=y defconfig
+	$(MAKE) -C $(@D) KBUILD_DEFCONFIG=pvshim_defconfig defconfig
 
 xen-shim: $(D)/xen/.config
-	$(MAKE) -C $(<D) build XEN_CONFIG_EXPERT=y
+	$(MAKE) -C $(<D) build
 	ln -sf $(D)/xen/xen $@
 	ln -sf $(D)/xen/xen-syms $@-syms
 

Re: [PATCH 2/3] firmware/shim: drop XEN_CONFIG_EXPERT uses

[Roger Pau Monné]

On Fri, Apr 30, 2021 at 04:44:21PM +0200, Jan Beulich wrote:
> As of commit d155e4aef35c ("xen: Allow EXPERT mode to be selected from
> the menuconfig directly") EXPERT is a regular config option.

Might be worth mentioning that the default pvshim config already have
EXPERT selected.

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

Acked-by: Roger Pau Monné <rogerpau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/3] firmware/shim: drop XEN_CONFIG_EXPERT uses

[Jan Beulich]

On 25.05.2021 16:14, Roger Pau Monné wrote:
> On Fri, Apr 30, 2021 at 04:44:21PM +0200, Jan Beulich wrote:
>> As of commit d155e4aef35c ("xen: Allow EXPERT mode to be selected from
>> the menuconfig directly") EXPERT is a regular config option.
> 
> Might be worth mentioning that the default pvshim config already have
> EXPERT selected.

That's not really related, as what the patch is removing is stale
regardless of config, but I've added a remark.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <rogerpau@citrix.com>

Thanks.

Jan

[PATCH 3/3] firmware/shim: UNSUPPORTED=n

[Jan Beulich]

We shouldn't default to include any unsupported code in the shim. Mark
the setting as off, replacing the ARGO specification. This points out
anomalies with the scheduler configuration: Unsupported schedulers
better don't default to Y in release builds (like is already the case
for ARINC653). Without these adjustments, the shim would suddenly build
with RTDS as its default scheduler.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
----
I'm certainly open to consider alterations on the sched/Kconfig
adjustments, but _something_ needs to be done there. In particular I'm
puzzled to find the NULL scheduler marked unsupported. Clearly with
the shim defaulting to it, it must be supported at least there.

--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -15,7 +15,7 @@ CONFIG_SCHED_NULL=y
 # CONFIG_KEXEC is not set
 # CONFIG_XENOPROF is not set
 # CONFIG_XSM is not set
-# CONFIG_ARGO is not set
+# CONFIG_UNSUPPORTED is not set
 # CONFIG_SCHED_CREDIT is not set
 # CONFIG_SCHED_CREDIT2 is not set
 # CONFIG_SCHED_RTDS is not set
--- a/xen/common/sched/Kconfig
+++ b/xen/common/sched/Kconfig
@@ -16,7 +16,7 @@ config SCHED_CREDIT2
 
 config SCHED_RTDS
 	bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
-	default y
+	default DEBUG
 	---help---
 	  The RTDS scheduler is a soft and firm real-time scheduler for
 	  multicore, targeted for embedded, automotive, graphics and gaming
@@ -31,7 +31,7 @@ config SCHED_ARINC653
 
 config SCHED_NULL
 	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
-	default y
+	default PV_SHIM || DEBUG
 	---help---
 	  The null scheduler is a static, zero overhead scheduler,
 	  for when there always are less vCPUs than pCPUs, typically

Re: [PATCH 3/3] firmware/shim: UNSUPPORTED=n

[Roger Pau Monné]

On Fri, Apr 30, 2021 at 04:45:03PM +0200, Jan Beulich wrote:
> We shouldn't default to include any unsupported code in the shim. Mark
> the setting as off, replacing the ARGO specification. This points out
> anomalies with the scheduler configuration: Unsupported schedulers
> better don't default to Y in release builds (like is already the case
> for ARINC653). Without these adjustments, the shim would suddenly build
> with RTDS as its default scheduler.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ----
> I'm certainly open to consider alterations on the sched/Kconfig
> adjustments, but _something_ needs to be done there. In particular I'm
> puzzled to find the NULL scheduler marked unsupported. Clearly with
> the shim defaulting to it, it must be supported at least there.

Indeed, I think we should mark NULL as supported for the shim usage
(which is very specific anyway, because it manages a single domain).

> --- a/xen/arch/x86/configs/pvshim_defconfig
> +++ b/xen/arch/x86/configs/pvshim_defconfig
> @@ -15,7 +15,7 @@ CONFIG_SCHED_NULL=y
>  # CONFIG_KEXEC is not set
>  # CONFIG_XENOPROF is not set
>  # CONFIG_XSM is not set
> -# CONFIG_ARGO is not set
> +# CONFIG_UNSUPPORTED is not set
>  # CONFIG_SCHED_CREDIT is not set
>  # CONFIG_SCHED_CREDIT2 is not set
>  # CONFIG_SCHED_RTDS is not set
> --- a/xen/common/sched/Kconfig
> +++ b/xen/common/sched/Kconfig
> @@ -16,7 +16,7 @@ config SCHED_CREDIT2
>  
>  config SCHED_RTDS
>  	bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
> -	default y
> +	default DEBUG

I would also be fine with leaving the default as 'n' for unsupported
features.

>  	---help---
>  	  The RTDS scheduler is a soft and firm real-time scheduler for
>  	  multicore, targeted for embedded, automotive, graphics and gaming
> @@ -31,7 +31,7 @@ config SCHED_ARINC653
>  
>  config SCHED_NULL
>  	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
> -	default y
> +	default PV_SHIM || DEBUG

Do we need the pvshim_defconfig to set CONFIG_SCHED_NULL=y after this?

Thanks, Roger.

Re: [PATCH 3/3] firmware/shim: UNSUPPORTED=n

[Jan Beulich]

On 25.05.2021 16:39, Roger Pau Monné wrote:
> On Fri, Apr 30, 2021 at 04:45:03PM +0200, Jan Beulich wrote:
>> We shouldn't default to include any unsupported code in the shim. Mark
>> the setting as off, replacing the ARGO specification. This points out
>> anomalies with the scheduler configuration: Unsupported schedulers
>> better don't default to Y in release builds (like is already the case
>> for ARINC653). Without these adjustments, the shim would suddenly build
>> with RTDS as its default scheduler.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ----
>> I'm certainly open to consider alterations on the sched/Kconfig
>> adjustments, but _something_ needs to be done there. In particular I'm
>> puzzled to find the NULL scheduler marked unsupported. Clearly with
>> the shim defaulting to it, it must be supported at least there.
> 
> Indeed, I think we should mark NULL as supported for the shim usage
> (which is very specific anyway, because it manages a single domain).

George, Dario, what is your position towards null's support status?

>> --- a/xen/common/sched/Kconfig
>> +++ b/xen/common/sched/Kconfig
>> @@ -16,7 +16,7 @@ config SCHED_CREDIT2
>>  
>>  config SCHED_RTDS
>>  	bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
>> -	default y
>> +	default DEBUG
> 
> I would also be fine with leaving the default as 'n' for unsupported
> features.

So would I be; I merely didn't want to make too big of step by
going straight from y to n. George, Dario - you're the maintainers
of this code (and I'd need your ack anyway), do you have any
preference?

>> @@ -31,7 +31,7 @@ config SCHED_ARINC653
>>  
>>  config SCHED_NULL
>>  	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
>> -	default y
>> +	default PV_SHIM || DEBUG
> 
> Do we need the pvshim_defconfig to set CONFIG_SCHED_NULL=y after this?

I don't think so, the default will be y for it. Explicit settings
are needed only when we want a non-default value.

Jan

Re: [PATCH 3/3] firmware/shim: UNSUPPORTED=n

[Roger Pau Monné]

On Tue, May 25, 2021 at 05:21:43PM +0200, Jan Beulich wrote:
> On 25.05.2021 16:39, Roger Pau Monné wrote:
> > On Fri, Apr 30, 2021 at 04:45:03PM +0200, Jan Beulich wrote:
> >> @@ -31,7 +31,7 @@ config SCHED_ARINC653
> >>  
> >>  config SCHED_NULL
> >>  	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
> >> -	default y
> >> +	default PV_SHIM || DEBUG
> > 
> > Do we need the pvshim_defconfig to set CONFIG_SCHED_NULL=y after this?
> 
> I don't think so, the default will be y for it. Explicit settings
> are needed only when we want a non-default value.

Right, I think I haven't expressed myself correctly. I wanted to point
out that I think CONFIG_SCHED_NULL=y is no longer needed in the
pvshim_defconfig.

Thanks, Roger.

Re: [PATCH 3/3] firmware/shim: UNSUPPORTED=n

[Jan Beulich]

On 25.05.2021 17:51, Roger Pau Monné wrote:
> On Tue, May 25, 2021 at 05:21:43PM +0200, Jan Beulich wrote:
>> On 25.05.2021 16:39, Roger Pau Monné wrote:
>>> On Fri, Apr 30, 2021 at 04:45:03PM +0200, Jan Beulich wrote:
>>>> @@ -31,7 +31,7 @@ config SCHED_ARINC653
>>>>  
>>>>  config SCHED_NULL
>>>>  	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
>>>> -	default y
>>>> +	default PV_SHIM || DEBUG
>>>
>>> Do we need the pvshim_defconfig to set CONFIG_SCHED_NULL=y after this?
>>
>> I don't think so, the default will be y for it. Explicit settings
>> are needed only when we want a non-default value.
> 
> Right, I think I haven't expressed myself correctly. I wanted to point
> out that I think CONFIG_SCHED_NULL=y is no longer needed in the
> pvshim_defconfig.

Oh, I see - yes, that ought to work. I'll make such an adjustment for
v2 (unless I discover something standing in the way).

Jan

Ping: [PATCH 0/3] firmware/shim: build adjustments

[Jan Beulich]

On 30.04.2021 16:39, Jan Beulich wrote:
> Originally I meant to finally get v2 of "firmware/shim: honor symlinks
> during Xen tree setup" sorted. However, the suggestion to use find's
> -L option, while a suitable equivalent of the -xtype primary, has the
> same drawback: It doesn't distinguish between relative and absolute
> symlinks (and we specifically want to skip relative ones). Locally I'm
> using '(' -type f -o -lname '/*' ')' now, but -lname again being non-
> standard I didn't think it would even be worth submitting. While
> looking into that I did notice a few other anomalies, though, which
> this series tries to address.
> 
> I notice tools/firmware/xen-dir/ isn't included in "X86 ARCHITECTURE".
> I wonder whether that should be added.
> 
> 1: update linkfarm exclusions
> 2: drop XEN_CONFIG_EXPERT uses
> 3: UNSUPPORTED=n

Ping?

Jan

[PATCH v2] firmware/shim: UNSUPPORTED=n

[Jan Beulich]

We shouldn't default to include any unsupported code in the shim. Mark
the setting as off, replacing the ARGO specification. This points out
anomalies with the scheduler configuration: Unsupported schedulers
better don't default to Y in release builds (like is already the case
for ARINC653). Without at least the SCHED_NULL adjustments, the shim
would suddenly build with RTDS as its default scheduler.

As a result, the SCHED_NULL setting can also be dropped from defconfig.

Clearly with the shim defaulting to it, SCHED_NULL must be supported at
least there.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
v2: Also drop SCHED_NULL setting from defconfig. Make SCHED_NULL the
    default when PV_SHIM_EXCLUSIVE.
---
I'm certainly open to consider alterations on the sched/Kconfig
adjustments, but _something_ needs to be done there. In particular I
was puzzled to find the NULL scheduler marked unsupported. Clearly with
the shim defaulting to it, it must be supported at least there.

In a PV_SHIM (but perhaps !PV_SHIM_EXCLUSIVE) build with the build-time
default not being SCHED_NULL, when actually running as shim I can't seem
to see how the null scheduler would get chosen as the default
nevertheless. Shouldn't this happen (in the absence of a command line
override)?

--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -351,9 +351,10 @@ Currently only single-vcpu domains are s
 A very simple, very static scheduling policy
 that always schedules the same vCPU(s) on the same pCPU(s).
 It is designed for maximum determinism and minimum overhead
-on embedded platforms.
+on embedded platforms and the x86 PV shim.
 
     Status: Experimental
+    Status, x86/shim: Supported
 
 ### NUMA scheduler affinity
 
--- a/xen/arch/x86/configs/pvshim_defconfig
+++ b/xen/arch/x86/configs/pvshim_defconfig
@@ -6,7 +6,6 @@ CONFIG_PV_SHIM=y
 CONFIG_PV_SHIM_EXCLUSIVE=y
 CONFIG_NR_CPUS=32
 CONFIG_EXPERT=y
-CONFIG_SCHED_NULL=y
 # Disable features not used by the PV shim
 # CONFIG_XEN_SHSTK is not set
 # CONFIG_GRANT_TABLE is not set
@@ -15,7 +14,7 @@ CONFIG_SCHED_NULL=y
 # CONFIG_KEXEC is not set
 # CONFIG_XENOPROF is not set
 # CONFIG_XSM is not set
-# CONFIG_ARGO is not set
+# CONFIG_UNSUPPORTED is not set
 # CONFIG_SCHED_CREDIT is not set
 # CONFIG_SCHED_CREDIT2 is not set
 # CONFIG_SCHED_RTDS is not set
--- a/xen/common/sched/Kconfig
+++ b/xen/common/sched/Kconfig
@@ -16,7 +16,7 @@ config SCHED_CREDIT2
 
 config SCHED_RTDS
 	bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
-	default y
+	default DEBUG
 	---help---
 	  The RTDS scheduler is a soft and firm real-time scheduler for
 	  multicore, targeted for embedded, automotive, graphics and gaming
@@ -31,7 +31,7 @@ config SCHED_ARINC653
 
 config SCHED_NULL
 	bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
-	default y
+	default PV_SHIM || DEBUG
 	---help---
 	  The null scheduler is a static, zero overhead scheduler,
 	  for when there always are less vCPUs than pCPUs, typically
@@ -39,6 +39,7 @@ config SCHED_NULL
 
 choice
 	prompt "Default Scheduler?"
+	default SCHED_NULL_DEFAULT if PV_SHIM_EXCLUSIVE
 	default SCHED_CREDIT2_DEFAULT
 
 	config SCHED_CREDIT_DEFAULT

Re: [PATCH v2] firmware/shim: UNSUPPORTED=n

[Roger Pau Monné]

On Wed, May 26, 2021 at 09:37:37AM +0200, Jan Beulich wrote:
> We shouldn't default to include any unsupported code in the shim. Mark
> the setting as off, replacing the ARGO specification. This points out
> anomalies with the scheduler configuration: Unsupported schedulers
> better don't default to Y in release builds (like is already the case
> for ARINC653). Without at least the SCHED_NULL adjustments, the shim
> would suddenly build with RTDS as its default scheduler.
> 
> As a result, the SCHED_NULL setting can also be dropped from defconfig.
> 
> Clearly with the shim defaulting to it, SCHED_NULL must be supported at
> least there.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> v2: Also drop SCHED_NULL setting from defconfig. Make SCHED_NULL the
>     default when PV_SHIM_EXCLUSIVE.
> ---
> I'm certainly open to consider alterations on the sched/Kconfig
> adjustments, but _something_ needs to be done there. In particular I
> was puzzled to find the NULL scheduler marked unsupported. Clearly with
> the shim defaulting to it, it must be supported at least there.
> 
> In a PV_SHIM (but perhaps !PV_SHIM_EXCLUSIVE) build with the build-time
> default not being SCHED_NULL, when actually running as shim I can't seem
> to see how the null scheduler would get chosen as the default
> nevertheless. Shouldn't this happen (in the absence of a command line
> override)?

I wondered the same yesterday and got to the conclusion that it's
then the duty of the user to add pvshim_extra="sched=null" to the
config file.

I think it would indeed be useful to select the null scheduler at
runtime if booting in shim mode and null is present, but then we would
be ignoring the default scheduler selection made in Kconfig, which
could be confusing.

It's also confusing that the scheduler that gets set as the default
when shim exclusive is selected is not available to enable/disable:

[*] Credit scheduler support (NEW)
[*] Credit2 scheduler support (NEW)
    Default Scheduler? (Null Scheduler)  --->

Thanks, Roger.

Re: [PATCH v2] firmware/shim: UNSUPPORTED=n

[Jan Beulich]

On 26.05.2021 10:27, Roger Pau Monné wrote:> It's also confusing that the scheduler that gets set as the default> when shim exclusive is selected is not available to enable/disable:> > [*] Credit scheduler support (NEW)> [*] Credit2 scheduler support (NEW)>     Default Scheduler? (Null Scheduler)  --->
I don't view this as confusing - we want the null scheduler there in
this case, unconditionally. But yes, the prompt's condition could of
course also have "PV_SHIM || " added.

Jan

Re: [PATCH v2] firmware/shim: UNSUPPORTED=n

[Dario Faggioli]

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

On Wed, 2021-05-26 at 09:37 +0200, Jan Beulich wrote:
> We shouldn't default to include any unsupported code in the shim. Mark
> the setting as off, replacing the ARGO specification. This points out
> anomalies with the scheduler configuration: Unsupported schedulers
> better don't default to Y in release builds (like is already the case
> for ARINC653). Without at least the SCHED_NULL adjustments, the shim
> would suddenly build with RTDS as its default scheduler.
> 
> As a result, the SCHED_NULL setting can also be dropped from defconfig.
> 
> Clearly with the shim defaulting to it, SCHED_NULL must be supported at
> least there.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

> ---
> v2: Also drop SCHED_NULL setting from defconfig. Make SCHED_NULL the
>     default when PV_SHIM_EXCLUSIVE.
> ---
> I'm certainly open to consider alterations on the sched/Kconfig
> adjustments, but _something_ needs to be done there. In particular I
> was puzzled to find the NULL scheduler marked unsupported. Clearly with
> the shim defaulting to it, it must be supported at least there.
> 
I think null both should and can be supported. There's an outstanding
bug [1], which we may want to finally fix before declaring it as such.
More important IMO, we should add an OSSTest test for it.

The latter may be tricky, as the hypervisor configuration of such test
needs to be host specific. In fact, we need to know how many CPUs we
have on the host, and configure Xen to give only a subset of them to
dom0 (or offline a few, after boot), as well as avoid running on 1 (and
problably also 2) CPUs hosts... or we won't be able to start guests
and/or do local migration jobs.

I can try to put something together, but I don't currently have an
OSSTest development environment up and running any longer, so it may
take a couple of iterations...

[1]
https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg01634.html

> --- a/xen/common/sched/Kconfig
> +++ b/xen/common/sched/Kconfig
> @@ -16,7 +16,7 @@ config SCHED_CREDIT2
>  
>  config SCHED_RTDS
>         bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
> -       default y
> +       default DEBUG
>         ---help---
>           The RTDS scheduler is a soft and firm real-time scheduler for
>           multicore, targeted for embedded, automotive, graphics and
> gaming
>
This is fine for me, for now.

That being said, if remember correctly, it should have all the features
that we said we wanted to it to have for declaring supported... Is
anyone of the embedded/safety/automotive/edge users and downstreams
interested interested on helping making RTDS first class citizen? And,
if yes, what's the path toward that?

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] firmware/shim: UNSUPPORTED=n

[Ian Jackson]

Dario Faggioli writes ("Re: [PATCH v2] firmware/shim: UNSUPPORTED=n"):
> I can try to put something together, but I don't currently have an
> OSSTest development environment up and running any longer, so it may
> take a couple of iterations...

(bit late with this but)

You could use your account on the Xen Project colo.  Let me know if
you need pointers or anything resetting.

Regards,
Ian.