Mailing List Archive: [PATCH 3/3] firmware/shim: UNSUPPORTED=n

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

Apr 30, 2021, 7:45 AM

Post #1 of 5 (15 views)

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 [ In reply to ]

roger.pau at citrix

May 25, 2021, 7:39 AM

Post #2 of 5 (11 views)

Permalink

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 [ In reply to ]

jbeulich at suse

May 25, 2021, 8:21 AM

Post #3 of 5 (11 views)

Permalink

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 [ In reply to ]

roger.pau at citrix

May 25, 2021, 8:51 AM

Post #4 of 5 (11 views)

Permalink

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 [ In reply to ]

jbeulich at suse

May 25, 2021, 8:54 AM

Post #5 of 5 (11 views)

Permalink

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