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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2022 14:27, Julien Grall wrote:
> Hi,
> 
> On 09/09/2022 13:14, Jan Beulich wrote:
>> On 09.09.2022 13:00, Julien Grall wrote:
>>> On 09/09/2022 11:18, 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).
>>>>
>>>> Beyond addressing the immediate issue also adjust core_parking_init():
>>>> There's no point registering any policy when there's no CPU to park.
>>>> Since this still doesn't result in the compiler spotting that
>>>> core_parking_policy is never written (and hence is continuously NULL),
>>>> also amend core_parking_helper() to avoid eventual similar issues there
>>>> (minimizing generated code at the same time).
>>>
>>> Given that CONFIG_NR_CPUS is a build time option. Wouldn't it be better
>>> to set CONFIG_CORE_PARKING=n and provide dummy helper for any function
>>> that may be called unconditionally?
>>
>> That might be an option, yes; not sure whether that's really better. It's
>> likely a more intrusive change ...
> 
> I quickly try to implement it (see below) and the result is IHMO a lot 
> nicer and make clear the code is not necessary on uni-processor.

Hmm, we can do something like this, but ...

> This is only compile tested.
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 6a7825f4ba3c..f9a3daccdc92 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
> +       select CORE_PARKING if NR_CPUS > 1
>          select HAS_ALTERNATIVE
>          select HAS_COMPAT
>          select HAS_CPUFREQ
> diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
> index 41a3b6a0dadf..7baca00be182 100644
> --- a/xen/arch/x86/include/asm/smp.h
> +++ b/xen/arch/x86/include/asm/smp.h
> @@ -61,7 +61,15 @@ long cf_check cpu_up_helper(void *data);
>   long cf_check cpu_down_helper(void *data);
> 
>   long cf_check core_parking_helper(void *data);
> +
> +#ifdef CONFIG_CORE_PARKING
>   bool core_parking_remove(unsigned int cpu);
> +#else
> +static inline bool core_parking_remove(unsigned int cpu)
> +{
> +    return false;
> +}
> +#endif
>   uint32_t get_cur_idle_nums(void);
> 
>   /*
> diff --git a/xen/arch/x86/platform_hypercall.c 
> b/xen/arch/x86/platform_hypercall.c
> index a7341dc3d7d3..5d13fac41bd4 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -718,6 +718,7 @@ ret_t do_platform_op(
>                         op->u.mem_add.pxm);
>           break;
> 
> +#ifdef CONFIG_CORE_PARKING
>       case XENPF_core_parking:
>       {
>           uint32_t idle_nums;
> @@ -743,6 +744,7 @@ ret_t do_platform_op(
>           }
>       }
>       break;
> +#endif

... this needs doing differently to prevent the hypercall changing
behavior. Will send a v2 in a minute.

Jan