Re: [PATCH v2] LoongArch: Clean useless vcsr in loongarch_fpu.

[WANG Xuerui <kernel@xen0n.name>]

Hi,

I just noticed that the subject line is in Chinglish too. It should 
probably be just "LoongArch: remove mentions of vcsr" and without the 
trailing period.

On 2022/7/4 23:36, Qi Hu wrote:
> The `vcsr` is not used anymore. Remove this member from `loongarch_fpu`.
>
>  From 3A5000(LoongArch), `vcsr` is removed in hardware. FP and LSX/LASX
> both use `fcsr` as their csr.
>
> Particularly, fcsr from $r16 to $r31 are reserved for LSX/LASX, and
> using the registers in this area will cause SXD/ASXD if LSX/LASX is
> not enabled.

Actually I'm still not very satisfied with the explanation; the code 
must be written with *something* in mind, since GS464V/LA464 is the only 
LoongArch implementation so far, it must have a VCSR to begin with. And 
you can't magically melt the VCSR on the tens of thousands of 
3A5000/3C5000's already shipped, because the old-world kernel obviously 
comes with LSX/LASX and it most likely utilizes the VCSR. In addition, 
you didn't mention what will happen if LSX/LASX *is* enabled on this 
new-world kernel, *and* fcsr16 is being accessed.

I think maybe you just want to remove the mentions of VCSR since they 
are dead code right now, as I don't believe it's gone in shipped 
hardware, as explained above. Except if there's magically a way to 
implement LSX/LASX without touching the said-to-have-disappeared VCSR, 
which I don't know of, and can't know because the LSX/LASX ISA manual is 
still not publicly accessible; so I don't feel comfortable approving 
this patch.

(BTW, the $rXX-style reference to the FCSRs is obviously an 
implementation wart of the toolchain, the FCSR is nothing like GPR so it 
obviously shouldn't be referred to as one. Binutils patch will be out 
shortly.)

>
> Signed-off-by: Qi Hu <huqi@loongson.cn>
> ---
> V2:
> - Add more details in the commit message.
> ---
>   arch/loongarch/include/asm/fpregdef.h  |  1 -
>   arch/loongarch/include/asm/processor.h |  2 --
>   arch/loongarch/kernel/asm-offsets.c    |  1 -
>   arch/loongarch/kernel/fpu.S            | 10 ----------
>   4 files changed, 14 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/fpregdef.h b/arch/loongarch/include/asm/fpregdef.h
> index adb16e4b4..b6be52783 100644
> --- a/arch/loongarch/include/asm/fpregdef.h
> +++ b/arch/loongarch/include/asm/fpregdef.h
> @@ -48,6 +48,5 @@
>   #define fcsr1	$r1
>   #define fcsr2	$r2
>   #define fcsr3	$r3
> -#define vcsr16	$r16
>   
>   #endif /* _ASM_FPREGDEF_H */
> diff --git a/arch/loongarch/include/asm/processor.h b/arch/loongarch/include/asm/processor.h
> index 1d63c934b..57ec45aa0 100644
> --- a/arch/loongarch/include/asm/processor.h
> +++ b/arch/loongarch/include/asm/processor.h
> @@ -80,7 +80,6 @@ BUILD_FPR_ACCESS(64)
>   
>   struct loongarch_fpu {
>   	unsigned int	fcsr;
> -	unsigned int	vcsr;
>   	uint64_t	fcc;	/* 8x8 */
>   	union fpureg	fpr[NUM_FPU_REGS];
>   };
> @@ -161,7 +160,6 @@ struct thread_struct {
>   	 */							\
>   	.fpu			= {				\
>   		.fcsr		= 0,				\
> -		.vcsr		= 0,				\
>   		.fcc		= 0,				\
>   		.fpr		= {{{0,},},},			\
>   	},							\
> diff --git a/arch/loongarch/kernel/asm-offsets.c b/arch/loongarch/kernel/asm-offsets.c
> index bfb65eb28..20cd9e16a 100644
> --- a/arch/loongarch/kernel/asm-offsets.c
> +++ b/arch/loongarch/kernel/asm-offsets.c
> @@ -166,7 +166,6 @@ void output_thread_fpu_defines(void)
>   
>   	OFFSET(THREAD_FCSR, loongarch_fpu, fcsr);
>   	OFFSET(THREAD_FCC,  loongarch_fpu, fcc);
> -	OFFSET(THREAD_VCSR, loongarch_fpu, vcsr);
>   	BLANK();
>   }
>   
> diff --git a/arch/loongarch/kernel/fpu.S b/arch/loongarch/kernel/fpu.S
> index 75c6ce068..a631a7137 100644
> --- a/arch/loongarch/kernel/fpu.S
> +++ b/arch/loongarch/kernel/fpu.S
> @@ -146,16 +146,6 @@
>   	movgr2fcsr	fcsr0, \tmp0
>   	.endm
>   
> -	.macro sc_save_vcsr base, tmp0
> -	movfcsr2gr	\tmp0, vcsr16
> -	EX	st.w \tmp0, \base, 0
> -	.endm
> -
> -	.macro sc_restore_vcsr base, tmp0
> -	EX	ld.w \tmp0, \base, 0
> -	movgr2fcsr	vcsr16, \tmp0
> -	.endm
> -
>   /*
>    * Save a thread's fp context.
>    */