[PATCH 0/2][4.15?] fix build when NR_CPUS == 1

[PATCH 0/2][4.15?] fix build when NR_CPUS == 1

[Jan Beulich]

While we've long done away with CONFIG_SMP, we still allow
CONFIG_NR_CPUS to be set to 1. Hence at least randconfig builds
may fail, and the first of the two issues addressed was actually
observed in the RISC-V bring-up work. I didn't check whether Arm
would also have issues like these.

1: sched: fix build when NR_CPUS == 1
2: x86: fix build when NR_CPUS == 1

Jan

[PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1

[Jan Beulich]

In this case the compiler is recognizing that no valid array indexes
remain, and hence e.g. reports:

core.c: In function 'cpu_schedule_up':
core.c:2769:19: error: array subscript 1 is above array bounds
of 'struct vcpu *[1]' [-Werror=array-bounds]
 2769 |     if ( idle_vcpu[cpu] == NULL )
      |          ~~~~~~~~~^~~~~

Reported-by: Connor Davis <connojdavis@gmail.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2768,6 +2768,12 @@ static int cpu_schedule_up(unsigned int
     if ( cpu == 0 )
         return 0;
 
+    /*
+     * Guard in particular against the compiler suspecting out-of-bounds
+     * array accesses below when NR_CPUS=1.
+     */
+    BUG_ON(cpu >= NR_CPUS);
+
     if ( idle_vcpu[cpu] == NULL )
         vcpu_create(idle_vcpu[0]->domain, cpu);
     else

[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

In this case the compiler is recognizing that no valid array indexes
remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
...)), but oddly enough isn't really consistent about the checking it
does (see the code comment).

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

--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -54,7 +54,17 @@ static void init_apic_ldr_x2apic_cluster
     per_cpu(cluster_cpus, this_cpu) = cluster_cpus_spare;
     for_each_online_cpu ( cpu )
     {
-        if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
+        if ( this_cpu == cpu )
+            continue;
+        /*
+         * Guard in particular against the compiler suspecting out-of-bounds
+         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
+         * is the 1st of these alone which actually helps, not the 2nd, nor
+         * are both required together there).
+         */
+        BUG_ON(this_cpu >= NR_CPUS);
+        BUG_ON(cpu >= NR_CPUS);
+        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
             continue;
         per_cpu(cluster_cpus, this_cpu) = per_cpu(cluster_cpus, cpu);
         break;

Re: [PATCH 0/2][4.15?] fix build when NR_CPUS == 1

[Jan Beulich]

On 01.03.2021 09:27, Jan Beulich wrote:
> While we've long done away with CONFIG_SMP, we still allow
> CONFIG_NR_CPUS to be set to 1. Hence at least randconfig builds
> may fail, and the first of the two issues addressed was actually
> observed in the RISC-V bring-up work. I didn't check whether Arm
> would also have issues like these.
> 
> 1: sched: fix build when NR_CPUS == 1
> 2: x86: fix build when NR_CPUS == 1

I've tagged this with a question mark because on one hand such
configurations are unusual and hence unlikely to be overly
relevant for the release. Otoh randconfig failures would better
be avoided.

Jan

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Roger Pau Monné]

On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
> In this case the compiler is recognizing that no valid array indexes
> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> ...)), but oddly enough isn't really consistent about the checking it
> does (see the code comment).

I assume this is because of the underlying per_cpu access to
__per_cpu_offset using cpu as the index, in which case wouldn't it be
better to place the BUG_ON there?

Also I wonder why the accesses the same function does to the per_cpu
area before the modified chunk using this_cpu as index don't also
trigger such warnings.

Thanks, Roger.

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

On 01.03.2021 15:01, Roger Pau Monné wrote:
> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
>> In this case the compiler is recognizing that no valid array indexes
>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>> ...)), but oddly enough isn't really consistent about the checking it
>> does (see the code comment).
> 
> I assume this is because of the underlying per_cpu access to
> __per_cpu_offset using cpu as the index, in which case wouldn't it be
> better to place the BUG_ON there?

