[PATCH 0/3] Fix Power CI build

[PATCH 0/3] Fix Power CI build

[Shawn Anastasio]

Hello all,

This series contains a few minor fixes to the CI config and one
additional patch to common/ that resolve ppc64-specific CI failures
that Jan discovered.

Thanks,

Shawn Anastasio (3):
  automation: Drop ppc64le-*randconfig jobs
  automation: Change build script to use arch defconfig
  xen/common: Add NUMA node id bounds check to
    page_alloc.c/node_to_scrub

 automation/gitlab-ci/build.yaml | 18 ------------------
 automation/scripts/build        |  7 ++++++-
 xen/common/page_alloc.c         |  3 +++
 3 files changed, 9 insertions(+), 19 deletions(-)

--
2.30.2

[PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Shawn Anastasio]

Since ppc64le is still undergoing early bringup, disable the randconfig
CI build which was causing spurious CI failures.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Reported-by: Jan Beulich <jbeulich@suse.com>
---
 automation/gitlab-ci/build.yaml | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 1619e9a558..32af30cced 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
     KBUILD_DEFCONFIG: ppc64_defconfig
     HYPERVISOR_ONLY: y
 
-debian-bullseye-gcc-ppc64le-randconfig:
-  extends: .gcc-ppc64le-cross-build
-  variables:
-    CONTAINER: debian:bullseye-ppc64le
-    KBUILD_DEFCONFIG: ppc64_defconfig
-    RANDCONFIG: y
-    EXTRA_FIXED_RANDCONFIG:
-      CONFIG_COVERAGE=n
-
-debian-bullseye-gcc-ppc64le-debug-randconfig:
-  extends: .gcc-ppc64le-cross-build-debug
-  variables:
-    CONTAINER: debian:bullseye-ppc64le
-    KBUILD_DEFCONFIG: ppc64_defconfig
-    RANDCONFIG: y
-    EXTRA_FIXED_RANDCONFIG:
-      CONFIG_COVERAGE=n
-
 # Yocto test jobs
 yocto-qemuarm64:
   extends: .yocto-test-arm64
-- 
2.30.2

[PATCH 2/3] automation: Change build script to use arch defconfig

[Shawn Anastasio]

Change automation build script to call the make defconfig target before
setting CONFIG_DEBUG and extra options. This fixes issues on Power where
the build fails without using the ppc64_defconfig.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Reported-by: Jan Beulich <jbeulich@suse.com>
---
 automation/scripts/build | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/automation/scripts/build b/automation/scripts/build
index b4edcf010e..19dd9e8270 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
     # RANDCONFIG implies HYPERVISOR_ONLY
     HYPERVISOR_ONLY="y"
 else
-    echo "CONFIG_DEBUG=${debug}" > xen/.config
+    # Start off with arch's defconfig
+    make -C xen defconfig
+
+    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
+    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
+    echo "CONFIG_DEBUG=${debug}" >> xen/.config

     if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
         echo "${EXTRA_XEN_CONFIG}" >> xen/.config
--
2.30.2

[PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Shawn Anastasio]

When building for Power with CONFIG_DEBUG unset, a compiler error gets
raised inside page_alloc.c's node_to_scrub function, likely due to the
increased optimization level:

common/page_alloc.c: In function 'node_to_scrub.part.0':
common/page_alloc.c:1217:29: error: array subscript 1 is above array
            bounds of 'long unsigned int[1]' [-Werror=array-bounds]
 1217 |         if ( node_need_scrub[node] )

It appears that this is a false positive, given that in practice
cycle_node should never return a node ID >= MAX_NUMNODES as long as the
architecture's node_online_map is properly defined and initialized, so
this additional bounds check is only to satisfy GCC.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 xen/common/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 35d9a26fa6..6df2a223e1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
         } while ( !cpumask_empty(&node_to_cpumask(node)) &&
                   (node != local_node) );

+        if ( node >= MAX_NUMNODES )
+            break;
+
         if ( node == local_node )
             break;

--
2.30.2

Re: [PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Stefano Stabellini]

