Re: [RFC] mips: Add MXU context switching support

[Florian Fainelli]

Le 25/06/2016 05:14, PrasannaKumar Muralidharan a Ãcrit :
> From: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx>
> 
> This patch adds support for context switching Xburst MXU registers. The
> registers are named xr0 to xr16. xr16 is the control register that can
> be used to enable and disable MXU instruction set. Read and write to
> these registers can be done without enabling MXU instruction set by user
> space. Only when MXU instruction set is enabled any MXU instruction
> (other than read or write to xr registers) can be done. xr0 is always 0.
> 
> Kernel does not know when MXU instruction is enabled or disabled. So
> during context switch if MXU is enabled in xr16 register then MXU
> registers are saved, restored when the task is run. When user space
> application enables MXU, it is not reflected in other threads
> immediately. So for convenience the applications can use prctl syscall
> to let the MXU state propagate across threads running in different CPUs.

This is all well and good and seems useful, but you have not stated why
this is even useful in the first place?

> 
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx>
> ---

[snip]

> diff --git a/arch/mips/include/asm/processor.h b/arch/mips/include/asm/processor.h
> index 7e78b62..a4def30 100644
> --- a/arch/mips/include/asm/processor.h
> +++ b/arch/mips/include/asm/processor.h
> @@ -142,6 +142,11 @@ struct mips_dsp_state {
>  	unsigned int	dspcontrol;
>  };
>  
> +#define NUM_MXU_REGS 16
> +struct xburst_mxu_state {
> +	unsigned int xr[NUM_MXU_REGS];
> +};
> +
>  #define INIT_CPUMASK { \
>  	{0,} \
>  }
> @@ -266,6 +271,10 @@ struct thread_struct {
>  	/* Saved state of the DSP ASE, if available. */
>  	struct mips_dsp_state dsp;
>  
> +	unsigned int mxu_used;
> +	/* Saved registers of Xburst MXU, if available. */
> +	struct xburst_mxu_state mxu;

That's adding about 17 * 4 bytes to a structure that is probably best
kept as small as possible, might be worth adding an ifdef here specific
to the Ingenic platforms w/ MXU?

> +
>  	/* Saved watch register state, if available. */
>  	union mips_watch_reg_state watch;
>  
> @@ -330,6 +339,10 @@ struct thread_struct {
>  		.dspr		= {0, },			\
>  		.dspcontrol	= 0,				\
>  	},							\
> +	.mxu_used		= 0,				\
> +	.mxu			= {				\
> +		.xr		= {0, },			\
> +	},							\
>  	/*							\
>  	 * saved watch register stuff				\
>  	 */							\
> @@ -410,4 +423,10 @@ extern int mips_set_process_fp_mode(struct task_struct *task,
>  #define GET_FP_MODE(task)		mips_get_process_fp_mode(task)
>  #define SET_FP_MODE(task,value)		mips_set_process_fp_mode(task, value)
>  
> +extern int mips_enable_mxu_other_cpus(void);
> +extern int mips_disable_mxu_other_cpus(void);
> +
> +#define ENABLE_MXU_OTHER_CPUS()         mips_enable_mxu_other_cpus()
> +#define DISABLE_MXU_OTHER_CPUS()        mips_disable_mxu_other_cpus()

Where is the stub when building for !MIPS? Have you build a kernel with
your changes for e.g: x86?

[snip]

> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index a8d0759..b193d91 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -197,4 +197,7 @@ struct prctl_mm_map {
>  # define PR_CAP_AMBIENT_LOWER		3
>  # define PR_CAP_AMBIENT_CLEAR_ALL	4
>  
> +#define PR_ENABLE_MXU_OTHER_CPUS        48
> +#define PR_DISABLE_MXU_OTHER_CPUS       49
> +
>  #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 89d5be4..fbbc7b2 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2270,6 +2270,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  	case PR_GET_FP_MODE:
>  		error = GET_FP_MODE(me);
>  		break;
> +	case PR_ENABLE_MXU_OTHER_CPUS:
> +		error = ENABLE_MXU_OTHER_CPUS();
> +		break;
> +	case PR_DISABLE_MXU_OTHER_CPUS:
> +		error = DISABLE_MXU_OTHER_CPUS();
> +		break;

Is not there a way to call into an architecture specific prctl() for the
unhandled options passed to prctl()? Not everybody will
want/implement/support that, and, more importantly changing generic code
here looks fishy and probably the wrong abstraction.
-- 
Florian