Skip to content
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.

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

Add aarch64-unknown-uefi target #85357

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

Andy-Python-Programmer
Copy link
Contributor

This pull request adds the aarch64-unknown-uefi target.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2021
@Andy-Python-Programmer Andy-Python-Programmer marked this pull request as ready for review May 16, 2021 04:55
@petrochenkov
Copy link
Contributor

This is basically a copy of x86_64_unknown_uefi.rs, right?
Was this spec tested in practice?
Some parts of it are still x86-specific, like -mmx,-sse, comments about long-mode etc.

@petrochenkov
Copy link
Contributor

What is the relation of this PR with #85358?

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2021
@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented May 17, 2021

Hello @petrochenkov! Yeup its kindof a copy of x86_64_unknown_uefi.rs.

Why add this target?

Well, I am building an operating system called Aero and currently is in the process of adding support for aarch64. As aero's bootloader is built on top of uefi we need the uefi target :D. Until then I had been using the following target script and works perfectly file though I am adding this as a target in rustc as it will be more accessible for others too :D

{
  "arch": "aarch64",
  "data-layout": "e-m:e-i64:64-f80:128-n8:16:32:64-S128",
  "default-hidden-visibility": true,
  "emit-debug-gdb-scripts": false,
  "exe-suffix": ".efi",
  "executables": true,
  "is-like-windows": true,
  "linker": "rust-lld",
  "linker-flavor": "lld-link",
  "llvm-target": "aarch64-pc-windows-msvc",
  "os": "uefi",
  "panic-strategy": "abort",
  "pre-link-args": {
    "lld-link": [
      "/subsystem:EFI_Application",
      "/entry:efi_main",
      "/machine:arm64"
    ]
  },
  "stack_probes": true,
  "target-c-int-width": "32",
  "target-endian": "little",
  "target-pointer-width": "64"
}

(edit): The following target script also seems to be used by the uefi-rs crate and redox's bootloader

@HTGAzureX1212
Copy link
Contributor

HTGAzureX1212 commented May 17, 2021

@Andy-Python-Programmer I believe we also need to write up the reasons why this new aarch64-unknown-uefi target satisfies the requirements from the Tier 3 Target Policy...? (As in a comment on my Pull Request)

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2021
@petrochenkov
Copy link
Contributor

I believe we also need to write up the reasons why this new aarch64-unknown-uefi target satisfies the requirements from the Tier 3 Target Policy...?

+1, I personally don't need to be convinced that this target is useful, but we have a formal policy now which we want to test in practice, so filling in the form from https://doc.rust-lang.org/nightly/rustc/target-tier-policy.html#tier-3-target-policy is required.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@dvdhrm dvdhrm left a comment

Choose a reason for hiding this comment

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

With the comments fixed as @petrochenkov mentioned, this looks good to me!

@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented May 25, 2021

I have been having issues while trying to test the target so I could test it on a real device and qemu. I have linked the toolchain using rustup link but when I try to run command like:

$ cargo +aarchdev build

This results in:

error: unable to unlink old fallback exe: Access is denied. (os error 5)

Updating rust fixed it once but now its back again :(

@petrochenkov
Copy link
Contributor

@Andy-Python-Programmer

error: unable to unlink old fallback exe: Access is denied. (os error 5)

Assuming the host is Windows, the error likely means that rustup is trying to remove an executable that is currently running.
I suggest checking that no rustc or cargo is currently running, perhaps trying to remove the linked toolchaing manually and re-adding it again (maybe rebuilding the custom toolchain from scratch as well).

@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented Jun 22, 2021

Sorry for the late reply, I tested building a simple UEFI application and tried to build it and it resulted in a lot of allocation errors that its not supported on COFF targets when building core.

2,3d1
<   "abi-return-struct-as-int": true,
<   "allows-weak-linkage": false,
5d2
<   "code-model": "large",
7c4
<   "disable-redzone": true,
---
>   "default-hidden-visibility": true,
11,12d7
<   "is-builtin": true,
<   "is-like-msvc": true,
16,19c11
<   "linker-is-gnu": false,
<   "lld-flavor": "link",
<   "llvm-target": "aarch64-unknown-windows",
<   "max-atomic-width": 64,
---
>   "llvm-target": "aarch64-pc-windows-msvc",
24c16
<       "/NOLOGO",
---
>       "/subsystem:EFI_Application",
26,32d17
<       "/subsystem:efi_application",
<       "/machine:arm64"
<     ],
<     "msvc": [
<       "/NOLOGO",
<       "/entry:efi_main",
<       "/subsystem:efi_application",
36,40c21,23
<   "singlethread": true,
<   "split-debuginfo": "packed",
<   "stack-probes": {
<     "kind": "call"
<   },
---
>   "stack_probes": true,
>   "target-c-int-width": "32",
>   "target-endian": "little",
42c25
< }
\ No newline at end of file
---
> }