On Mon, 25 Sep 2023, Shawn Anastasio wrote:
> Since ppc64le is still undergoing early bringup, disable the randconfig
> CI build which was causing spurious CI failures.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> Reported-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  automation/gitlab-ci/build.yaml | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 1619e9a558..32af30cced 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
>      KBUILD_DEFCONFIG: ppc64_defconfig
>      HYPERVISOR_ONLY: y
>  
> -debian-bullseye-gcc-ppc64le-randconfig:
> -  extends: .gcc-ppc64le-cross-build
> -  variables:
> -    CONTAINER: debian:bullseye-ppc64le
> -    KBUILD_DEFCONFIG: ppc64_defconfig
> -    RANDCONFIG: y
> -    EXTRA_FIXED_RANDCONFIG:
> -      CONFIG_COVERAGE=n
> -
> -debian-bullseye-gcc-ppc64le-debug-randconfig:
> -  extends: .gcc-ppc64le-cross-build-debug
> -  variables:
> -    CONTAINER: debian:bullseye-ppc64le
> -    KBUILD_DEFCONFIG: ppc64_defconfig
> -    RANDCONFIG: y
> -    EXTRA_FIXED_RANDCONFIG:
> -      CONFIG_COVERAGE=n
> -
>  # Yocto test jobs
>  yocto-qemuarm64:
>    extends: .yocto-test-arm64
> -- 
> 2.30.2
> 

Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Stefano Stabellini]

Adding Henry as this issue is currently causing gitlab-ci failures. I
think we should fix it as soon as possible to get clearer results for
the 4.18 release. (This comment applies to the rest of the series as
well).


On Mon, 25 Sep 2023, Shawn Anastasio wrote:
> When building for Power with CONFIG_DEBUG unset, a compiler error gets
> raised inside page_alloc.c's node_to_scrub function, likely due to the
> increased optimization level:
> 
> common/page_alloc.c: In function 'node_to_scrub.part.0':
> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>  1217 |         if ( node_need_scrub[node] )
> 
> It appears that this is a false positive, given that in practice
> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
> architecture's node_online_map is properly defined and initialized, so
> this additional bounds check is only to satisfy GCC.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/common/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 35d9a26fa6..6df2a223e1 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>                    (node != local_node) );
> 
> +        if ( node >= MAX_NUMNODES )
> +            break;
> +
>          if ( node == local_node )
>              break;
> 
> --
> 2.30.2
> 

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Stefano Stabellini]

On Mon, 25 Sep 2023, Shawn Anastasio wrote:
> Change automation build script to call the make defconfig target before
> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
> the build fails without using the ppc64_defconfig.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> Reported-by: Jan Beulich <jbeulich@suse.com>

What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
before make olddefconfig causes certain DEBUG options also to default to
yes, and these additional options don't work well on Power?

If that is the case, wouldn't it be better to remove the -debug jobs
until they work well on Power? Or make them allow_failure: true ?


> ---
>  automation/scripts/build | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/scripts/build b/automation/scripts/build
> index b4edcf010e..19dd9e8270 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>      # RANDCONFIG implies HYPERVISOR_ONLY
>      HYPERVISOR_ONLY="y"
>  else
> -    echo "CONFIG_DEBUG=${debug}" > xen/.config
> +    # Start off with arch's defconfig
> +    make -C xen defconfig
> +
> +    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
> +    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
> +    echo "CONFIG_DEBUG=${debug}" >> xen/.config
> 
>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>          echo "${EXTRA_XEN_CONFIG}" >> xen/.config
> --
> 2.30.2
> 

Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Henry Wang]

Hi Stefano,

> On Sep 26, 2023, at 07:04, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Adding Henry as this issue is currently causing gitlab-ci failures. I
> think we should fix it as soon as possible to get clearer results for
> the 4.18 release. (This comment applies to the rest of the series as
> well).

Thanks, the CI should indeed be fixed asap.

