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);
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
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
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
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
© 2016 - 2024 Red Hat, Inc.