1517594 - Don't use mrs instruction in aarch64-windows

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[(Away)]

Switching aarch64-windows builds to clang-cl makes us start going down the `#ifdef __aarch64__` codepath in https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/js/src/jit/arm64/vixl/Cpu-vixl.cpp#60.

The build then crashes on that mrs instruction (is it privileged on Windows or something?).

But Windows builds don't care about the result of GetCacheType(), so we might as well just return 0 under clang-cl.

Comment 1

[(Away)]

Attachment: Don't use mrs instruction in aarch64-windows

Comment 2

[Lars T Hansen [:lth]]

I'm curious to hear more details here - the build "crashes" how?  During compilation or (I assume) during execution?

The instruction, if properly encoded, should not be privileged, but what is the precise encoding?

In general, it's Bad to return 0 here (major performance implications) so in an ideal world we'd simply not call this if its result is not going to be used, and MOZ_CRASH along the "return 0" path.

Comment 3

[Nicolas B. Pierron [:nbp]]

The ARM64 documentation mention that the decoding of the instruction "call":

> CheckSystemAccess(bits(3) op1)
>     // Perform the generic checks that an AArch64 MSR/MRS/SYS instruction is valid at the
>     // current exception level, based on the opcode's 'op1' field value.
>     // Further checks for enables/disables/traps specific to a particular system register
>     // or operation will be performed in System_Put(), System_Get(), SysOp_W(), or SysOp_R().

However, the instruction is using the Exception Level 0, so I would not expect it to fail at runtime.

(In reply to David Major [:dmajor] from comment #0)
> But Windows builds don't care about the result of GetCacheType(), so we
> might as well just return 0 under clang-cl.

Looking at the usage of icache_line_size_ and dcache_line_size_, which are only used within CPU::EnsureIAndDCacheCoherency, it seems that we have a special code path on Windows & ARM64:

  https://searchfox.org/mozilla-central/source/js/src/jit/arm64/vixl/MozCpu-vixl.cpp#35

So I would be ok if we replicate the same guard in GetCacheType. I do not expect us to JIT cache-line optimization yet soon.

Comment 4

[(Away)]

(In reply to Lars T Hansen [:lth] from comment #2)
> I'm curious to hear more details here - the build "crashes" how?  During
> compilation or (I assume) during execution?

You caught me... I was being vague because I don't quite remember the details from before the holidays, and I still need to get set back up before I can replicate it again.

It was during execution, and I *think* that windbg complained of an illegal instruction, but I didn't want to say the wrong thing and mislead. I can find the exact encoding at some point.

As nbp noted, Windows builds go down a special case codepath that doesn't use the cache line size variables.

Comment 5

[(Away)]

Ok, here's the instruction:

(22f0.e34): Illegal instruction - code c000001d (first chance)
(22f0.e34): Illegal instruction - code c000001d (!!! second chance !!!)
xul!vixl::CPU::SetUp+0x4:
00007ffc`60587688 d53b0028 mrs x8,ctr_el0

Anything obviously wrong?

Comment 6

[Lars T Hansen [:lth]]

No, nothing seems wrong - the encoding seems correct for reading CTR_EL0 according to the ARMv8 manual.

The only other thing I've been able to find so far is that there's a comment in the pseudocode for CheckSystemAccess() that says

// Further checks for enables/disables/traps specific to a particular system register
// or operation will be performed in System_Put(), System_Get(), SysOp_W(), or SysOp_R()

which suggests that the system might further restrict access. Indeed, looking in D8.2.21, I find that access to CTR at EL0 is under some configuration bit control, suggesting Windows may choose to limit access (and has so chosen). Since the config registers can't usually be read from EL0 I don't know that we can detect this situation without trying to read CTR_EL0 and handling the exception.

(OK, duh, the comment is what :nbp cited earlier. I guess the new finding here is that it is possible for the system to restrict access to the register. That may be OK on Windows but makes me worried about what we may find in various other environments.)

Comment 7

[(Away)]

Alright, I'll go ahead with the current workaround then.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bda1e5223922
Don't use mrs instruction in aarch64-windows  r=nbp

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/bda1e5223922

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/402d96c127ef
[components] Fix 1517594: Document the events ping