> 
> 
> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>> When building for Power with CONFIG_DEBUG unset, a compiler error gets
>> raised inside page_alloc.c's node_to_scrub function, likely due to the
>> increased optimization level:
>> 
>> common/page_alloc.c: In function 'node_to_scrub.part.0':
>> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>>            bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>> 1217 |         if ( node_need_scrub[node] )
>> 
>> It appears that this is a false positive, given that in practice
>> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
>> architecture's node_online_map is properly defined and initialized, so
>> this additional bounds check is only to satisfy GCC.
>> 
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> 
>> ---
>> xen/common/page_alloc.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 35d9a26fa6..6df2a223e1 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>>         } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>>                   (node != local_node) );
>> 
>> +        if ( node >= MAX_NUMNODES )
>> +            break;
>> +
>>         if ( node == local_node )
>>             break;
>> 
>> --
>> 2.30.2
>> 

Re: [PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Henry Wang]

Hi,

> On Sep 26, 2023, at 06:57, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>> Since ppc64le is still undergoing early bringup, disable the randconfig
>> CI build which was causing spurious CI failures.
>> 
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> 
>> ---
>> automation/gitlab-ci/build.yaml | 18 ------------------
>> 1 file changed, 18 deletions(-)
>> 
>> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
>> index 1619e9a558..32af30cced 100644
>> --- a/automation/gitlab-ci/build.yaml
>> +++ b/automation/gitlab-ci/build.yaml
>> @@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
>>     KBUILD_DEFCONFIG: ppc64_defconfig
>>     HYPERVISOR_ONLY: y
>> 
>> -debian-bullseye-gcc-ppc64le-randconfig:
>> -  extends: .gcc-ppc64le-cross-build
>> -  variables:
>> -    CONTAINER: debian:bullseye-ppc64le
>> -    KBUILD_DEFCONFIG: ppc64_defconfig
>> -    RANDCONFIG: y
>> -    EXTRA_FIXED_RANDCONFIG:
>> -      CONFIG_COVERAGE=n
>> -
>> -debian-bullseye-gcc-ppc64le-debug-randconfig:
>> -  extends: .gcc-ppc64le-cross-build-debug
>> -  variables:
>> -    CONTAINER: debian:bullseye-ppc64le
>> -    KBUILD_DEFCONFIG: ppc64_defconfig
>> -    RANDCONFIG: y
>> -    EXTRA_FIXED_RANDCONFIG:
>> -      CONFIG_COVERAGE=n
>> -
>> # Yocto test jobs
>> yocto-qemuarm64:
>>   extends: .yocto-test-arm64
>> -- 
>> 2.30.2
>> 
> 

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Jan Beulich]

On 26.09.2023 01:12, Stefano Stabellini wrote:
> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>> Change automation build script to call the make defconfig target before
>> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
>> the build fails without using the ppc64_defconfig.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> Reported-by: Jan Beulich <jbeulich@suse.com>

Nit: Tags in chronological order please (also affects patch 1).

> What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
> before make olddefconfig causes certain DEBUG options also to default to
> yes, and these additional options don't work well on Power?

No, the issue is that "make olddefconfig" takes the existing .config
without even considering the arch's default configuration that was
specified (KBUILD_DEFCONFIG).

>> --- a/automation/scripts/build
>> +++ b/automation/scripts/build
>> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>>      # RANDCONFIG implies HYPERVISOR_ONLY
>>      HYPERVISOR_ONLY="y"
>>  else
>> -    echo "CONFIG_DEBUG=${debug}" > xen/.config
>> +    # Start off with arch's defconfig
>> +    make -C xen defconfig
>> +
>> +    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
>> +    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
>> +    echo "CONFIG_DEBUG=${debug}" >> xen/.config
>>
>>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>>          echo "${EXTRA_XEN_CONFIG}" >> xen/.config

It never really became clear to me whether kconfig honors the first,
last, or any random setting in a .config that it takes as input, when
a certain option appears there more than once. The change you make
implies it's consistently "last" - can you confirm that's the actual
behavior of kconfig?

Jan

Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich]

On 26.09.2023 00:42, Shawn Anastasio wrote:
> When building for Power with CONFIG_DEBUG unset, a compiler error gets
> raised inside page_alloc.c's node_to_scrub function, likely due to the
> increased optimization level:
> 
> common/page_alloc.c: In function 'node_to_scrub.part.0':
> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>  1217 |         if ( node_need_scrub[node] )

