Re: [PATCH v2] core-parking: fix build with gcc12 and NR_CPUS=1

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/09/2022 3:30 pm, Jan Beulich wrote:
> Gcc12 takes issue with core_parking_remove()'s
>
>    for ( ; i < cur_idle_nums; ++i )
>        core_parking_cpunum[i] = core_parking_cpunum[i + 1];
>
> complaining that the right hand side array access is past the bounds of
> 1. Clearly the compiler can't know that cur_idle_nums can only ever be
> zero in this case (as the sole CPU cannot be parked).
>
> Arrange for core_parking.c's contents to not be needed altogether, and
> then disable its building when NR_CPUS == 1.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Disable building of core_parking.c altogether.
>
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -10,7 +10,7 @@ config X86
>     select ALTERNATIVE_CALL
>     select ARCH_MAP_DOMAIN_PAGE
>     select ARCH_SUPPORTS_INT128
> -    select CORE_PARKING
> +    select CORE_PARKING if NR_CPUS > 1

The appropriate change is:

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba3c..2a5c3304e2b0 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -10,7 +10,7 @@ config X86
        select ALTERNATIVE_CALL
        select ARCH_MAP_DOMAIN_PAGE
        select ARCH_SUPPORTS_INT128
-       select CORE_PARKING
+       imply CORE_PARKING
        select HAS_ALTERNATIVE
        select HAS_COMPAT
        select HAS_CPUFREQ
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f1ea3199c8eb..855c843113e3 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -10,6 +10,7 @@ config COMPAT
 
 config CORE_PARKING
        bool
+       depends on NR_CPUS > 1
 
 config GRANT_TABLE
        bool "Grant table support" if EXPERT


The core parking code really does malfunction with NR_CPUS == 1, so
really does need a hard dependency.

It turns out our version of Kbuild does understand the imply keyword,
which is the right way to spell "I want this feature unless something
else happens to rule it out".

As noted in the kbuild docs, select should only be used for immutable
properties (this arch has $X), whereas imply should be used for all "I
want $Y".  Most places we use select ought to use imply instead.



>     select HAS_ALTERNATIVE
>     select HAS_COMPAT
>     select HAS_CPUFREQ
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -727,12 +727,17 @@ ret_t do_platform_op(
>         case XEN_CORE_PARKING_SET:
>             idle_nums = min_t(uint32_t,
>                     op->u.core_parking.idle_nums, num_present_cpus() -
> 1);
> -            ret = continue_hypercall_on_cpu(
> -                    0, core_parking_helper, (void *)(unsigned
> long)idle_nums);
> +            if ( CONFIG_NR_CPUS > 1 )
> +                ret = continue_hypercall_on_cpu(
> +                        0, core_parking_helper,
> +                        (void *)(unsigned long)idle_nums);
> +            else if ( idle_nums )
> +                ret = -EINVAL;
>             break;
>
>         case XEN_CORE_PARKING_GET:
> -            op->u.core_parking.idle_nums = get_cur_idle_nums();
> +            op->u.core_parking.idle_nums = CONFIG_NR_CPUS > 1
> +                                           ? get_cur_idle_nums() : 0;

These don't look right.

If the core parking feature isn't available, it should uniformly fail,
not report success on the get side and fail on the set side.

>             ret = __copy_field_to_guest(u_xenpf_op, op, u.core_parking) ?
>                   -EFAULT : 0;
>             break;
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -157,7 +157,7 @@ long arch_do_sysctl(
>         long (*fn)(void *);
>         void *hcpu;
>
> -        switch ( op )
> +        switch ( op | -(CONFIG_NR_CPUS == 1) )

Extending what Julien has said...

We use this pattern exactly twice, and I would probably ack a patch
disallowing it in the coding style.

Its a trick that is too clever for its own good.  To anyone who hasn't
encountered it, its straight obfuscation, and even I, who knows how the
trick works, still has to reverse engineer the expression every single
time to figure out which way it ends up resolving.

~Andrew