Bug 144680 - [JSC] Erratum (843419) in Cortex-A53
Summary: [JSC] Erratum (843419) in Cortex-A53
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gabor Loki
URL:
Keywords:
Depends on:
Blocks: 108645
  Show dependency treegraph
 
Reported: 2015-05-06 03:05 PDT by Gabor Loki
Modified: 2015-05-12 01:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2015-05-06 03:12 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2015-05-11 06:22 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Loki 2015-05-06 03:05:13 PDT
If there is a load or store instruction at a page boundary which uses the result of an ADRP instruction as a base it might access an incorrect address. There is a second variant of this if the base register is written by an instruction immediately after an ADRP to the same register, might also cause incorrect address access.
Comment 1 Gabor Loki 2015-05-06 03:06:39 PDT
Although the adrp instruction is never used from ARM64Assembler.h, it is possible to add a simple workaround for this.

I will upload a patch soon.
Comment 2 Gabor Loki 2015-05-06 03:12:59 PDT
Created attachment 252464 [details]
Patch
Comment 3 Michael Saboff 2015-05-07 15:24:00 PDT
Comment on attachment 252464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252464&action=review

> Source/JavaScriptCore/assembler/ARM64Assembler.h:3668
> +        nop();

Why this third nop?  I thought that the bug was an issue only for a dependent instruction at the page boundary.  The check looks for the page boundary and inserts two nop's that should take care of the problem, making the last nop unnecessary.

> Source/JavaScriptCore/assembler/ARM64Assembler.h:3670
> +    ALWAYS_INLINE void nopCortexA53Fix843419()
> +    {
> +#if CPU(ARM64_CORTEXA53)
> +        if (UNLIKELY((m_buffer.codeSize() & 0xFF8) == 0xFF8)) {
> +            nop();
> +            nop();
> +        }
> +        nop();
> +#endif
> +    }

This nop padding needs to happen at link time.  I don't think we have any guarantees that the page offset at assemble time will be the same after we link.  Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization.
Comment 4 Gabor Loki 2015-05-08 01:49:15 PDT
There are two cases:
1) If the ADRP is near to the page boundary (last two instruction) you will need more than two instructions between the ADRP and the load or store.
2) If the ADRP is followed by an instruction which writes the result register you will need an addition register.

So, there is no unnecessary nops.

> This nop padding needs to happen at link time.  I don't think we have any guarantees that the page offset at assemble time will be the same after we link.
> Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization.

Sad to hear that. It was a long time when I last touched the JSC.

I am sure that the appropriate fix will be to check all the conditions which cause the issue, but it surely slow down the code generation - because the load and store instructions are very frequent. If we do these checks as a later pass after code generation (link time), the effect to the performance will be the same.

I think the most easiest way to fix this - which might not influence the performance much - is to generate three nops after ADRP.

How does it sound?
Comment 5 Gabor Loki 2015-05-11 06:22:55 PDT
Created attachment 252858 [details]
Patch
Comment 6 Michael Saboff 2015-05-11 07:08:40 PDT
(In reply to comment #4)
> There are two cases:
> 1) If the ADRP is near to the page boundary (last two instruction) you will
> need more than two instructions between the ADRP and the load or store.
> 2) If the ADRP is followed by an instruction which writes the result
> register you will need an addition register.
> 
> So, there is no unnecessary nops.
> 
> > This nop padding needs to happen at link time.  I don't think we have any guarantees that the page offset at assemble time will be the same after we link.
> > Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization.
> 
> Sad to hear that. It was a long time when I last touched the JSC.
> 
> I am sure that the appropriate fix will be to check all the conditions which
> cause the issue, but it surely slow down the code generation - because the
> load and store instructions are very frequent. If we do these checks as a
> later pass after code generation (link time), the effect to the performance
> will be the same.
> 
> I think the most easiest way to fix this - which might not influence the
> performance much - is to generate three nops after ADRP.
> 
> How does it sound?

Sounds fine.

As a heads up, there has been some discussion to change the current movz/movk/movk absolute address materialization to adrp/add.
Comment 7 Michael Saboff 2015-05-11 07:09:02 PDT
Comment on attachment 252858 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2015-05-12 01:16:54 PDT
Comment on attachment 252858 [details]
Patch

Clearing flags on attachment: 252858

Committed r184170: <http://trac.webkit.org/changeset/184170>
Comment 9 WebKit Commit Bot 2015-05-12 01:17:01 PDT
All reviewed patches have been landed.  Closing bug.