linux-kernel - Re: [RFC v2 1/2] ppc/fpu: Add generic FPU api similar to x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YPfFgkD+kcRaH8Ow@infradead.org>
Date:   Wed, 21 Jul 2021 07:58:10 +0100
From:   Christoph Hellwig <hch@...radead.org>
To:     Anson Jacob <Anson.Jacob@....com>
Cc:     mpe@...erman.id.au, benh@...nel.crashing.org, paulus@...ba.org,
        christophe.leroy@...roup.eu, linuxppc-dev@...ts.ozlabs.org,
        amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Sunpeng.Li@....com, Harry Wentland <harry.wentland@....com>,
        qingqing.zhuo@....com, Rodrigo.Siqueira@....com, roman.li@....com,
        Christoph Hellwig <hch@...radead.org>,
        Aurabindo.Pillai@....com, Bhawanpreet.Lakha@....com,
        Christian K??nig <christian.koenig@....com>, bindu.r@....com
Subject: Re: [RFC v2 1/2] ppc/fpu: Add generic FPU api similar to x86

> +
> +/*
> + * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> + * disables preemption so be careful if you intend to use it for long periods
> + * of time.
> + * TODO: If you intend to use the FPU in irq/softirq you need to check first with
> + * irq_fpu_usable() if it is possible.

Please avoid the overly lone lines comments.

> +extern bool kernel_fpu_enabled(void);
> +extern void kernel_fpu_begin(void);
> +extern void kernel_fpu_end(void);

No need for the externs.

> +/*
> + * Track whether the kernel is using the FPU state
> + * currently.

This all fits on a single line.

> +static bool fpu_support(void)
> +{
> +	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
> +		return true;
> +	} else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
> +		return true;
> +	} else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
> +		return true;
> +	}

No need for the braces, or else after a return.  In fact this could
be simplified down to:

	return cpu_has_feature(CPU_FTR_VSX_COMP) ||
		cpu_has_feature(CPU_FTR_ALTIVEC_COMP) ||
		cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE));

> +	preempt_disable();
> +
> +#ifdef CONFIG_VSX
> +	if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
> +		enable_kernel_vsx();
> +		return;
> +	}
> +#endif
> +
> +#ifdef CONFIG_ALTIVEC
> +	if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
> +		enable_kernel_altivec();
> +		return;
> +	}
> +#endif
> +
> +#ifdef CONFIG_PPC_FPU
> +	if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
> +		enable_kernel_fp();
> +		return;
> +	}
> +#endif

All the features are defined away if not supported (and we already rely
on that in fpu_support()).  So this could become:

	if (cpu_has_feature(CPU_FTR_VSX_COMP))
		enable_kernel_vsx();
	else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP))
		enable_kernel_altivec();
	else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE))
		enable_kernel_fp();

Same for the disable path.