Not sure, to be honest. It seemed more logical to me to place it
next to where the issue is.

> Also I wonder why the accesses the same function does to the per_cpu
> area before the modified chunk using this_cpu as index don't also
> trigger such warnings.

The compiler appears to be issuing the warning when it can prove
that no legitimate index can make it to a respective use. in this
case it means that is is

        if ( this_cpu == cpu )
            continue;

which makes it possible for the compiler to know that what gets
past this would be an out of bounds access, since for NR_CPUS=1
both this_cpu and cpu can only validly both be zero. (This also
plays into my choice of placement, because it is not
x2apic_cluster() on its own which has this issue.)

Jan

Re: [PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1

[Ian Jackson]

Jan Beulich writes ("[PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1"):
> In this case the compiler is recognizing that no valid array indexes
> remain, and hence e.g. reports:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Ian Jackson]

Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> In this case the compiler is recognizing that no valid array indexes
> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> ...)), but oddly enough isn't really consistent about the checking it
> does (see the code comment).
...
> -        if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
> +        if ( this_cpu == cpu )
> +            continue;
> +        /*
> +         * Guard in particular against the compiler suspecting out-of-bounds
> +         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
> +         * is the 1st of these alone which actually helps, not the 2nd, nor
> +         * are both required together there).
> +         */
> +        BUG_ON(this_cpu >= NR_CPUS);
> +        BUG_ON(cpu >= NR_CPUS);
> +        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
>              continue;

Is there some particular reason for not putting the BUG_ON before the
if test ?  That would avoid the refactoring.

Of course putting it in next to the array indexing would address that
too.

Ian.

Re: [PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Mon, 2021-03-01 at 15:57 +0000, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 1/2][4.15?] sched: fix build when NR_CPUS
> == 1"):
> > In this case the compiler is recognizing that no valid array
> > indexes
> > remain, and hence e.g. reports:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Roger Pau Monné]

On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote:
> On 01.03.2021 15:01, Roger Pau Monné wrote:
> > On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
> >> In this case the compiler is recognizing that no valid array indexes
> >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> >> ...)), but oddly enough isn't really consistent about the checking it
> >> does (see the code comment).
> > 
> > I assume this is because of the underlying per_cpu access to
> > __per_cpu_offset using cpu as the index, in which case wouldn't it be
> > better to place the BUG_ON there?
> 
> Not sure, to be honest. It seemed more logical to me to place it
> next to where the issue is.

I'm not sure whether I would prefer to place it in per_cpu directly to
avoid similar issues popping up in other parts of the code in the
future?

Maybe that's not likely. TBH it seems kind of random to be placing
this BUG_ON conditionals to make the compilers happy, but maybe
there's no other option.

> > Also I wonder why the accesses the same function does to the per_cpu
> > area before the modified chunk using this_cpu as index don't also
> > trigger such warnings.
> 
> The compiler appears to be issuing the warning when it can prove
> that no legitimate index can make it to a respective use. in this
> case it means that is is
> 
>         if ( this_cpu == cpu )
>             continue;
> 
> which makes it possible for the compiler to know that what gets
> past this would be an out of bounds access, since for NR_CPUS=1
> both this_cpu and cpu can only validly both be zero. (This also
> plays into my choice of placement, because it is not
> x2apic_cluster() on its own which has this issue.)

I see, thanks for the explanation. It makes me wonder whether other
random issues like this will pop up in other parts of the code. We
should have a gitlab build with NR_CPUS=1 in order to assert we don't
regress it. Speaking for myself I certainly won't be able to detect
whether something broke this support in the future.

Thanks, Roger.

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

On 01.03.2021 19:00, Roger Pau Monné wrote:
> On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote:
>> On 01.03.2021 15:01, Roger Pau Monné wrote:
>>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
>>>> In this case the compiler is recognizing that no valid array indexes
>>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>>>> ...)), but oddly enough isn't really consistent about the checking it
>>>> does (see the code comment).
>>>
>>> I assume this is because of the underlying per_cpu access to
>>> __per_cpu_offset using cpu as the index, in which case wouldn't it be
>>> better to place the BUG_ON there?
>>
>> Not sure, to be honest. It seemed more logical to me to place it
>> next to where the issue is.
> 
> I'm not sure whether I would prefer to place it in per_cpu directly to
> avoid similar issues popping up in other parts of the code in the
> future?

