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.

Sign up for GitHub

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 arm64 support & fixes for running on /usr-merged/Fedora systems #129

Closed
wants to merge 5 commits into from
Closed

Add arm64 support & fixes for running on /usr-merged/Fedora systems #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
513a964
Add support for arm64 hosts
refi64 Jan 24, 2023
a840504
cpio: Wrap errors with %w
refi64 Jan 24, 2023
a0da57a
Fix boot failures on merged-/usr systems
refi64 Jan 24, 2023
8b28222
Fix broken busybox on recent Fedora releases
refi64 Jan 24, 2023
3eabc10
Fix SSL failures on Fedora systems
refi64 Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build linux && amd64
// +build linux,amd64
//go:build linux
// +build linux
Copy link

Choose a reason for hiding this comment

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

I'm really happy to see arm64 support. Thank you. Did you try it with Fedora only or did you also give Debian/Arch a try?

Small nit: Personally, I would add the new/supported arch instead of removing the existing restriction.

With the proposed change, one can trivially build binaries that are known broken. Which in itself is poor user experience IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine as we could also add support for currently unsupported archs ;-) - as long the error is explicit at runtime I am happy.


package fakemachine

Expand Down
26 changes: 22 additions & 4 deletions backend_qemu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build linux && amd64
// +build linux,amd64
//go:build linux
// +build linux

package fakemachine

Expand Down Expand Up @@ -35,8 +35,18 @@ func (b qemuBackend) Supported() (bool, error) {
return true, nil
}

var qemuBinaryNames = map[Arch]string{
Amd64: "qemu-system-x86_64",
Arm64: "qemu-system-aarch64",
}

func (b qemuBackend) QemuPath() (string, error) {
return exec.LookPath("qemu-system-x86_64")
binary, ok := qemuBinaryNames[b.machine.arch]
if !ok {
return "", fmt.Errorf("unsupported arch for qemu: %s", b.machine.arch)
}

return exec.LookPath(binary)
}