That's gcc13?

> It appears that this is a false positive, given that in practice
> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
> architecture's node_online_map is properly defined and initialized, so
> this additional bounds check is only to satisfy GCC.

Looks very similar to the situation that c890499871cc ("timer: fix
NR_CPUS=1 build with gcc13") was dealing with, just that here it's
MAX_NUMNODES. I'd therefore prefer a solution similar to the one
taken there, i.e. make code conditional rather than add yet more
code.

Otherwise, ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>                    (node != local_node) );
> 
> +        if ( node >= MAX_NUMNODES )
> +            break;

... this clearly redundant check would need to gain a comment.

What I'm puzzled by is that on Arm we had no reports of a similar
problem, despite NUMA also not getting selected there (yet).

Jan

Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich]

On 26.09.2023 09:32, Jan Beulich wrote:
> On 26.09.2023 00:42, Shawn Anastasio wrote:
>> When building for Power with CONFIG_DEBUG unset,

Hmm, depending on what gcc versions are used in CI, the above may be the
reason why ...

>> a compiler error gets
>> raised inside page_alloc.c's node_to_scrub function, likely due to the
>> increased optimization level:
>>
>> common/page_alloc.c: In function 'node_to_scrub.part.0':
>> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>>  1217 |         if ( node_need_scrub[node] )
> 
> That's gcc13?
> 
>> It appears that this is a false positive, given that in practice
>> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
>> architecture's node_online_map is properly defined and initialized, so
>> this additional bounds check is only to satisfy GCC.
> 
> Looks very similar to the situation that c890499871cc ("timer: fix
> NR_CPUS=1 build with gcc13") was dealing with, just that here it's
> MAX_NUMNODES. I'd therefore prefer a solution similar to the one
> taken there, i.e. make code conditional rather than add yet more
> code.
> 
> Otherwise, ...
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>>                    (node != local_node) );
>>
>> +        if ( node >= MAX_NUMNODES )
>> +            break;
> 
> ... this clearly redundant check would need to gain a comment.
> 
> What I'm puzzled by is that on Arm we had no reports of a similar
> problem, despite NUMA also not getting selected there (yet).

... this wasn't observed, yet. As far as I'm concerned, all my Arm builds
are debug ones (which I may need to change).

Jan

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Shawn Anastasio]

On 9/26/23 2:19 AM, Jan Beulich wrote:
> On 26.09.2023 01:12, Stefano Stabellini wrote:
>> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>>> Change automation build script to call the make defconfig target before
>>> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
>>> the build fails without using the ppc64_defconfig.
>>>
>>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>> Reported-by: Jan Beulich <jbeulich@suse.com>
> 
> Nit: Tags in chronological order please (also affects patch 1).
>
Will fix.

>> What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
>> before make olddefconfig causes certain DEBUG options also to default to
>> yes, and these additional options don't work well on Power?
> 
> No, the issue is that "make olddefconfig" takes the existing .config
> without even considering the arch's default configuration that was
> specified (KBUILD_DEFCONFIG).
> 
>>> --- a/automation/scripts/build
>>> +++ b/automation/scripts/build
>>> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>>>      # RANDCONFIG implies HYPERVISOR_ONLY
>>>      HYPERVISOR_ONLY="y"
>>>  else
>>> -    echo "CONFIG_DEBUG=${debug}" > xen/.config
>>> +    # Start off with arch's defconfig
>>> +    make -C xen defconfig
>>> +
>>> +    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
>>> +    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
>>> +    echo "CONFIG_DEBUG=${debug}" >> xen/.config
>>>
>>>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>>>          echo "${EXTRA_XEN_CONFIG}" >> xen/.config
> 
> It never really became clear to me whether kconfig honors the first,
> last, or any random setting in a .config that it takes as input, when
> a certain option appears there more than once. The change you make
> implies it's consistently "last" - can you confirm that's the actual
> behavior of kconfig?
>

