tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 8291eaafed36f575f23951f3ce18407f480e9ecf
commit: e0838f6373e5cb72516fc4c26bba309097e2a80a arm64/sme: Save and restore streaming mode over EFI runtime calls
date: 5 weeks ago
config: arm64-randconfig-s032-20220527 (https://download.01.org/0day-ci/archive/20220528/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 11.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-14-g5a0004b5-dirty
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0838f6373e5cb72516fc4c26bba309097e2a80a
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git fetch --no-tags linus master
git checkout e0838f6373e5cb72516fc4c26bba309097e2a80a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kernel/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> arch/arm64/kernel/fpsimd.c:1944:55: sparse: sparse: dereference of noderef expression
vim +1944 arch/arm64/kernel/fpsimd.c
1916
1917 /*
1918 * __efi_fpsimd_end(): clean up FPSIMD after an EFI runtime services call
1919 */
1920 void __efi_fpsimd_end(void)
1921 {
1922 if (!system_supports_fpsimd())
1923 return;
1924
1925 if (!__this_cpu_xchg(efi_fpsimd_state_used, false)) {
1926 kernel_neon_end();
1927 } else {
1928 if (system_supports_sve() &&
1929 likely(__this_cpu_read(efi_sve_state_used))) {
1930 char const *sve_state = this_cpu_ptr(efi_sve_state);
1931 bool ffr = true;
1932
1933 /*
1934 * Restore streaming mode; EFI calls are
1935 * normal function calls so should not return in
1936 * streaming mode.
1937 */
1938 if (system_supports_sme()) {
1939 if (__this_cpu_read(efi_sm_state)) {
1940 sysreg_clear_set_s(SYS_SVCR_EL0,
1941 0,
1942 SYS_SVCR_EL0_SM_MASK);
1943 if (!system_supports_fa64())
> 1944 ffr = efi_sm_state;
1945 }
1946 }
1947
1948 sve_load_state(sve_state + sve_ffr_offset(sve_max_vl()),
1949 &this_cpu_ptr(&efi_fpsimd_state)->fpsr,
1950 ffr);
1951
1952 __this_cpu_write(efi_sve_state_used, false);
1953 } else {
1954 fpsimd_load_state(this_cpu_ptr(&efi_fpsimd_state));
1955 }
1956 }
1957 }
1958
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sat, May 28, 2022 at 06:49:01AM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 8291eaafed36f575f23951f3ce18407f480e9ecf
> commit: e0838f6373e5cb72516fc4c26bba309097e2a80a arm64/sme: Save and restore streaming mode over EFI runtime calls
> date: 5 weeks ago
> config: arm64-randconfig-s032-20220527 (https://download.01.org/0day-ci/archive/20220528/[email protected]/config)
> compiler: aarch64-linux-gcc (GCC) 11.3.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # apt-get install sparse
> # sparse version: v0.6.4-14-g5a0004b5-dirty
> # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0838f6373e5cb72516fc4c26bba309097e2a80a
> git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git fetch --no-tags linus master
> git checkout e0838f6373e5cb72516fc4c26bba309097e2a80a
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kernel/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
>
>
> sparse warnings: (new ones prefixed by >>)
> >> arch/arm64/kernel/fpsimd.c:1944:55: sparse: sparse: dereference of noderef expression
>
> vim +1944 arch/arm64/kernel/fpsimd.c
>
> 1916
> 1917 /*
> 1918 * __efi_fpsimd_end(): clean up FPSIMD after an EFI runtime services call
> 1919 */
> 1920 void __efi_fpsimd_end(void)
> 1921 {
> 1922 if (!system_supports_fpsimd())
> 1923 return;
> 1924
> 1925 if (!__this_cpu_xchg(efi_fpsimd_state_used, false)) {
> 1926 kernel_neon_end();
> 1927 } else {
> 1928 if (system_supports_sve() &&
> 1929 likely(__this_cpu_read(efi_sve_state_used))) {
> 1930 char const *sve_state = this_cpu_ptr(efi_sve_state);
> 1931 bool ffr = true;
> 1932
> 1933 /*
> 1934 * Restore streaming mode; EFI calls are
> 1935 * normal function calls so should not return in
> 1936 * streaming mode.
> 1937 */
> 1938 if (system_supports_sme()) {
> 1939 if (__this_cpu_read(efi_sm_state)) {
> 1940 sysreg_clear_set_s(SYS_SVCR_EL0,
> 1941 0,
> 1942 SYS_SVCR_EL0_SM_MASK);
> 1943 if (!system_supports_fa64())
> > 1944 ffr = efi_sm_state;
This looks wrong indeed. IIUC, something like below but waiting for Mark
to confirm:
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 819979398127..ac521b3403d6 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1965,7 +1965,7 @@ void __efi_fpsimd_end(void)
0,
SVCR_SM_MASK);
if (!system_supports_fa64())
- ffr = efi_sm_state;
+ ffr = false;
}
}
--
Catalin
On Tue, May 31, 2022 at 05:57:42PM +0100, Catalin Marinas wrote:
> This looks wrong indeed. IIUC, something like below but waiting for Mark
> to confirm:
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 819979398127..ac521b3403d6 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1965,7 +1965,7 @@ void __efi_fpsimd_end(void)
> 0,
> SVCR_SM_MASK);
> if (!system_supports_fa64())
> - ffr = efi_sm_state;
> + ffr = false;
Yes, that looks right - feel free to add my Reviewed-by or I'll
do that when I see the patch sent properly.
On Wed, Jun 01, 2022 at 09:50:39AM +0200, Mark Brown wrote:
> On Tue, May 31, 2022 at 05:57:42PM +0100, Catalin Marinas wrote:
> > This looks wrong indeed. IIUC, something like below but waiting for Mark
> > to confirm:
> >
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index 819979398127..ac521b3403d6 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -1965,7 +1965,7 @@ void __efi_fpsimd_end(void)
> > 0,
> > SVCR_SM_MASK);
> > if (!system_supports_fa64())
> > - ffr = efi_sm_state;
> > + ffr = false;
>
> Yes, that looks right - feel free to add my Reviewed-by or I'll
> do that when I see the patch sent properly.
I'm no longer convinced that's the right fix. In __efi_fpsimd_begin(),
ffr is true if either !system_supports_fa64() or SVCR.SM is set. The FFR
state would be saved in this case.
In __efi_fpsimd_end(), shouldn't we just restore the FFR state if
efi_sm_state, irrespective of system_supports_fa64() (as it was checked
in the beginning). Something like:
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 819979398127..8bc9080adab2 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1960,13 +1960,12 @@ void __efi_fpsimd_end(void)
* streaming mode.
*/
if (system_supports_sme()) {
- if (__this_cpu_read(efi_sm_state)) {
+ if (__this_cpu_read(efi_sm_state))
sysreg_clear_set_s(SYS_SVCR,
0,
SVCR_SM_MASK);
- if (!system_supports_fa64())
- ffr = efi_sm_state;
- }
+ else
+ ffr = false;
}
sve_load_state(sve_state + sve_ffr_offset(sve_max_vl()),
--
Catalin
On Wed, Jun 01, 2022 at 05:54:40PM +0100, Catalin Marinas wrote:
> On Wed, Jun 01, 2022 at 09:50:39AM +0200, Mark Brown wrote:
> > On Tue, May 31, 2022 at 05:57:42PM +0100, Catalin Marinas wrote:
> > Yes, that looks right - feel free to add my Reviewed-by or I'll
> > do that when I see the patch sent properly.
> I'm no longer convinced that's the right fix. In __efi_fpsimd_begin(),
> ffr is true if either !system_supports_fa64() or SVCR.SM is set. The FFR
> state would be saved in this case.
That's the wrong way round - we should only save FFR if we have
FA64 or SVCR.SM is *not* set. Checking again efi_fpsimd_begin()
is wrong.
> In __efi_fpsimd_end(), shouldn't we just restore the FFR state if
> efi_sm_state, irrespective of system_supports_fa64() (as it was checked
> in the beginning). Something like:
No, we should restore it only if we saved it. The below should
resolve things but it's completely untested:
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 819979398127..3c66a061ff6f 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1916,10 +1916,11 @@ void __efi_fpsimd_begin(void)
if (system_supports_sme()) {
svcr = read_sysreg_s(SYS_SVCR);
- if (!system_supports_fa64())
- ffr = svcr & SVCR_SM_MASK;
+ __this_cpu_write(efi_sm_state,
+ svcr & SVCR_SM_MASK);
- __this_cpu_write(efi_sm_state, ffr);
+ if (!system_supports_fa64())
+ ffr = !(svcr & SVCR_SM_MASK);
}
sve_save_state(sve_state + sve_ffr_offset(sve_max_vl()),
@@ -1965,7 +1966,7 @@ void __efi_fpsimd_end(void)
0,
SVCR_SM_MASK);
if (!system_supports_fa64())
- ffr = efi_sm_state;
+ ffr = false;
}
}