That's going to be a lot of BUG_ON(), and hence a lot of "ud2"
instances. Not something I'm keen on having. The more that it's
not the per_cpu() which triggers the warning, but separate
conditionals allowing the compiler to deduce value ranges of
variables.

> Maybe that's not likely. TBH it seems kind of random to be placing
> this BUG_ON conditionals to make the compilers happy, but maybe
> there's no other option.

In principle I agree - hence the longish comment.

>>> Also I wonder why the accesses the same function does to the per_cpu
>>> area before the modified chunk using this_cpu as index don't also
>>> trigger such warnings.
>>
>> The compiler appears to be issuing the warning when it can prove
>> that no legitimate index can make it to a respective use. in this
>> case it means that is is
>>
>>         if ( this_cpu == cpu )
>>             continue;
>>
>> which makes it possible for the compiler to know that what gets
>> past this would be an out of bounds access, since for NR_CPUS=1
>> both this_cpu and cpu can only validly both be zero. (This also
>> plays into my choice of placement, because it is not
>> x2apic_cluster() on its own which has this issue.)
> 
> I see, thanks for the explanation. It makes me wonder whether other
> random issues like this will pop up in other parts of the code. We
> should have a gitlab build with NR_CPUS=1 in order to assert we don't
> regress it. Speaking for myself I certainly won't be able to detect
> whether something broke this support in the future.

I guess I'll carry on having such a build test locally.

Jan

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

On 01.03.2021 17:03, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
>> In this case the compiler is recognizing that no valid array indexes
>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>> ...)), but oddly enough isn't really consistent about the checking it
>> does (see the code comment).
> ...
>> -        if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
>> +        if ( this_cpu == cpu )
>> +            continue;
>> +        /*
>> +         * Guard in particular against the compiler suspecting out-of-bounds
>> +         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
>> +         * is the 1st of these alone which actually helps, not the 2nd, nor
>> +         * are both required together there).
>> +         */
>> +        BUG_ON(this_cpu >= NR_CPUS);
>> +        BUG_ON(cpu >= NR_CPUS);
>> +        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
>>              continue;
> 
> Is there some particular reason for not putting the BUG_ON before the
> if test ?  That would avoid the refactoring.

I want it to be as close as possible to the place where the issue
is. I also view the refactoring as a good thing, since it allows
a style correction as a side effect.

> Of course putting it in next to the array indexing would address that
> too.

See my earlier reply to Roger's similar remark (which still was
along the lines of what I've said above).

Jan

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Roger Pau Monné]

On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote:
> On 01.03.2021 19:00, Roger Pau Monné wrote:
> > On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote:
> >> On 01.03.2021 15:01, Roger Pau Monné wrote:
> >>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
> >>>> In this case the compiler is recognizing that no valid array indexes
> >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> >>>> ...)), but oddly enough isn't really consistent about the checking it
> >>>> does (see the code comment).
> >>>
> >>> I assume this is because of the underlying per_cpu access to
> >>> __per_cpu_offset using cpu as the index, in which case wouldn't it be
> >>> better to place the BUG_ON there?
> >>
> >> Not sure, to be honest. It seemed more logical to me to place it
> >> next to where the issue is.
> > 
> > I'm not sure whether I would prefer to place it in per_cpu directly to
> > avoid similar issues popping up in other parts of the code in the
> > future?
> 
> That's going to be a lot of BUG_ON(), and hence a lot of "ud2"
> instances. Not something I'm keen on having. The more that it's
> not the per_cpu() which triggers the warning, but separate
> conditionals allowing the compiler to deduce value ranges of
> variables.

Right. I don't see much other way around this then. Did you check with
clang also?

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

