-
Notifications
You must be signed in to change notification settings - Fork 710
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 feature detection for FreeBSD #892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatively recently I've changed my policies so as to be more liberal in accepting ports to architectures and operating systems I don't generally use, so in general I'm now happy to accept a change like this. However, I do think we need to insist on having such ports include the necessary infrastructure for people to test the port independently of any help from the submitter.
Are you trying to get FreeBSD + Raspberry Pi working? If so, which Raspberry Pi?
Could you please add a script that I can run locally on Linux (and/or macOS) x86_64 that can be used to test this code, e.g. in QEMU?
And/or, if you are testing this directly on hardware, could you provide step-by-step instructions, "Purchase this device, do this to install the OS, do this to build ring's test suite for the device, do this to run the tests on the device."
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240037 doesn't look very encouraging as far as getting a QEMU-based testing setup working.
crypto/cpu-aarch64.c
Outdated
// | ||
// Only FreeBSD uses this right now. | ||
|
||
#ifdef __FreeBSD__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the best mechanism that FreeBSD has for this? They don't support the getauxval()
API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elf_aux_info
(like getauxval
but slightly different call signature - with a buffer length) was not present on aarch64 for a long time. It is there in -CURRENT now, but not in any release.
This mechanism is better, it seems to be what ARM intended, it's more similar to x86 cpuid. Linux supports it since 4.11 too.
crypto/cpu-aarch64.c
Outdated
// Only FreeBSD uses this right now. | ||
|
||
#ifdef __FreeBSD__ | ||
#pragma GCC diagnostic ignored "-Wgnu-statement-expression" |
There was a problem hiding this comment.
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 suppress this warning? It would be best to avoid needing to suppress it. If that's not practical, we should have a comment documenting why not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because of the FreeBSD armreg.h
macros, we won't need this if I switch to directly using assembly
crypto/cpu-aarch64.c
Outdated
|
||
uint64_t id_aa64isar0 = READ_SPECIALREG(ID_AA64ISAR0_EL1); | ||
|
||
if (ID_AA64ISAR0_AES(id_aa64isar0) >= ID_AA64ISAR0_AES_BASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to follow the Google BoringSSL convention of always using braces.
crypto/cpu-aarch64.c
Outdated
#pragma GCC diagnostic ignored "-Wgnu-statement-expression" | ||
#include <GFp/cpu.h> | ||
#include <GFp/arm_arch.h> | ||
#include <machine/armreg.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would require us to have system header files available just to build for this target, which is something I've been trying to avoid. Maybe that's unavoidable?
READ_SPECIALREG
is a macro that expands to inline asm. Maybe we should just have that inline asm here that exposes READ_SPECIALREG
as a function callable from Rust, and then do the rest in Rust? IIUC this is ARM's ABI, so it's not going to change out from underneath us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea.
I'm on a full aarch64 desktop (Marvell MACCHIATObin + AMD GPU), typing on it right now :) As for testing, EC2 a1 instances are super easy if you have an AWS account.
|
I believe I have an EspessoBin sitting around. Is AAarch64 ported to that? If so, I could try that. AWS is a little unrealistic because I'm not going to rent a (vritual) machine to test this over time. |
Yeah, that works, you can boot the generic memstick image, but you need a DTB built from our tree (here I just built one) and here's an example u-boot command to boot. (That example uses USB, commands are a bit different for SD, and Also, for RPi3 and Pine64(-LTS), their specific memstick images should just work. https://download.freebsd.org/ftp/snapshots/arm64/aarch64/ISO-IMAGES/13.0/ |
…hod) I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
79fd0f8
to
300516a
Compare
I suggest we pick Raspberry Pi 3 or 4 or EspressoBin if that works for you. Then the step-by-step instructions would include basically how to build the SD card image for FreeBSD from Linux starting with the |
dang, you're really serious about this testing thing, why? Most projects, like Rust itself, do not test on every os/arch combination but do merge the support.
You don't produce anything, just write the existing one o_0
Why would you not build everything on the device?? 0_o This is the most step-by-step I can get (for RPi 3):
|
To cut it short, I think this is a better way than that.
I have a 32 core ~4.0ghz x86-64 machine that runs an operating system I'm familiar with, which is much faster than the espressobin that I've booted like once :) |
Can this be merged? I do not quite understand what the necessary condition to get this accepted is. It has been ~8 months since there was any activity here. PR still applies cleanly. |
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@532914 35697150-7ecd-e111-bb59-0022644237b5
security/cloak: Unbreak on aarch64 The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 Approved by: ports-secteam blanket
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@532942 35697150-7ecd-e111-bb59-0022644237b5
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892
www/zola: Unbreak on aarch64 The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 Approved by: ports-secteam blanket
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@532914 35697150-7ecd-e111-bb59-0022644237b5
The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 git-svn-id: svn+ssh://svn.freebsd.org/ports/head@532942 35697150-7ecd-e111-bb59-0022644237b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards using #1007 instead of this one due to the concerns I raise in my other comments. WDYT?
|
||
uint64_t GFp_aarch64_read_isar0(void) { | ||
uint64_t val; | ||
__asm __volatile("mrs %0, ID_AA64ISAR0_EL1" : "=&r" (val)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a std::arch
intrinsic for this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but stdarch
uses this for its own detection:
(but that's nightly only right now)
} | ||
|
||
let mut features = 0; | ||
let aa64isar0 = unsafe { GFp_aarch64_read_isar0() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know it is safe to execute the mrs
instruction?
My understanding is that a conservative person should use getauxval()
(or equivalent) to determine whether it is safe to execute mrs
. Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's guaranteed to be safe on FreeBSD 12.0 and newer.
HWCAP
auxval (#1007) has been merged into 12.2, so I guess it's fine now too. (Used to only be available in -CURRENT.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the respond. I am really liking the idea of using getauxval
because then it works just like Linux and we can avoid adding this tiny chunk of C code, so I think we should continue with #1007.
Thanks for your PR. I'm going to close this as a duplicate of #1007. To be frank, it's because it's a lot easier for me to maintain it if it shares the logic of the Linux implementation. I have nothing against FreeBSD but I don't have any FreeBSD systems. |
security/cloak: Unbreak on aarch64 The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 Approved by: ports-secteam blanket
www/zola: Unbreak on aarch64 The ring crate is missing aarch64 support on FreeBSD. Apply [1] that adds support for it. [1] briansmith/ring#892 Approved by: ports-secteam blanket
and fix build on any ARM platform that's not Linux/Fuchsia.
I hope I didn't screw up any platforms.. :D
The
all(target_os = "freebsd", target_arch = "aarch64")
inside of anall(…, any(target_arch = "arm", target_arch = "aarch64"))
is to exclude FreeBSD/arm 32-bit.cc #387