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

[Sandhya Viswanathan]

On Mon, 1 Apr 2024 12:01:27 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Summary of changes include with the patch:-
> 
> 1) CPUID based feature detection check for Intel APX extension (https://www.intel.com/content/www/us/en/developer/articles/technical/advanced-performance-extensions-apx.html)
> 2) Validation during VM initialization for extended GPRs state save / restoration by OS across context switches of java application threads executing JIT compiled code with new APX ISA.
> 
> Kindly review and share your feedback.
> 
> Best Regards,
> Jatin

Marked as reviewed by sviswanathan (Reviewer).

src/hotspot/cpu/x86/vm_version_x86.cpp line 113:

> 111:   VM_Version_StubGenerator(CodeBuffer *c) : StubCodeGenerator(c) {}
> 112: 
> 113:   address clear_apx_test_state() {

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.

src/hotspot/cpu/x86/vm_version_x86.cpp line 433:

> 431:     __ jcc(Assembler::notEqual, vector_save_restore);
> 432: 
> 433:     /* FIXME: Uncomment after integration of JDK-8328998

Did you mean to uncomment these now that JDK-8328998 has integrated?

src/hotspot/cpu/x86/vm_version_x86.cpp line 434:

> 432: 
> 433:     /* FIXME: Uncomment after integration of JDK-8328998
> 434:     __ mov64(r15, VM_Version::egpr_test_value());

Why are we modifying r15? It is not an APX egpr.

src/hotspot/cpu/x86/vm_version_x86.cpp line 435:

> 433:     /* FIXME: Uncomment after integration of JDK-8328998
> 434:     __ mov64(r15, VM_Version::egpr_test_value());
> 435:     __ mov64(r16, VM_Version::egpr_test_value());

You would need to temporarily set UseAPX feature before generating this instruction, otherwise assembler will complain.

src/hotspot/cpu/x86/vm_version_x86.cpp line 447:

> 445:     /* FIXME: Uncomment after integration of JDK-8328998
> 446:     __ mov64(rax, VM_Version::egpr_test_value());
> 447:     __ cmpq(rax, r15);

Likewise r15 validation can be removed.

src/hotspot/cpu/x86/vm_version_x86.cpp line 456:

> 454:     // Generate SEGV to signal unsuccessful save/restore.
> 455:     __ bind(apx_save_restore_error);
> 456:     __ lea(rax, ExternalAddress(VM_Version::_apx_state_restore_error_handler));

Generating an error message here won't be the right thing (especially since this is default by feature detection). It should only result in setting UseAPX feature to false.

src/hotspot/cpu/x86/vm_version_x86.hpp line 476:

> 474:     uint32_t     dcp_cpuid4_edx; // unused currently
> 475: 
> 476:     // cpuid function 7 (structured extended features enumeration leaf)

Good to add here a comment:
// eax = 7, ecx = 0

src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp line 420:

> 418: 
> 419: #ifndef PRODUCT
> 420:     if ((sig == SIGSEGV) && VM_Version::is_cpuinfo_segv_addr_apx(pc)) {

Do we want to include SIGBUS also here like above?

-------------

PR Review: https://git.openjdk.org/jdk/pull/18562#pullrequestreview-2097590632
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1626760270
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1628196385
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1626759049
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1628197767
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1626759263
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1628208548
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1626753662
PR Review Comment: https://git.openjdk.org/jdk/pull/18562#discussion_r1626756465