I actually tried to avoid this issue alltogether with the sed command I
added before the echo to drop any existing CONFIG_DEBUG= line.

> Jan

Thanks,
Shawn

Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Shawn Anastasio]

On 9/26/23 2:32 AM, Jan Beulich wrote:
> On 26.09.2023 00:42, Shawn Anastasio wrote:
>> When building for Power with CONFIG_DEBUG unset, a compiler error gets
>> raised inside page_alloc.c's node_to_scrub function, likely due to the
>> increased optimization level:
>>
>> common/page_alloc.c: In function 'node_to_scrub.part.0':
>> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>>  1217 |         if ( node_need_scrub[node] )
> 
> That's gcc13?
> 

Gcc 10.2.1, actually.

>> It appears that this is a false positive, given that in practice
>> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
>> architecture's node_online_map is properly defined and initialized, so
>> this additional bounds check is only to satisfy GCC.
> 
> Looks very similar to the situation that c890499871cc ("timer: fix
> NR_CPUS=1 build with gcc13") was dealing with, just that here it's
> MAX_NUMNODES. I'd therefore prefer a solution similar to the one
> taken there, i.e. make code conditional rather than add yet more
> code.
> 

Looking at that commit, it doesn't look like conditionalizing this code
in the same way would be as clean -- it'd likely require separate
conditional blocks around the initial variable declarations and the
function body.

> Otherwise, ...
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>>                    (node != local_node) );
>>
>> +        if ( node >= MAX_NUMNODES )
>> +            break;
> 
> ... this clearly redundant check would need to gain a comment.
>

This isn't a problem. I'll submit a v2 with a commit.

> What I'm puzzled by is that on Arm we had no reports of a similar
> problem, despite NUMA also not getting selected there (yet).
>

As you pointed out in your follow-up email, it might be due to
CONFIG_DEBUG. It might also be because on ppc, our implementation of
find_next_bit (which is used by cycle_node) is declared static inline,
so the compiler has access to its definition when doing this sort of
static analysis. On Arm I believe the function lives in its own
compilation unit which might preclude GCC from making any assumptions
about its return value.

> Jan

Thanks,
Shawn

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Shawn Anastasio]

On 9/26/23 12:43 PM, Shawn Anastasio wrote:
> On 9/26/23 2:19 AM, Jan Beulich wrote:
>> On 26.09.2023 01:12, Stefano Stabellini wrote:
>>> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>>>> Change automation build script to call the make defconfig target before
>>>> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
>>>> the build fails without using the ppc64_defconfig.
>>>>
>>>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>
>> Nit: Tags in chronological order please (also affects patch 1).
>>
> Will fix.
> 
>>> What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
>>> before make olddefconfig causes certain DEBUG options also to default to
>>> yes, and these additional options don't work well on Power?
>>
>> No, the issue is that "make olddefconfig" takes the existing .config
>> without even considering the arch's default configuration that was
>> specified (KBUILD_DEFCONFIG).
>>
>>>> --- a/automation/scripts/build
>>>> +++ b/automation/scripts/build
>>>> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>>>>      # RANDCONFIG implies HYPERVISOR_ONLY
>>>>      HYPERVISOR_ONLY="y"
>>>>  else
>>>> -    echo "CONFIG_DEBUG=${debug}" > xen/.config
>>>> +    # Start off with arch's defconfig
>>>> +    make -C xen defconfig
>>>> +
>>>> +    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
>>>> +    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
>>>> +    echo "CONFIG_DEBUG=${debug}" >> xen/.config
>>>>
>>>>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>>>>          echo "${EXTRA_XEN_CONFIG}" >> xen/.config
>>
>> It never really became clear to me whether kconfig honors the first,
>> last, or any random setting in a .config that it takes as input, when
>> a certain option appears there more than once. The change you make
>> implies it's consistently "last" - can you confirm that's the actual
>> behavior of kconfig?
>>
> 
> I actually tried to avoid this issue alltogether with the sed command I
> added before the echo to drop any existing CONFIG_DEBUG= line.
>

