Received-SPF: pass (google.com: domain of linux-kernel+bounces-197687-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 31 May 2024 21:35:55 -0700
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Zhouyi Zhou <zhouzhouyi@gmail.com>
Cc: linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
	linux-kselftest@vger.kernel.org, lance@osuosl.org
Subject: Re: [PATCH V2] rcutorture: Add CFcommon.arch for the various arch's
 need
Message-ID: <cd3709dc-2c08-4ac2-a19b-d8edb66195e3@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20240531235347.1118612-1-zhouzhouyi@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240531235347.1118612-1-zhouzhouyi@gmail.com>

On Fri, May 31, 2024 at 11:53:47PM +0000, Zhouyi Zhou wrote:
> Add CFcommon.arch for the various arch's need for rcutorture.
> 
> According to [1] and [2], this patch
> Fixes: a6fda6dab93c ("rcutorture: Tweak kvm options") by moving
> x86 specific kernel option CONFIG_HYPERVISOR_GUEST to CFcommon.x86
> 
> [1] https://lore.kernel.org/all/20240427005626.1365935-1-zhouzhouyi@gmail.com/
> [2] https://lore.kernel.org/all/059d36ce-6453-42be-a31e-895abd35d590@paulmck-laptop/
> 
> Tested in x86_64 and PPC VM of Open Source Lab of Oregon State University.
> 
> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> ---
> Hi Paul,
> 
> I tried very hard to find in Linux kernel on how to dig out
> the x86 specific kernel option CONFIG_HYPERVISOR_GUEST before configcheck.sh
> generates ConfigFragment.diags.
> 
> I can only find this functionality in scripts/kconfig/conf which travels
> the Kconfig hierarchy.
> 
> But the output of scripts/kconfig/conf, which is .config
> is also one of the input of configcheck.sh:
> ```
> kvm-recheck.sh:	configcheck.sh $i/.config $i/ConfigFragment > $i/ConfigFragment.diags 2>&1
> ```
> 
> I feel some logic paradox in it ;-)

At least now we can say that you looked.

> So, I pick the simplest way.

Very good, thank you!

> One more thing, recent change in include/linux/bitmap.h cause the make
> of allmodconfig fail because of warning on both x86 platforms, I am
> going to do research on it.

Thank you, looking forward to seeing what you come up with.

One question below.

> Thank your for your guidance
> Zhouyi
> --
>  tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh | 9 +++++++++
>  tools/testing/selftests/rcutorture/configs/rcu/CFcommon  | 1 -
>  .../selftests/rcutorture/configs/rcu/CFcommon.x86        | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/rcutorture/configs/rcu/CFcommon.x86
> 
> diff --git a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> index b33cd8753689..5332224238ba 100755
> --- a/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> +++ b/tools/testing/selftests/rcutorture/bin/kvm-test-1-run.sh
> @@ -62,6 +62,15 @@ config_override_param () {
>  }
>  
>  echo > $T/KcList
> +if uname -m | grep -q 86

Why not just use "uname -m" output itself for the filename extension?
That way, we wouldn't need this "if" statement.  Or are you looking to
map both "i686" and "x86_64" to "x86"?

> +# TODO: add other architecture-specific common configuration when needed
> +then
> +	if test -f $config_dir/CFcommon.x86

Do we really need this inner "if"?  The config_override_param() bash
function already checks for ran empty-string third argument.  Or does
this cause some other problem?

> +	then
> +		config_override_param "$config_dir/CFcommon.x86" KcList\
> +			"`cat $config_dir/CFcommon.x86 2> /dev/null`"
> +	fi
> +fi
>  config_override_param "$config_dir/CFcommon" KcList "`cat $config_dir/CFcommon 2> /dev/null`"

Also, the CFcommon.x86 check shouild follow the CFcommon check.  This
allows architecture-specific values to override the global ones.

							Thanx, Paul

>  config_override_param "$config_template" KcList "`cat $config_template 2> /dev/null`"
>  config_override_param "--gdb options" KcList "$TORTURE_KCONFIG_GDB_ARG"
> diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> index 0e92d85313aa..cf0387ae5358 100644
> --- a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> +++ b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon
> @@ -1,6 +1,5 @@
>  CONFIG_RCU_TORTURE_TEST=y
>  CONFIG_PRINTK_TIME=y
> -CONFIG_HYPERVISOR_GUEST=y
>  CONFIG_PARAVIRT=y
>  CONFIG_KVM_GUEST=y
>  CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n
> diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFcommon.x86 b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon.x86
> new file mode 100644
> index 000000000000..2770560d56a0
> --- /dev/null
> +++ b/tools/testing/selftests/rcutorture/configs/rcu/CFcommon.x86
> @@ -0,0 +1 @@
> +CONFIG_HYPERVISOR_GUEST=y
> -- 
> 2.25.1
>