Page MenuHomeFreeBSD

amd64 Store full 64bit of FIP/FDP for 64bit processes when using XSAVE.
ClosedPublic

Authored by kib on Oct 2 2020, 2:33 PM.

Details

Summary

If current process is 64bit, use rex-prefixed version of XSAVE (XSAVE64). If current process is 32bit and CPU supports saving segment registers cs/ds in the FPU save area, use non-prefixed variant of XSAVE.

Reported by: Michał Górny <mgorny@mgorny@moritz.systems>
PR: 250043

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Oct 2 2020, 2:33 PM

To be honest, I think you can call 32-bit version for 32-bit programs unconditionally, independently of CPUID_STDEXT_NFPUSG. After all, 32-bit programs use 32-bit addresses, so they always fit in the shorter form.

That said, either I'm doing something wrong or the patch doesn't seem to work. My (simpler) hack-patch didn't work either. So either GETFPREGS calls it somehow differently, or I failed to install and boot the new kernel properly ;-).

To be honest, I think you can call 32-bit version for 32-bit programs unconditionally, independently of CPUID_STDEXT_NFPUSG. After all, 32-bit programs use 32-bit addresses, so they always fit in the shorter form.

That said, either I'm doing something wrong or the patch doesn't seem to work. My (simpler) hack-patch didn't work either. So either GETFPREGS calls it somehow differently, or I failed to install and boot the new kernel properly ;-).

Can you give me a stand-alone test that checks this stuff ?

Sure. Here you are:

#include <sys/types.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <machine/reg.h>

#include <assert.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

int main() {
	int ret;
	int child = fork();
	assert(child != -1);
	if (child == 0) {
		ret = ptrace(PT_TRACE_ME, 0, NULL, 0);
		assert(ret != -1);

		uint16_t cw = 0x037e;

		asm volatile (
			"finit\n\t"
			"fldcw %0\n\t"
			"fdiv\n\t"
			"int3\n\t"
			:
			: "m"(cw)
			: "st"
		);

		exit(0);
	}

	int wst;
	ret = waitpid(child, &wst, 0);
	assert(ret == child);
	assert(WSTOPSIG(wst) == SIGTRAP);

	struct reg gpr;
	ret = ptrace(PT_GETREGS, child, (void*)&gpr, sizeof(gpr));
	assert(ret != -1);

	struct fpreg fpr;
	ret = ptrace(PT_GETFPREGS, child, (void*)&fpr, sizeof(fpr));
	assert(ret != -1);

#if defined(__x86_64__)
	printf("rip = %016lx\n", gpr.r_rip);
#else
	printf("rip = %08x\n", gpr.r_eip);
#endif
	printf("fip = %016lx\n", fpr.fpr_env[1]);
	printf("fdp = %016lx\n", fpr.fpr_env[2]);

	return 0;
}

On my system, it prints:

$ ./a.out 
rip = 0000000000201395
fip = 0000004300201392
fdp = 0000000000000000

which indicates it's gotten fcs = 0x0043. It should be just rip - 3.

Of course I forgot about the _main_ place where fpu context is typically stored.
Patch context switch code to use xsave64.

And include missed chunk from x86/. The definitions will be moved to amd64 md_var.h as the followup.

This revision is now accepted and ready to land.Oct 3 2020, 5:51 PM

I'm currently building the kernel with the new version and I should have some test results in an hour or so.

I can confirm that the patch fixes the LLDB problem too!