Add support for ARMv8 to use AAVMF EFI firmware #880

dustins · 2024-05-17T21:00:37Z

I discovered I wasn't able to run VMs on my RK3588 (ARM64). Although ARM64 supports UEFI it wasn't able to find appropriate *.fd files because it is assumed every architecture was using OVMF. On ARM64 it should be using AAVMF as far as I can tell.

This PR adds support for using AAVMF on ARMv8 architectures and updates the documentation to reflect the code changes.

I've tested creating VMs with these changes on my ARM64 and x86_64 machines but don't have many devices to test against.

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

stgraber · 2024-05-21T19:46:43Z

doc/environment.md

@@ -36,7 +36,7 @@ Name                            | Description
 `INCUS_EXEC_PATH`               | Full path to the Incus binary (used when forking subcommands)
 `INCUS_IDMAPPED_MOUNTS_DISABLE` | Disable idmapped mounts support (useful when testing traditional UID shifting)
 `INCUS_LXC_TEMPLATE_CONFIG`     | Path to the LXC template configuration directory
-`INCUS_OVMF_PATH`               | Path to an OVMF build including `OVMF_CODE.fd` and `OVMF_VARS.ms.fd`
+`INCUS_EFI_PATH`                | Path to EFI firmware build including `*_CODE.fd` and `*_VARS.fd`


I'd probably use INCUS_EDK2_PATH instead to be a bit more specific about what we're using.

Based on your other comment about the ed2k driver would prefer edk2 be used throughout the code instead of the more generic efi?

Yeah, I think so, especially as EFI is misleading anyway given that what we're using today is UEFI, not the old EFI (which I had to deal with a long time ago ;)).

stgraber · 2024-05-21T19:50:10Z

doc/installing.md

+```{note}
+On ARM64 CPUs you need to install AAVMF instead of OVMF for UEFI to work with virtual machines.
+```
+


I don't think we should need that.

Instead we should just remove all that symlink stuff and make sure that the logic in the code knows to look into /usr/share/qemu too in this case.

stgraber · 2024-05-21T19:52:07Z

internal/server/instance/drivers/driver_qemu.go

@@ -114,40 +114,60 @@ const qemuBlockDevIDPrefix = "incus_"
 // qemuMigrationNBDExportName is the name of the disk device export by the migration NBD server.
 const qemuMigrationNBDExportName = "incus_root"

