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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

Is this intended for 4.17?

On 09/09/2022 15:30, 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
>         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;
>               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) )
This code is quite confusing to read and potentially risky as you are 
are relying the top bit of 'op' to never be 1. While I am expecting this 
will ever be the case, this will be a "fun" issue to debug if this ever 
happen. So I would suggest to check CONFIG_NR_CPUS == 1 separately.
The rest of the changes looks fine to me.

Cheers,

--
Julien Grall