On 02.03.2021 11:57, Roger Pau Monné wrote:
> On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote:
>> On 01.03.2021 19:00, Roger Pau Monné wrote:
>>> On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote:
>>>> On 01.03.2021 15:01, Roger Pau Monné wrote:
>>>>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote:
>>>>>> In this case the compiler is recognizing that no valid array indexes
>>>>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>>>>>> ...)), but oddly enough isn't really consistent about the checking it
>>>>>> does (see the code comment).
>>>>>
>>>>> I assume this is because of the underlying per_cpu access to
>>>>> __per_cpu_offset using cpu as the index, in which case wouldn't it be
>>>>> better to place the BUG_ON there?
>>>>
>>>> Not sure, to be honest. It seemed more logical to me to place it
>>>> next to where the issue is.
>>>
>>> I'm not sure whether I would prefer to place it in per_cpu directly to
>>> avoid similar issues popping up in other parts of the code in the
>>> future?
>>
>> That's going to be a lot of BUG_ON(), and hence a lot of "ud2"
>> instances. Not something I'm keen on having. The more that it's
>> not the per_cpu() which triggers the warning, but separate
>> conditionals allowing the compiler to deduce value ranges of
>> variables.
> 
> Right. I don't see much other way around this then. Did you check with
> clang also?

No, I didn't. But if anything it would require further additions,
surely no less.

> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> On 01.03.2021 17:03, Ian Jackson wrote:
> > Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> >> In this case the compiler is recognizing that no valid array indexes
> >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> >> ...)), but oddly enough isn't really consistent about the checking it
> >> does (see the code comment).
> > ...
> >> -        if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
> >> +        if ( this_cpu == cpu )
> >> +            continue;
> >> +        /*
> >> +         * Guard in particular against the compiler suspecting out-of-bounds
> >> +         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
> >> +         * is the 1st of these alone which actually helps, not the 2nd, nor
> >> +         * are both required together there).
> >> +         */
> >> +        BUG_ON(this_cpu >= NR_CPUS);
> >> +        BUG_ON(cpu >= NR_CPUS);
> >> +        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
> >>              continue;
> > 
> > Is there some particular reason for not putting the BUG_ON before the
> > if test ?  That would avoid the refactoring.
> 
> I want it to be as close as possible to the place where the issue
> is. I also view the refactoring as a good thing, since it allows
> a style correction as a side effect.

I'm afraid that at this stage of the release I would prefer changes to
be as small as reasonably sensible.  So unless there is some
reason, other than taste, style or formatting, could we please just
introduce the new BUG_ON and not also do other refactoring.

Ian.

Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich]

On 02.03.2021 13:28, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
>> On 01.03.2021 17:03, Ian Jackson wrote:
>>> Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
>>>> In this case the compiler is recognizing that no valid array indexes
>>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>>>> ...)), but oddly enough isn't really consistent about the checking it
>>>> does (see the code comment).
>>> ...
>>>> -        if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
>>>> +        if ( this_cpu == cpu )
>>>> +            continue;
>>>> +        /*
>>>> +         * Guard in particular against the compiler suspecting out-of-bounds
>>>> +         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
>>>> +         * is the 1st of these alone which actually helps, not the 2nd, nor
>>>> +         * are both required together there).
>>>> +         */
>>>> +        BUG_ON(this_cpu >= NR_CPUS);
>>>> +        BUG_ON(cpu >= NR_CPUS);
>>>> +        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
>>>>              continue;
>>>
>>> Is there some particular reason for not putting the BUG_ON before the
>>> if test ?  That would avoid the refactoring.
>>
>> I want it to be as close as possible to the place where the issue
>> is. I also view the refactoring as a good thing, since it allows
>> a style correction as a side effect.
> 
> I'm afraid that at this stage of the release I would prefer changes to
> be as small as reasonably sensible.  So unless there is some
> reason, other than taste, style or formatting, could we please just
> introduce the new BUG_ON and not also do other refactoring.

FAOD: That's fine - I'll keep this queued for 4.16 then. I did put
a question mark behind the version anyway.

Jan