-// OVMF firmwares.
-type ovmfFirmware struct {
+// EFI firmwares.


The rework here looks good, but I think we should make a new internal/server/instance/drivers/edk2 package which would contain those definitions and possibly some functions to access it.

That would then make it easy to consume from the apparmor package too so we don't need to keep duplicating logic between the two.

I like that. Thanks for the feedback. I'll give it a go and push some new commits.

stgraber · 2024-05-30T04:52:51Z

@dustins have you had any luck with this one?

dustins · 2024-05-30T13:12:03Z

Yes, I actually am almost done just wanted to test on x86 and ARM64 before pushing. Sorry for the long delay! I think I can test tonight and then push up the changes for you to review.

stgraber · 2024-05-30T13:44:58Z

Sounds good.

We're releasing 6.2 today so that will most likely miss that release by a hair but it will be in 6.3 for sure.

- no longer needing to symlink directories in SUSE - EDK2 installation override environemtn variable now INCUS_EDK2_PATH

dustins · 2024-06-01T10:26:03Z

I still have to test this on arm64/x86_64 to check that this all still works.

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

dustins · 2024-06-02T10:37:13Z

Ok I tested this on my x86_64/arm64 boxes and I was able to launch VMs with and without secure boot.

stgraber · 2024-06-02T22:03:24Z

doc/.wordlist.txt

@@ -1,4 +1,5 @@
 AAAA
+AAVMF


The file also needs to have EDK2 added to it.

stgraber · 2024-06-02T22:03:51Z

internal/server/instance/drivers/edk2/driver_edk2.go

@@ -0,0 +1,141 @@
+package edk2


This commit is missing the Signed-off-by line

stgraber · 2024-06-02T22:04:04Z

internal/server/instance/drivers/driver_qemu.go

@@ -40,7 +40,7 @@ import (
 	"google.golang.org/protobuf/proto"


This commit is missing the Signed-off-by line

stgraber · 2024-06-02T22:04:18Z

internal/server/apparmor/instance.go

@@ -7,6 +7,7 @@ import (
 	"strings"


This commit is missing the Signed-off-by line

stgraber · 2024-06-02T22:04:27Z

doc/environment.md

@@ -36,7 +36,7 @@ Name                            | Description
 `INCUS_EXEC_PATH`               | Full path to the Incus binary (used when forking subcommands)


This commit is missing the Signed-off-by line

What's the correct way for me to resolve the commits without the Signed-off-by line?

stgraber · 2024-06-02T22:04:56Z

internal/server/instance/drivers/driver_qemu.go

-	"github.com/lxc/incus/v6/client"
+	incus "github.com/lxc/incus/v6/client"


Needless change caused by goimports.

stgraber · 2024-06-02T22:05:30Z

Looking good, just a few changes needed to get the tests to be happy!

dustins added 3 commits May 17, 2024 12:57

incus/drivers: Add support for AAVMF EFI firmware on ARM64

8b93b28

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

incus/apparmor: Add support for AAVMF EFI firmware on ARM64

84ac9a7

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

doc: Replaced OVMF references with EFI now that AAVMF is supported too

a793947

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

github-actions bot added the Documentation Documentation needs updating label May 17, 2024

dustins marked this pull request as ready for review May 17, 2024 21:03

dustins requested a review from stgraber as a code owner May 17, 2024 21:03

dustins changed the title ~~cmd/incusd: Add support for ARMv8 to use AAVMF EFI firmware~~ Add support for ARMv8 to use AAVMF EFI firmware May 17, 2024

stgraber reviewed May 21, 2024

View reviewed changes

dustins added 4 commits June 1, 2024 10:01

incus/edk2 adding new package for dealing with edk2

7898f08

incus/drivers updating to use new edk2 package

fe22c67

incus/apparmor updating to use new edk2 package

13c2eb4

docs: updated to reflect edk2 changes

dcf8840

- no longer needing to symlink directories in SUSE - EDK2 installation override environemtn variable now INCUS_EDK2_PATH

dustins added 2 commits June 2, 2024 10:21

incus/edk2: fixed inverted logic errors

9b94e1a

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

incus/drivers: removed using '-accel kvm' for arm64 qemu feature check

898a6a7

Signed-off-by: Dustin Sweigart <dustins@swigg.net>

stgraber reviewed Jun 2, 2024

View reviewed changes

doc/.wordlist.txt

@@ -1,4 +1,5 @@

AAAA

AAVMF

Copy link

Member

stgraber Jun 2, 2024

Choose a reason for hiding this comment

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

The file also needs to have EDK2 added to it.

stgraber reviewed Jun 2, 2024

View reviewed changes

internal/server/instance/drivers/edk2/driver_edk2.go

@@ -0,0 +1,141 @@

package edk2

Copy link

Member

stgraber Jun 2, 2024

Choose a reason for hiding this comment

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

This commit is missing the Signed-off-by line

stgraber reviewed Jun 2, 2024

View reviewed changes

internal/server/apparmor/instance.go

@@ -7,6 +7,7 @@ import (

"strings"

Copy link

Member

stgraber Jun 2, 2024

Choose a reason for hiding this comment

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

This commit is missing the Signed-off-by line

stgraber reviewed Jun 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ARMv8 to use AAVMF EFI firmware #880

Add support for ARMv8 to use AAVMF EFI firmware #880

dustins commented May 17, 2024 •

edited

stgraber May 21, 2024

dustins May 21, 2024

stgraber May 22, 2024

stgraber May 21, 2024

stgraber May 21, 2024

dustins May 21, 2024 •

edited

stgraber May 22, 2024

stgraber commented May 30, 2024

dustins commented May 30, 2024

stgraber commented May 30, 2024

dustins commented Jun 1, 2024

dustins commented Jun 2, 2024

stgraber Jun 2, 2024

stgraber Jun 2, 2024

stgraber Jun 2, 2024

stgraber Jun 2, 2024

stgraber Jun 2, 2024

dustins Jun 9, 2024

stgraber Jun 2, 2024

stgraber commented Jun 2, 2024

		@@ -40,7 +40,7 @@ import (
		"google.golang.org/protobuf/proto"

		@@ -36,7 +36,7 @@ Name \| Description
		`INCUS_EXEC_PATH` \| Full path to the Incus binary (used when forking subcommands)

		"github.com/lxc/incus/v6/client"
		incus "github.com/lxc/incus/v6/client"

Add support for ARMv8 to use AAVMF EFI firmware #880

Are you sure you want to change the base?

Add support for ARMv8 to use AAVMF EFI firmware #880

Conversation

dustins commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustins May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stgraber commented May 30, 2024

dustins commented May 30, 2024

stgraber commented May 30, 2024

dustins commented Jun 1, 2024

dustins commented Jun 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stgraber commented Jun 2, 2024

dustins commented May 17, 2024 •

edited

dustins May 21, 2024 •

edited