Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR

On Tue, Mar 26, 2019 at 04:22:34PM -0500, Scott Wood wrote:

On Tue, 2019-03-26 at 14:33 -0500, Alan Tull wrote:

quoted

On Mon, Mar 25, 2019 at 5:58 PM Scott Wood [off-list ref] wrote:

Hi Scott,

quoted

On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:

quoted

On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:

quoted

+#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
+
+#include <asm/fpu/api.h>
+
+static inline void copy512(void *src, void __iomem *dst)
+{
+   kernel_fpu_begin();
+
+   asm volatile("vmovdqu64 (%0), %%zmm0;"
+                "vmovntdq %%zmm0, (%1);"
+                :
+                : "r"(src), "r"(dst));
+
+   kernel_fpu_end();
+}

Shouldn't there be some sort of check that AVX512 is actually
supported
on the running system?

Also, src should be const, and the asm statement should have a memory
clobber.

Yes, I will fix this in the next version.

quoted

quoted

quoted

quoted

+#else
+static inline void copy512(void *src, void __iomem *dst)
+{
+   WARN_ON_ONCE(1);
+}
+#endif

Likewise, this will be called if a revision 2 device is used on non-
x86
(or on x86 with an old binutils).  The driver should fall back to 32-
bit
in such cases.

Unfortunately revision 2 is only for integrated FPGA solution, and it doesn't
support any fallback solution (original 32bit data partial reconfiguration is
not supported any more), so driver has to WARN in such path.

quoted

quoted

Sorry, I missed the comment about revision 2 only being on integrated
devices -- but will that always be the case?  Seems worthwhile to check
for
AVX512 support anyway.  And there's still the possibility of being built
with an old binutils such that CONFIG_AS_AVX512 is not set, or running
on a
kernel where avx512 was disabled via a boot option.

The code checks for CONFIG_AS_AVX512 above.

That just indicates that binutils supports it.  Plus, the code does not
check for CONFIG_AS_AVX512 when deciding whether to set pr_datawidth to 64
(and thus call copy512), so you'll get a WARN_ON rather than falling back to
32-bit.

quoted

What boot option are you referring to?

clearcpuid=304

Just tried it, my system was down after running above AVX512 with this option.

I agree that it needs to add some check code to make sure it's safe to run
such instructions. I will add some cpu_feature_enabled() check in the next
version.

Thanks a lot for the review and comments.

Hao

-Scott