Diff from the file that I provided in the above comment and the one that target tend to generate.

<unknown>:0: error: relocation type :abs_g2_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g3: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g0_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g1_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g2_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g3: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g0_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g1_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g2_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g3: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g0_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g1_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g2_nc: unsupported on COFF targets
<unknown>:0: error: relocation type :abs_g3: unsupported on COFF targets
...

@petrochenkov
Copy link
Contributor

@Andy-Python-Programmer
Do the relocation errors happen with the custom target from #85357 (comment)?

If not then I suggest to start explicitly adding options from the custom target to the builtin target until it works, then explicitly add all options from uefi_msvc_base with default values if necessary (in other words, find the exact source of the error).

@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented Jun 22, 2021

Do the relocation errors happen with the custom target from #85357 (comment)?

Nope. It works perfectly fine...

@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented Jul 13, 2021

After a bit of research on the allocation errors, apparently it’s because I have set the code model to large. If I remove the explicit setter for the code model in the target it apparently compiles successfully.

Since the code model only applies to the code and not the data and the code model only applies to functions you call through using call, jmp and data with lea, etc… If you are calling functions using the function pointers from the UEFI structures the code model does not apply in that case. It’s just related to the address space size of your own binary. And I am probably not getting the relocation errors if I set the code model to small as rust probably does not emit the relocations.

Since UEFI (uefi is all relocatable) uses relocatable PEs and AFAIK relocatable code does not care about the code model.

After this research 🧐, I think it’s perfectly fine to use the small code model in this case (I guess I was wrong about the need of code model set to large here). CC @dvdhrm and @petrochenkov. I suppose it will be perfectly fine to use the small code model in the x86_64 UEFI target too.

Applications don't usually take gigabytes of memory though, so the defaults are different and removing this should result in better codegen (comparable with majority of other targets).

Makes sense now, I believe, should set to use the code model to small instead.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@Andy-Python-Programmer
Could you mark this PR as waiting on review when it's ready?
https://rustc-dev-guide.rust-lang.org/rustbot.html#issue-relabeling

@Andy-Python-Programmer
Copy link
Contributor Author

Andy-Python-Programmer commented Jul 20, 2021

Sure. Just doing some final tests and checks.

@JohnCSimon
Copy link
Member

Ping from triage:
@Andy-Python-Programmer Can you please address the failing tests and then set the
PR to S-waiting-on-reviewer and remove S-waiting-on-author

Thank you!

* This commit adds the aarch64-unknown-uefi target and also adds it into
the supported targets list under the tier-3 target table.
* Uses the small code model by default

Signed-off-by: Andy-Python-Programmer <andypythonappdeveloper@gmail.com>
@Andy-Python-Programmer
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2021
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 9, 2021

📌 Commit 44b81fb has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2021
@bors
Copy link
Contributor

bors commented Aug 9, 2021

⌛ Testing commit 44b81fb with merge ae90dcf...

@bors
Copy link
Contributor

bors commented Aug 9, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing ae90dcf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 9, 2021
@bors bors merged commit ae90dcf into rust-lang:master Aug 9, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 9, 2021
@Andy-Python-Programmer Andy-Python-Programmer deleted the aarch64_uefi_target branch August 9, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.