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

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

[Jan Beulich]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -175,7 +175,7 @@ long cf_check core_parking_helper(void *
     unsigned int cpu;
     int ret = 0;
 
-    if ( !core_parking_policy )
+    if ( !core_parking_policy || CONFIG_NR_CPUS == 1 )
         return -EINVAL;
 
     while ( cur_idle_nums < idle_nums )
@@ -213,8 +213,9 @@ long cf_check core_parking_helper(void *
 
 bool core_parking_remove(unsigned int cpu)
 {
-    unsigned int i;
     bool found = false;
+#if CONFIG_NR_CPUS > 1
+    unsigned int i;
 
     spin_lock(&accounting_lock);
 
@@ -230,6 +231,7 @@ bool core_parking_remove(unsigned int cp
         core_parking_cpunum[i] = core_parking_cpunum[i + 1];
 
     spin_unlock(&accounting_lock);
+#endif /* CONFIG_NR_CPUS > 1 */
 
     return found;
 }
@@ -260,9 +262,11 @@ static int __init register_core_parking_
 
 static int __init cf_check core_parking_init(void)
 {
-    int ret = 0;
+    int ret;
 
-    if ( core_parking_controller == PERFORMANCE_FIRST )
+    if ( CONFIG_NR_CPUS == 1 )
+        ret = 0;
+    else if ( core_parking_controller == PERFORMANCE_FIRST )
         ret = register_core_parking_policy(&performance_first);
     else
         ret = register_core_parking_policy(&power_first);

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

[Julien Grall]

Hi Jan,

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?

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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

Jan

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

[Julien Grall]

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.

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

      case XENPF_resource_op:
      {

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

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