Page MenuHomeFreeBSD

arm: support for VFP in kernel
Needs ReviewPublic

Authored by mkoz_semihalf.com on Thu, Nov 17, 2:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 1, 2:01 AM
Unknown Object (File)
Thu, Dec 1, 1:57 AM
Unknown Object (File)
Thu, Dec 1, 1:57 AM
Subscribers

Details

Reviewers
mw
wma
kd
imp
Summary

Implement missing fpu_kern_* functions to allow in-kernel VFP operation for ARMv7 NEON.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

sys/arm/conf/ARMADA38X
25 ↗(On Diff #113234)

This file should be a separate review as it adds support for neon/ossl to the specific board.

85 ↗(On Diff #113234)

revert that change

113 ↗(On Diff #113234)

should be removed

sys/arm/conf/std.armv7
69 ↗(On Diff #113234)

that lines should be left intact

mkoz_semihalf.com marked 3 inline comments as done.

Revert some changes in ARMADA38X and std.armv7 conf files.
neon/ossl support for ARMADA38X moved to another commit: https://reviews.freebsd.org/D37441

mw requested changes to this revision.Fri, Nov 18, 6:07 PM
mw added inline comments.
sys/arm/arm/vfp.c
423

This line is a bit too long, please break it as below:

	    ("Mangled pcb_fpusaved %x %p %p", pcb->pcb_fpflags, pcb->pcb_fpusaved,
            &pcb->pcb_fpustate));
sys/arm/conf/ARMADA38X
59 ↗(On Diff #113280)

Please add comment above or add to a relevant group. Also align spacing to the other entries.

60 ↗(On Diff #113280)

Shouldn't this change go to https://reviews.freebsd.org/D37441 ?

This revision now requires changes to proceed.Fri, Nov 18, 6:07 PM
sys/arm/conf/ARMADA38X
60 ↗(On Diff #113280)

IMO it should be included in D37420.

I'm not sure this is correct in the case we have a userspace thread calling into the kernel when the kernel uses the VFP.

When we enter the fpu state we save the userspace vfp registers to pcb->pcb_vfpstate (because pcb->pcb_vfpsaved == NULL). We then enable the VFP, however if a context switch was to occur before restoring the old vfp registers cpu_switch will also try to save the vfp registers to pcb->pcb_vfpstate. This will lead to the kernel registers trashing the userspace values.

You also need to update share/man/man9/fpu_kern.9

@andrew
Thanks for looking into it.
However, isn't that why we always pass ctx to fpu_kern_enter? In the end of that function (line 425) the vfpsaved pointer is swapped with ctx->prev which apparently should provide additional place for storing "old" vfp state. Therefore, in context switch any additional call to vfp_save_state should cause storing vfp state in temporary &ctx->state. Is it a case you were referring to?

I'm just curious as this approach works well on arm64 and I never saw issues with in-kernel vfp, however I admit I might not pushed it hard enough. Anyway, if there is a bug in this code I think it must be resolved in other platforms as well.

Therefore, in context switch any additional call to vfp_save_state should cause storing vfp state in temporary &ctx->state. Is it a case you were referring to?

The problem is cpu_switch doesn't call vfp_save_state on arm so it won't use the pcb->pcb_vfpsaved pointer.

I'm just curious as this approach works well on arm64 and I never saw issues with in-kernel vfp, however I admit I might not pushed it hard enough. Anyway, if there is a bug in this code I think it must be resolved in other platforms as well.

On arm64 we always use pcb->pcb_vfpsaved to find the correct state to use in context switch so make sure it is always valid (except in the savectx special case).

If you make sure the pcb_vfpsaved pointer is always valid, and call vfp_save_state where we are currently calling vfp_store the save side should work the same as on arm64. For restore you'll need to update vfp_bounce to handle a kernel VFP exception.

On arm64 we always use pcb->pcb_vfpsaved to find the correct state to use in context switch so make sure it is always valid (except in the savectx special case).
If you make sure the pcb_vfpsaved pointer is always valid, and call vfp_save_state where we are currently calling vfp_store the save side should work the same as on arm64. For restore you'll need to update vfp_bounce to handle a kernel VFP exception.

Yes, you're right, good point. We will follow up on this.

mkoz_semihalf.com marked an inline comment as done.

Fix context switch bug mentioned by @andrew.
Replace vfp_store with vfp_save_state in proper way.
vfp_bounce updated according to comment.

mkoz_semihalf.com marked 4 inline comments as done.

Update fpu_kern man
Move sys/arm/conf/ARMADA38X from https://reviews.freebsd.org/D37419