Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions

[Nicholas Piggin <npiggin@gmail.com>]

Jordan Niethe's on February 26, 2020 2:07 pm:
> From: Alistair Popple <alistair@popple.id.au>
> 
> Prefix instructions have their own FSCR bit which needs to enabled via
> a CPU feature. The kernel will save the FSCR for problem state but it
> needs to be enabled initially.
> 
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
>  arch/powerpc/include/asm/reg.h    |  3 +++
>  arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 1aa46dff0957..c7758c2ccc5f 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -397,6 +397,7 @@
>  #define SPRN_RWMR	0x375	/* Region-Weighting Mode Register */
>  
>  /* HFSCR and FSCR bit numbers are the same */
> +#define FSCR_PREFIX_LG	13	/* Enable Prefix Instructions */
>  #define FSCR_SCV_LG	12	/* Enable System Call Vectored */
>  #define FSCR_MSGP_LG	10	/* Enable MSGP */
>  #define FSCR_TAR_LG	8	/* Enable Target Address Register */
> @@ -408,11 +409,13 @@
>  #define FSCR_VECVSX_LG	1	/* Enable VMX/VSX  */
>  #define FSCR_FP_LG	0	/* Enable Floating Point */
>  #define SPRN_FSCR	0x099	/* Facility Status & Control Register */
> +#define   FSCR_PREFIX	__MASK(FSCR_PREFIX_LG)

When you add a new FSCR, there's a couple more things to do, check
out traps.c.

>  #define   FSCR_SCV	__MASK(FSCR_SCV_LG)
>  #define   FSCR_TAR	__MASK(FSCR_TAR_LG)
>  #define   FSCR_EBB	__MASK(FSCR_EBB_LG)
>  #define   FSCR_DSCR	__MASK(FSCR_DSCR_LG)
>  #define SPRN_HFSCR	0xbe	/* HV=1 Facility Status & Control Register */
> +#define   HFSCR_PREFIX	__MASK(FSCR_PREFIX_LG)
>  #define   HFSCR_MSGP	__MASK(FSCR_MSGP_LG)
>  #define   HFSCR_TAR	__MASK(FSCR_TAR_LG)
>  #define   HFSCR_EBB	__MASK(FSCR_EBB_LG)
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 182b4047c1ef..396f2c6c588e 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
>  	return 1;
>  }
>  
> +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
> +{
> +	u64 fscr, hfscr;
> +
> +	if (f->usable_privilege & USABLE_HV) {
> +		hfscr = mfspr(SPRN_HFSCR);
> +		hfscr |= HFSCR_PREFIX;
> +		mtspr(SPRN_HFSCR, hfscr);
> +	}
> +
> +	if (f->usable_privilege & USABLE_OS) {
> +		fscr = mfspr(SPRN_FSCR);
> +		fscr |= FSCR_PREFIX;
> +		mtspr(SPRN_FSCR, fscr);
> +
> +		if (f->usable_privilege & USABLE_PR)
> +			current->thread.fscr |= FSCR_PREFIX;
> +	}
> +
> +	return 1;
> +}

It would be good to be able to just use the default feature matching
for this, if possible? Do we not do the right thing with 
init_thread.fscr?


> +
>  struct dt_cpu_feature_match {
>  	const char *name;
>  	int (*enable)(struct dt_cpu_feature *f);
> @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
>  	{"vector-binary128", feat_enable, 0},
>  	{"vector-binary16", feat_enable, 0},
>  	{"wait-v3", feat_enable, 0},
> +	{"prefix-instructions", feat_enable_prefix, 0},

That's reasonable to make that a feature, will it specify a minimum
base set of prefix instructions or just that prefix instructions
with the prefix/suffix arrangement exist?

You may not need "-instructions" on the end, none of the other 
instructions do.

I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
a bit. We have to do a pass over new CPU feature device tree, and
some compatibility questions have come up recently.

If you wouldn't mind just adding the new [H]FSCR bits and faults
upstream for now, that would be good.

Thanks,
Nick