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
F106122967: D47394.id146995.diff
Wed, Dec 25, 7:05 PM
Unknown Object (File)
Wed, Dec 25, 6:45 PM
Unknown Object (File)
Wed, Dec 25, 6:25 PM
Unknown Object (File)
Wed, Dec 25, 6:08 PM
Unknown Object (File)
Wed, Dec 25, 6:41 AM
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
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 Skipped
Unit
Tests Skipped

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
1309

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
1309

I was not aware of use_xsave, thanks!

sys/amd64/amd64/fpu.c
1302

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
1302

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
1299

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

1326

Is this ilog2()?

1327
1339
1365
1368
1386

ilog2()?

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