Page MenuHomeFreeBSD

x86: Add routines for querying XSAVE feature information
ClosedPublic

Authored by bnovkov on Nov 1 2024, 3:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 5:11 PM
Unknown Object (File)
Sun, Dec 15, 2:04 PM
Unknown Object (File)
Fri, Dec 13, 1:36 PM
Unknown Object (File)
Thu, Dec 5, 4:42 AM
Unknown Object (File)
Nov 16 2024, 8:02 AM
Unknown Object (File)
Nov 15 2024, 9:07 PM
Unknown Object (File)
Nov 6 2024, 5:37 AM
Unknown Object (File)
Nov 4 2024, 4:12 PM
Subscribers

Details

Summary

This patch adds several routines that track and expose information about various XSAVE-related features.
More specifically, it adds the ability to check whether a given XFEATURE is supported and which XSAVE extensions are supported.
Furthermore, it adds several routines for calculating the size and offsets within a save area given a XSAVE feature bitmap.

Test Plan

Tested by converting D46397 to use the newly added interfaces.
No issues were encountered while tracing.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

A quick remark regarding code placement - I couldn't decide where this code should reside so I ultimately placed it in cpu_machdep.c, but I'm not too happy about it.
I'd certainly like to hear your thoughts on where this should reside.

amd64/amd64/fpu.c seems to be the natural place for the new functions.
Despite named 'fpu', the file deals with the whole non-GPR part of the CPU state.

sys/x86/include/specialreg.h
381 ↗(On Diff #145860)

Commit this chunk already

sys/x86/x86/cpu_machdep.c
1811 ↗(On Diff #145860)

Does it ever make sense to call these functions if XSAVE(S) are not supported?
Would it be better to convert the checks into KASSERTs?

1817 ↗(On Diff #145860)

'else' is not needed

Address @kib 's comments:

  • split specialreg.h definitions into separate commit
  • move xstate routines to amd64/fpu.c
sys/x86/x86/cpu_machdep.c
1811 ↗(On Diff #145860)

hm, calling these routines without xsave support does not make sense indeed, but I wonder what's the best way to handle this.
I guess we could convert these into KASSERTs and document that the caller must check for XSAVE support before calling these routines, maybe even add cpu_xsave_supported into fpu.h. What do you think?

sys/amd64/amd64/fpu.c
1311

Why cannot you check for use_xsave instead?

sys/x86/x86/cpu_machdep.c
1811 ↗(On Diff #145860)

Typically the low level functions are only called by the higher-level when the hw has required features because high-level simply refuses to work if features are absent. This makes the re-check of the features in the low-level redundant.

But asserts are useful at least as a form of the documentation for assumptions.

Address @kib 's comments:

  • Move checks to assertions
  • Check for use_xsave
bnovkov added inline comments.
sys/amd64/amd64/fpu.c
1311

I was not aware of use_xsave, thanks!

sys/amd64/amd64/fpu.c
1304

We already have xsave_area_desc.
Can we extend it instead of introducing a parallel structure?

Also look how we malloc large enough xsave_area_desc instead of hardcoding the max known extension.

bnovkov marked an inline comment as done.

Address @kib 's comments:

  • Move XFEATURE component metadata into xsave_area_desc
sys/amd64/amd64/fpu.c
1304

Same as with`use_xsave`, I managed to miss xsave_area_desc.
I've extended the struct with a flag field, but I had to keep an additional variable for tracking XSAVE extensions.

sys/amd64/amd64/fpu.c
1301

Still I want to get rid of the max xfeature number hardcoded.

1328

Is this ilog2()?

1329
1341
1367
1370
1388

ilog2()?

1390
This revision is now accepted and ready to land.Mon, Dec 9, 3:34 AM