Just realized that options in $EXTRA_XEN_CONFIG would also be subject to
this which is likely what you meant to point out -- my apologies.

I've tested locally and Kconfig does indeed seem to honor the last
setting. It throws some warnings about the overridden symbol but the
build continues with the latest value, e.g:

.config:94:warning: override: reassigning to symbol DEBUG
.config:95:warning: override: reassigning to symbol DEBUG

Thanks,
Shawn

Re: [PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Andrew Cooper]

On 25/09/2023 11:42 pm, Shawn Anastasio wrote:
> Since ppc64le is still undergoing early bringup, disable the randconfig
> CI build which was causing spurious CI failures.
>
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> Reported-by: Jan Beulich <jbeulich@suse.com>
> ---
>  automation/gitlab-ci/build.yaml | 18 ------------------
>  1 file changed, 18 deletions(-)
>
> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
> index 1619e9a558..32af30cced 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
>      KBUILD_DEFCONFIG: ppc64_defconfig
>      HYPERVISOR_ONLY: y
>  
> -debian-bullseye-gcc-ppc64le-randconfig:
> -  extends: .gcc-ppc64le-cross-build
> -  variables:
> -    CONTAINER: debian:bullseye-ppc64le
> -    KBUILD_DEFCONFIG: ppc64_defconfig
> -    RANDCONFIG: y
> -    EXTRA_FIXED_RANDCONFIG:
> -      CONFIG_COVERAGE=n
> -
> -debian-bullseye-gcc-ppc64le-debug-randconfig:
> -  extends: .gcc-ppc64le-cross-build-debug
> -  variables:
> -    CONTAINER: debian:bullseye-ppc64le
> -    KBUILD_DEFCONFIG: ppc64_defconfig
> -    RANDCONFIG: y
> -    EXTRA_FIXED_RANDCONFIG:
> -      CONFIG_COVERAGE=n

I know this has been committed, but it shouldn't have been.  Randconfig
is important to have even at this point in the bringup.

For options which are known-incompatible, append them to
EXTRA_FIXED_RANDCONFIG, just like COVERAGE already is.

However, it was only grant tables which showed up as broken, and ought
to be easy to address.

~Andrew

Re: [PATCH 2/3] automation: Change build script to use arch defconfig

[Jan Beulich]

On 26.09.2023 20:35, Shawn Anastasio wrote:
> On 9/26/23 12:43 PM, Shawn Anastasio wrote:
>> On 9/26/23 2:19 AM, Jan Beulich wrote:
>>> On 26.09.2023 01:12, Stefano Stabellini wrote:
>>>> On Mon, 25 Sep 2023, Shawn Anastasio wrote:
>>>>> Change automation build script to call the make defconfig target before
>>>>> setting CONFIG_DEBUG and extra options. This fixes issues on Power where
>>>>> the build fails without using the ppc64_defconfig.
>>>>>
>>>>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Nit: Tags in chronological order please (also affects patch 1).
>>>
>> Will fix.
>>
>>>> What is the problem specifically? Is the issue that CONFIG_DEBUG enabled
>>>> before make olddefconfig causes certain DEBUG options also to default to
>>>> yes, and these additional options don't work well on Power?
>>>
>>> No, the issue is that "make olddefconfig" takes the existing .config
>>> without even considering the arch's default configuration that was
>>> specified (KBUILD_DEFCONFIG).
>>>
>>>>> --- a/automation/scripts/build
>>>>> +++ b/automation/scripts/build
>>>>> @@ -22,7 +22,12 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
>>>>>      # RANDCONFIG implies HYPERVISOR_ONLY
>>>>>      HYPERVISOR_ONLY="y"
>>>>>  else
>>>>> -    echo "CONFIG_DEBUG=${debug}" > xen/.config
>>>>> +    # Start off with arch's defconfig
>>>>> +    make -C xen defconfig
>>>>> +
>>>>> +    # Drop existing CONFIG_DEBUG and replace with value of ${debug}
>>>>> +    sed -i 's/^CONFIG_DEBUG=[yn]//g' xen/.config
>>>>> +    echo "CONFIG_DEBUG=${debug}" >> xen/.config
>>>>>
>>>>>      if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
>>>>>          echo "${EXTRA_XEN_CONFIG}" >> xen/.config
>>>
>>> It never really became clear to me whether kconfig honors the first,
>>> last, or any random setting in a .config that it takes as input, when
>>> a certain option appears there more than once. The change you make
>>> implies it's consistently "last" - can you confirm that's the actual
>>> behavior of kconfig?
>>>
>>
>> I actually tried to avoid this issue alltogether with the sed command I
>> added before the echo to drop any existing CONFIG_DEBUG= line.
>>
> 
> Just realized that options in $EXTRA_XEN_CONFIG would also be subject to
> this which is likely what you meant to point out -- my apologies.
> 
> I've tested locally and Kconfig does indeed seem to honor the last
> setting. It throws some warnings about the overridden symbol but the
> build continues with the latest value, e.g:
> 
> .config:94:warning: override: reassigning to symbol DEBUG
> .config:95:warning: override: reassigning to symbol DEBUG

