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@suse.com>
---
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) )
{
case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
plug = true;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@suse.com> > --- > 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". 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. > 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; 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. > 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) ) 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. ~Andrew
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@suse.com> >> --- >> 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
On 27/02/2023 7:43 pm, Andrew Cooper wrote: > On 09/09/2022 3:30 pm, Jan Beulich wrote: >> 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; > 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. Huh, and in extra fun. It turns out we've had core parking unconditionally disabled in XenServer for ~9 years now. It looks at the idleness of dom0 and starts taking CPUs offline, even when the VMs are busy. I don't see how this functionality can ever have worked suitably... I think there's a good argument to be made for having it user selectable, and frankly, off-by-default. ~Andrew
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@suse.com> > --- > 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
On 22.10.2022 17:30, Julien Grall wrote: > Is this intended for 4.17? Well, yes, it was meant to be - it has been ... > On 09/09/2022 15:30, Jan Beulich wrote: ... well over a month since it was sent. >> --- 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. You're aware that we use this pattern in a few other places already (I guess in my local tree I have one or two which aren't upstream yet)? Just grep for "switch[^_].*[|]" to see them. Also note that it's not just the top bit of "op" - we merely assume "op" will never be ~0. Personally I prefer this way of coding the logic, but if at least one of the other x86 maintainers agreed with you, I'd be okay to switch to using a separate if(). Jan
Hi Jan, On 24/10/2022 08:26, Jan Beulich wrote: > On 22.10.2022 17:30, Julien Grall wrote: >> Is this intended for 4.17? > > Well, yes, it was meant to be - it has been ... > >> On 09/09/2022 15:30, Jan Beulich wrote: > > ... well over a month since it was sent. > >>> --- 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. > > You're aware that we use this pattern in a few other places already (I > guess in my local tree I have one or two which aren't upstream yet)? Just > grep for "switch[^_].*[|]" to see them. I could only spot two upstream in arch/x86/hvm/svm/svm.c and arch/x86/hvm/vmx/vmx.c. But I am not convinced this is a good enough reason to continue to use this approach. There are a few bad code examples in Xen and we have been pushing against continue to spread certain construct. > Also note that it's not just the > top bit of "op" - we merely assume "op" will never be ~0. Yes, I misread the code. > Personally I > prefer this way of coding the logic, but if at least one of the other x86 > maintainers agreed with you, I'd be okay to switch to using a separate > if(). I am curious why, is it just a matter of taste? If you really want to go down this route, then I think you should add "ASSERT(op != ~0U);" to catch someone introducing a value match that one we exclude. Cheers, -- Julien Grall
On 24.10.2022 12:03, Julien Grall wrote: > Hi Jan, > > On 24/10/2022 08:26, Jan Beulich wrote: >> On 22.10.2022 17:30, Julien Grall wrote: >>> Is this intended for 4.17? >> >> Well, yes, it was meant to be - it has been ... >> >>> On 09/09/2022 15:30, Jan Beulich wrote: >> >> ... well over a month since it was sent. >> >>>> --- 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. >> >> You're aware that we use this pattern in a few other places already (I >> guess in my local tree I have one or two which aren't upstream yet)? Just >> grep for "switch[^_].*[|]" to see them. > > I could only spot two upstream in arch/x86/hvm/svm/svm.c and > arch/x86/hvm/vmx/vmx.c. > > But I am not convinced this is a good enough reason to continue to use > this approach. There are a few bad code examples in Xen and we have been > pushing against continue to spread certain construct. Sure. But these were introduced consciously and deliberately, iirc. >> Also note that it's not just the >> top bit of "op" - we merely assume "op" will never be ~0. > Yes, I misread the code. > >> Personally I >> prefer this way of coding the logic, but if at least one of the other x86 >> maintainers agreed with you, I'd be okay to switch to using a separate >> if(). > > I am curious why, is it just a matter of taste? I think so. It's not written down anywhere that such constructs should not be used. > If you really want to go down this route, then I think you should add > "ASSERT(op != ~0U);" to catch someone introducing a value match that one > we exclude. No, such an assertion would be checking user input; a caller might deliberately pass this (invalid) value. Jan
© 2016 - 2024 Red Hat, Inc.