func (b qemuBackend) KernelRelease() (string, error) {
Expand Down Expand Up @@ -173,7 +183,7 @@ func (b qemuBackend) StartQemu(kvm bool) (bool, error) {
}
memory := fmt.Sprintf("%d", m.memory)
numcpus := fmt.Sprintf("%d", m.numcpus)
qemuargs := []string{"qemu-system-x86_64",
qemuargs := []string{qemuBinaryNames[m.arch],
Copy link
Member

Choose a reason for hiding this comment

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

why not call QemuPath() here ?

"-smp", numcpus,
"-m", memory,
"-kernel", kernelPath,
Expand All @@ -187,6 +197,14 @@ func (b qemuBackend) StartQemu(kvm bool) (bool, error) {
"-enable-kvm")
}

if m.arch == Arm64 {
Copy link
Member

Choose a reason for hiding this comment

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

did you test this with both qemu and kvm backends ?

qemuargs = append(qemuargs, "-machine", "virt")
if !kvm {
// QEMU uses a 32-bit CPU by default, so override it.
qemuargs = append(qemuargs, "-cpu", "cortex-a53")
}
}

kernelargs := []string{"console=ttyS0", "panic=-1",
"systemd.unit=fakemachine.service"}

Expand Down
9 changes: 7 additions & 2 deletions backend_uml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build linux && amd64
// +build linux,amd64
//go:build linux
// +build linux

package fakemachine

Expand Down Expand Up @@ -27,6 +27,11 @@ func (b umlBackend) Name() string {
}

func (b umlBackend) Supported() (bool, error) {
// only support amd64
if b.machine.arch != Amd64 {
return false, fmt.Errorf("unsupported arch: %s", b.machine.arch)
}

// check the kernel exists
if _, err := b.KernelPath(); err != nil {
return false, err
Expand Down
2 changes: 1 addition & 1 deletion cpio/writerhelper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (w *WriterHelper) CopyFileTo(src, dst string) error {

f, err := os.Open(src)
if err != nil {
return fmt.Errorf("open failed: %s - %v", src, err)
return fmt.Errorf("open failed: %s - %w", src, err)
}
defer f.Close()

Expand Down
64 changes: 58 additions & 6 deletions machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build linux && amd64
// +build linux,amd64
//go:build linux
// +build linux

package fakemachine

Expand Down Expand Up @@ -142,6 +142,18 @@ func realDir(path string) (string, error) {
return filepath.Dir(p), nil
}

type Arch string

const (
Amd64 Arch = "amd64"
Arm64 = "arm64"
)

var archMap = map[string]Arch{
"amd64": Amd64,
"arm64": Arm64,
}

type mountPoint struct {
hostDirectory string
machineDirectory string
Expand All @@ -155,6 +167,7 @@ type image struct {
}

type Machine struct {
arch Arch
backend backend
mounts []mountPoint
count int
Expand All @@ -181,6 +194,11 @@ func NewMachineWithBackend(backendName string) (*Machine, error) {
var err error
m := &Machine{memory: 2048, numcpus: runtime.NumCPU()}

var ok bool
if m.arch, ok = archMap[runtime.GOARCH]; !ok {
return nil, fmt.Errorf("unsupported arch %s", runtime.GOARCH)
}

m.backend, err = newBackend(backendName, m)
if err != nil {
return nil, err
Expand All @@ -199,10 +217,18 @@ func NewMachineWithBackend(backendName string) (*Machine, error) {
if _, err := os.Stat("/etc/ca-certificates"); err == nil {
m.AddVolume("/etc/ca-certificates")
}
if _, err := os.Stat("/etc/pki"); err == nil {
m.AddVolume("/etc/pki")
}
if _, err := os.Stat("/etc/ssl"); err == nil {
m.AddVolume("/etc/ssl")
}

// Mounts for gnutls (used by wget).
if _, err := os.Stat("/etc/crypto-policies"); err == nil {
Comment on lines +227 to +228
Copy link
Member

Choose a reason for hiding this comment

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

crypto-policies is not just for gnutls it's the centralized policies for all crypto :)

m.AddVolume("/etc/crypto-policies")
}

// Dbus configuration
if _, err := os.Stat("/etc/dbus-1"); err == nil {
m.AddVolume("/etc/dbus-1")
Expand Down Expand Up @@ -663,25 +689,51 @@ func (m *Machine) startup(command string, extracontent [][2]string) (int, error)
}

/* Ensure systemd-resolved is available */
if _, err := os.Stat("/lib/systemd/systemd-resolved"); err != nil {
if _, err := os.Stat(prefix + "/lib/systemd/systemd-resolved"); err != nil {
return -1, err
}

/* Amd64 dynamic linker */
err = w.CopyFile("/lib64/ld-linux-x86-64.so.2")
var glibcDynamicLinker, muslDynamicLinker string
if m.arch == Arm64 {
Copy link
Member

Choose a reason for hiding this comment

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

would it be worth adding a table of arch vs. path for these, and erroring out if it's not a supported arch (like we do for Qemu path for instance) ?

// arm64 dynamic linker is in /lib:
// https://sourceware.org/bugzilla/show_bug.cgi?id=25129
glibcDynamicLinker = prefix + "/lib/ld-linux-aarch64.so.1"
muslDynamicLinker = prefix + "/lib/ld-musl-aarch64.so.1"
} else {
glibcDynamicLinker = prefix + "/lib64/ld-linux-x86-64.so.2"
muslDynamicLinker = prefix + "/lib64/ld-musl-x86_64.so.1"
}

/* dynamic linkers */
err = w.CopyFile(glibcDynamicLinker)
if err != nil {
return -1, err
}

// Fedora's busybox needs musl
Copy link
Member

Choose a reason for hiding this comment

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

nit: keep comment style of /* */
also worth saying that muscl is optional here, it's not really clear from a first look :-)

err = w.CopyFile(muslDynamicLinker)
if err != nil && !errors.Is(err, os.ErrNotExist) {
return -1, err
}

/* C libraries */
libraryDir, err := realDir("/lib64/ld-linux-x86-64.so.2")
libraryDir, err := realDir(glibcDynamicLinker)
if err != nil {
return -1, err
}

err = w.CopyFile(libraryDir + "/libc.so.6")
if err != nil && errors.Is(err, os.ErrNotExist) && m.arch == Arm64 && strings.HasSuffix(libraryDir, "/lib") {
Copy link
Member

Choose a reason for hiding this comment

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

would it be worth making the check for arch more clear ?
or even, do we need the arch check here ?

Copy link
Member

Choose a reason for hiding this comment

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

This needs a more generic fix to get this to work on fedora.

The overall assumption this code makes is that the main library directory matches the real location of the dynamic linker.

This is true for debian where the canonical linker locations are e.g. "/lib/ld-linux-aarch64.so.1" or "/lib64/ld-linux-x86-64.so.2" are symlinks to "
aarch64-linux-gnu/ld-linux-aarch64.so.1" and "/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2" respectively..

It also works for arch as it only seems to use /lib64 on amd64 where it's just a symlink to /usr/lib anyway.

So fedora is the odd one out here in that it uses /usr/lib64 on arm64 as well and having it's dynamic linkers directly in their canonical location.. Which causes the failure on arm64 as the linker in lib but everything else in lib64

TL;DR to cover fedora (and the likes) properly the way we determine libraryDir should change/be expanded, and probably broken out to an helper, rather then patching it up after the fact

// Because the dynamic linker is in /lib, on systems with a separate
// lib64, it might actually be a in a different folder from libc. In
// that case, try using the lib64 directory.
libraryDir += "64"
err = w.CopyFile(libraryDir + "/libc.so.6")
}
if err != nil {
return -1, err
}

err = w.CopyFile(libraryDir + "/libresolv.so.2")
if err != nil {
return -1, err
Expand Down