diff mbox series

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

Message ID 1d8d5089-32a9-9c42-5949-8f9cd20f33e0@suse.com (mailing list archive)
State New
Headers show
Series fix build when NR_CPUS == 1 | expand

Commit Message

Jan Beulich March 1, 2021, 8:31 a.m. UTC
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>

Comments

Roger Pau Monné March 1, 2021, 2:01 p.m. UTC | #1
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.
Jan Beulich March 1, 2021, 3:14 p.m. UTC | #2
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
Ian Jackson March 1, 2021, 4:03 p.m. UTC | #3
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.
Roger Pau Monné March 1, 2021, 6 p.m. UTC | #4
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.
Jan Beulich March 2, 2021, 9:59 a.m. UTC | #5
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
Jan Beulich March 2, 2021, 10:02 a.m. UTC | #6
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
Roger Pau Monné March 2, 2021, 10:57 a.m. UTC | #7
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.
Jan Beulich March 2, 2021, 11:18 a.m. UTC | #8
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
Ian Jackson March 2, 2021, 12:28 p.m. UTC | #9
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.
Jan Beulich March 2, 2021, 1:37 p.m. UTC | #10
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
diff mbox series

Patch

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