amd64: handle MXCSR from XSAVEOPT when x87 state was optimized
ClosedPublic
Actions

Authored by kib on Mar 27 2024, 11:17 AM.

Details

Reviewers

kevans
markj
emaste
jhb
olce

Commits

rG1c091d11261a: x86: handle MXCSR from XSAVEOPT when x87 state was optimized

Summary

PR: 275322
Reported by: Cheyenne Wills <cheyenne.wills@gmail.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Mar 27 2024, 11:17 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 27 2024, 11:17 AM

kib requested review of this revision.Mar 27 2024, 11:17 AM

kib added a reviewer: jhb.Mar 27 2024, 11:19 AM

I'm not fully familiar with the xsave details - I'm curious why this was AMD-specific (and/or, what implies that the x87 area was not saved, in the new block)?

AMD-specific thing there is that X87 state was not saved, as reported by xstate_bv.bit0 == 0 (I believe Intel always saves X87). But regardless of that, MXCSR state is always saved even if X87 is not. By overwriting X87 non-saved area with the default content, we also override the potentially changed MXCSR, which was reported in the PR.

Presumably i386 needs the same fixup as well?

What an odd bug.

sys/amd64/amd64/fpu.c
889	AVX512? Realistically I suspect anything using AVX512 will also be dirtying AVX state (and really AVX should always imply SSE so you could maybe even get by with only checking SSE here?)
893

This revision is now accepted and ready to land.Mar 27 2024, 2:55 PM

kib marked 2 inline comments as done.Mar 27 2024, 3:25 PM

kib added inline comments.

sys/amd64/amd64/fpu.c
889	Literally, Intel SDM states IF RFBM[1] = 1 or RFBM[2] = 1 THEN store MXCSR and MXCSR_MASK into legacy region of XSAVE area; FI; But now I think that the mask must be saved as well.

Handle i386

This revision now requires review to proceed.Mar 27 2024, 3:25 PM

emaste added inline comments.Mar 27 2024, 3:37 PM

sys/amd64/amd64/fpu.c
905–908	we write this each time through the loop?

kib marked an inline comment as done.Mar 27 2024, 3:42 PM

kib added inline comments.

sys/amd64/amd64/fpu.c
905–908	Indeed. This is not incorrect but excessive of course.

Initialize do_mxcsr on each iteration.

olce added a subscriber: olce.Mar 27 2024, 4:22 PM

olce added inline comments.

sys/amd64/amd64/fpu.c
889	The Intel SDM also states that, when XSAVEC is used, only RFBM[1] set causes MXCSR and MXCSR_MASK to be stored. So shouldn't the newly introduced `if` be changed to deal with this situation, by testing only for RFBM[1], or RFBM[1] and RFBM[2], depending on whether XCOMP_BV[63] is set (flag for the compact format)?