Skip to content

8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) #18562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) #18562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b38266c
8329031: CPUID feature detection for APX during VM initialization.
Apr 1, 2024
de7d8cb
Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031
May 1, 2024
ad39d99
Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031
May 18, 2024
e98b5c6
32-bit build fix
May 18, 2024
93adcc2
Making UseAPX a boolean flag.
May 18, 2024
3284fd9
Minor modification in UseAPX flag description
May 25, 2024
24baa30
Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031
May 25, 2024
97d6538
Post merge clenups.
May 25, 2024
0881e43
Update vm_version_x86.cpp
May 31, 2024
b5da093
Review comments resolutions.
Jun 6, 2024
68df08c
Review comments addressed.
Jun 7, 2024
d8fcde9
Lazy restored state comparison after OS signal handling.
Jun 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions src/hotspot/cpu/x86/vm_version_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ address VM_Version::_cpuinfo_segv_addr_apx = 0;
// Address of instruction after the one which causes APX specific SEGV
address VM_Version::_cpuinfo_cont_addr_apx = 0;
// Address of apx state restore error handler.
address VM_Version::_apx_state_restore_error_handler = (address)VM_Version::report_apx_state_restore_error;
address VM_Version::_apx_state_restore_warning_handler = (address)VM_Version::report_apx_state_restore_warning;

