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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2023 20:43, Andrew Cooper wrote:
> 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".

I've switched to that; as said in the private discussion we had, I
simply didn't know of "imply"; I've never come across a use so far in
Linux. But ...

> 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.

... I agree that's what we want to use here and likely a several other
places.

>> --- 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.

I disagree. There's no reason to report failure when we can fulfill a
request. Note also that "set" doesn't unconditionally fail either the
way I've coded it. Both are so people won't have to special case
single-CPU environment higher up the call stack.

>> --- 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.

Well, I've replaced it then. Will send a v3 in due course.

Jan