I might guess such warnings are okay to appear, but this needs confirming
by one of the people more familiar with how the CI works and what output
is or is not okay to appear. I can see upsides and downsides to such
warnings ...

Jan

Re: [PATCH 1/3] automation: Drop ppc64le-*randconfig jobs

[Jan Beulich]

On 26.09.2023 21:49, Andrew Cooper wrote:
> On 25/09/2023 11:42 pm, Shawn Anastasio wrote:
>> Since ppc64le is still undergoing early bringup, disable the randconfig
>> CI build which was causing spurious CI failures.
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> ---
>>  automation/gitlab-ci/build.yaml | 18 ------------------
>>  1 file changed, 18 deletions(-)
>>
>> diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
>> index 1619e9a558..32af30cced 100644
>> --- a/automation/gitlab-ci/build.yaml
>> +++ b/automation/gitlab-ci/build.yaml
>> @@ -563,24 +563,6 @@ debian-bullseye-gcc-ppc64le-debug:
>>      KBUILD_DEFCONFIG: ppc64_defconfig
>>      HYPERVISOR_ONLY: y
>>  
>> -debian-bullseye-gcc-ppc64le-randconfig:
>> -  extends: .gcc-ppc64le-cross-build
>> -  variables:
>> -    CONTAINER: debian:bullseye-ppc64le
>> -    KBUILD_DEFCONFIG: ppc64_defconfig
>> -    RANDCONFIG: y
>> -    EXTRA_FIXED_RANDCONFIG:
>> -      CONFIG_COVERAGE=n
>> -
>> -debian-bullseye-gcc-ppc64le-debug-randconfig:
>> -  extends: .gcc-ppc64le-cross-build-debug
>> -  variables:
>> -    CONTAINER: debian:bullseye-ppc64le
>> -    KBUILD_DEFCONFIG: ppc64_defconfig
>> -    RANDCONFIG: y
>> -    EXTRA_FIXED_RANDCONFIG:
>> -      CONFIG_COVERAGE=n
> 
> I know this has been committed, but it shouldn't have been.  Randconfig
> is important to have even at this point in the bringup.

Well. As so often you only comment when it's too late. The question whether
randconfig is sensible to have in the bringup phase was raised earlier. You
could have said "yes, keep it" there. With there not having been any comment
on the suggestion to drop this (and I similarly think for RISC-V, which is
likely to be similarly affected the moment the full build is enabled there),
I didn't expect any opposition to the patch, and hence went ahead committing
it.

> For options which are known-incompatible, append them to
> EXTRA_FIXED_RANDCONFIG, just like COVERAGE already is.

But in this early phase that merely means re-stating some/all of what the
default config states (which may in fact state a few too many fixed
settings).

> However, it was only grant tables which showed up as broken, and ought
> to be easy to address.

It may only be grant tables right now (that's what CI had spotted), but
other settings are likely to become (transient) problems as well. See
e.g.

#define smp_processor_id() 0 /* TODO: Fix this */

which imo might lead to the compiler spot bogus constructs, just because
at the same time NR_CPUS > 1.

Jan