static BufferBlob* stub_blob;
static const int stub_size = 2000;
Expand Down Expand Up @@ -113,11 +113,16 @@ class VM_Version_StubGenerator: public StubCodeGenerator {
address clear_apx_test_state() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to clear_apx_test_state? r16 onwards are not callee saved. And checking r15 save/restore is not needed so we could remove r15 changes altogether.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, EGPRs are call clobbered registers, but here we are trying to ascertain if their values are preserved across signal handling. Explicit clearing of r16 and r31 during signal handling guarantees that preserved register values post signal handling were re-instantiated by operating system and not because they were not modified externally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add comment about that.

# define __ _masm->
address start = __ pc();
//__ mov64(r15, 0L);
// FIXME Uncomment following code after OS enablement of
/* FIXME Uncomment following code after OS enablement of
bool save_apx = UseAPX;
VM_Version::set_apx_cpuFeatures();
UseAPX = true;
// EGPR state save/restoration.
//__ mov64(r16, 0L);
//__ mov64(r31, 0L);
__ mov64(r16, 0L);
__ mov64(r31, 0L);
UseAPX = save_apx;
VM_Version::clean_cpuFeatures();
*/
__ ret(0);
return start;
}
Expand All @@ -134,7 +139,7 @@ class VM_Version_StubGenerator: public StubCodeGenerator {

Label detect_486, cpu486, detect_586, std_cpuid1, std_cpuid4;
Label sef_cpuid, sefsl1_cpuid, ext_cpuid, ext_cpuid1, ext_cpuid5, ext_cpuid7;
Label ext_cpuid8, done, wrapup, vector_save_restore, apx_save_restore_error;
Label ext_cpuid8, done, wrapup, vector_save_restore, apx_save_restore_warning;
Label legacy_setup, save_restore_except, legacy_save_restore, start_simd_check;

StubCodeMark mark(this, "VM_Version", "get_cpu_info_stub");
Expand Down Expand Up @@ -430,8 +435,10 @@ class VM_Version_StubGenerator: public StubCodeGenerator {
__ cmpl(rax, 0x80000);
__ jcc(Assembler::notEqual, vector_save_restore);

/* FIXME: Uncomment after integration of JDK-8328998
__ mov64(r15, VM_Version::egpr_test_value());
/* FIXME: Uncomment while integrating JDK-8329032
bool save_apx = UseAPX;
Comment on lines +440 to +441
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you missing to uncomment this code?
8329032 is about .ad file changes. It should not affect execution of this code.
You need changes in register_x86.* files and may be somewhere else but you don't need C2 changes for this code to work.

Copy link
Member Author

@jatin-bhateja jatin-bhateja Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we already have that in place with #19042, which will be open for review after this patch. I added it in comments since this piece of logic is centered around CPUID feature check and pertinent to this patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

VM_Version::set_apx_cpuFeatures();
UseAPX = true;
__ mov64(r16, VM_Version::egpr_test_value());
__ mov64(r31, VM_Version::egpr_test_value());
*/
Expand All @@ -441,19 +448,18 @@ class VM_Version_StubGenerator: public StubCodeGenerator {
__ movl(rax, Address(rsi, 0));

VM_Version::set_cpuinfo_cont_addr_apx(__ pc());
// Validate the contents of r15, r16 and r31
/* FIXME: Uncomment after integration of JDK-8328998
// Validate the contents of r16 and r31
/* FIXME: Uncomment after integration of JDK-8329032
__ mov64(rax, VM_Version::egpr_test_value());
__ cmpq(rax, r15);
__ jccb(Assembler::notEqual, apx_save_restore_error);
__ cmpq(rax, r16);
__ jccb(Assembler::notEqual, apx_save_restore_error);
__ jccb(Assembler::notEqual, apx_save_restore_warning);
__ cmpq(rax, r31);
__ jccb(Assembler::equal, vector_save_restore);

// Generate SEGV to signal unsuccessful save/restore.
__ bind(apx_save_restore_error);
__ lea(rax, ExternalAddress(VM_Version::_apx_state_restore_error_handler));
UseAPX = save_apx;

__ bind(apx_save_restore_warning);
__ lea(rax, ExternalAddress(VM_Version::_apx_state_restore_warning_handler));
__ call(rax);
*/
#endif
Expand Down Expand Up @@ -868,6 +874,11 @@ class VM_Version_StubGenerator: public StubCodeGenerator {
};
};

void VM_Version::report_apx_state_restore_warning() {
tty->print("warning: Unsuccessful EGPRs state restoration across signal handling, setting UseAPX to false.\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This print is fine during development but I would instead save some value in memory to indicate that OS does not save/restore APX. And then check it after we execute this assembler code. Similar how we do that for AVX.
You would not need to do runtime call and this method then.
Note: tty->print() can do "nasty"/unexpected things which you want to avoid.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vnkozlov , doing a lazy restored state comparison now to align with existing AVX handling.

_cpuid_info.sefsl1_cpuid7_edx.bits.apx_f = 0;
}

void VM_Version::get_processor_features() {

_cpu = 4; // 486 by default
Expand Down
6 changes: 3 additions & 3 deletions src/hotspot/cpu/x86/vm_version_x86.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ class VM_Version : public Abstract_VM_Version {
uint32_t dcp_cpuid4_edx; // unused currently

// cpuid function 7 (structured extended features enumeration leaf)
// eax = 7, ecx = 0
SefCpuid7Eax sef_cpuid7_eax;
SefCpuid7Ebx sef_cpuid7_ebx;
SefCpuid7Ecx sef_cpuid7_ecx;
Expand Down Expand Up @@ -617,8 +618,7 @@ class VM_Version : public Abstract_VM_Version {
// The value used to check ymm register after signal handle
static int ymm_test_value() { return 0xCAFEBABE; }
static long long egpr_test_value() { return 0xCAFEBABECAFEBABEULL; }
static void report_apx_state_restore_error() { report_vm_error(__FILE__, __LINE__,"Unsuccessful APX state restoration.\n"); }

static void report_apx_state_restore_warning();
static void get_cpu_info_wrapper();
static void set_cpuinfo_segv_addr(address pc) { _cpuinfo_segv_addr = pc; }
static bool is_cpuinfo_segv_addr(address pc) { return _cpuinfo_segv_addr == pc; }
Expand All @@ -630,7 +630,7 @@ class VM_Version : public Abstract_VM_Version {
static void set_cpuinfo_cont_addr_apx(address pc) { _cpuinfo_cont_addr_apx = pc; }
static address cpuinfo_cont_addr_apx() { return _cpuinfo_cont_addr_apx; }
// address of apx state restore error handler.
static address _apx_state_restore_error_handler;
static address _apx_state_restore_warning_handler;

static void clear_apx_test_state();

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ bool PosixSignals::pd_hotspot_signal_handler(int sig, siginfo_t* info,
}

#ifndef PRODUCT
if ((sig == SIGSEGV) && VM_Version::is_cpuinfo_segv_addr_apx(pc)) {
if ((sig == SIGSEGV || sig == SIGBUS) && VM_Version::is_cpuinfo_segv_addr_apx(pc)) {
// Verify that OS save/restore APX registers.
stub = VM_Version::cpuinfo_cont_addr_apx();
VM_Version::clear_apx_test_state();
Expand Down