Add a new uefi-edk2-qemu port.

[bcran]

Summary

This builds the x64 and i386 EDK2 OVMF firmware for the QEMU emulator, installing
the files QEMU_UEFI_CODE-${arch}.fd and QEMU_UEFI_VARS-${arch}.fd.
These can be used with the pflash device to separate the firmware into
readonly firmware and read-write persistent variable storage areas.

Note: I named the files QEMU_UEFI_*.fd to follow the existing scheme used by bhyve. I'm not sure if we want to mention OVMF instead, since that's what we build?

[bcran]

sysutils/uefi-edk2-qemu/Makefile

13

For now, OVMF only builds i386 (IA32) and X64 configurations in upstream edk2. But I agree we should allow both to install simultaneously, so perhaps installing QEMU_OVMF_CODE_${ARCH}.fd ?

[araujo]

sysutils/uefi-edk2-qemu/Makefile

13

I don't follow also, if your host is AMD64 or ARM64, why would you need to install both?

If it is for test purpose, you can crosscompile without any problem.

Ex.:

• https://filipenf.wordpress.com/2011/10/18/cross-compiling-for-x86-in-freebsd-64-bit/
• https://www.textplain.net/tutorials/2015/cross-compiling-freebsd-ports-for-the-beaglebone-black/

OR are you suggesting have:
uefi-edk2-qemu-x86_64
uefi-edk2-qemu-arm64

[emaste]

sysutils/uefi-edk2-qemu/Makefile

13

There are arm64 OVMF builds, if not in upstream, for example:
https://wiki.freebsd.org/arm64/QEMU
https://github.com/tianocore/tianocore.github.io/wiki/ArmPlatformPkg-AArch64

Anyhow I'm not suggesting we address that right now, just prepare for the future. Adding ${ARCH} to the installed files seems reasonable, IMO we should also have the ${ARCH} in the package name though.

[emaste]

OR are you suggesting have:
uefi-edk2-qemu-x86_64
uefi-edk2-qemu-arm64

Yes, exactly this. On the same (amd64) host I can run qemu-system-x86_64 -bios foo ... and qemu-system-aarch64 -bios bar ...; as far as QEMU is concerned the file installed by this proposed port is just a binary blob, it does not have any direct relationship with the host arch.

[emaste]

sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c

8–9

Is this (going/already) upstream?

[bcran]

• Rename the port and files to include a -x86_64 suffix

[emaste]

Conceptually this LGTM, but I will defer to ports folks for implementation details related to MD/MI ports.

[araujo]

In D19869#426696, @bcran wrote:

• Rename the port and files to include a -x86_64 suffix

I think you will need to rename all the port, not only the PORTNAME.

[bcran]

sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c

8–9

It's not something I've worked on, though re-adding that change when checking out upstream is getting old so I'd like to see it (or a better fix) in upstream code.
Probably not something I'll have the bandwidth to pursue for the time being though.

[bcran]

• Rename port

[bcran]

In D19869#426706, @araujo wrote:

I think you will need to rename all the port, not only the PORTNAME.

Oh, yeah. I've now renamed the directory too.

[araujo]

LGTM!

sysutils/uefi-edk2-qemu-x86_64/Makefile

27 ↗

(On Diff #56054)

Could you please add a new line between these two files? Pretty much like on BUILD_DEPENDS.

PLIST_FILES= ${PREFIX}/share/uefi-firmware/QEMU_UEFI_CODE${PLIST_SUFFIX}-x86_64.fd \
                            ${PREFIX}/share/uefi-firmware/QEMU_UEFI_VARS${PLIST_SUFFIX}-x86_64.fd

[manu]

As said on irc we want multiple version of this package for each arch.
Since we also want all the edk2 arch available for all FreeBSD arch the best way is to convert the port to a master/slave scheme.
You will have sysutils/uefi-edk2-qemu as the master port and sysutils/uefi-edk2-qemu-${ARCH} for each available arch.
The slave ports will select which compiler to use based on the host and target.

As for the installed file I don't know which is better between :
${LOCALBASE}/share/uefi-firmware-${ARCH} and ${LOCALBASE}/share/uefi-firmware/<filename>-${ARCH}
But uefi-firmware seems vague no ? maybe ${LOCALBASE}/share/uefi-edk2-qemu/ ?

[jhb]

Or instead of slave ports, flavors, where each target arch was a separate flavor. In that case you would keep the port name as it was and just append ARCH to the page name as it's flavor name.

[manu]

In D19869#426773, @jhb wrote:

Or instead of slave ports, flavors, where each target arch was a separate flavor. In that case you would keep the port name as it was and just append ARCH to the page name as it's flavor name.

I don't think that FLAVORS are there to represent arches, the result file are the same, it's just for different arches.
I once asked @bapt if it would make sense to convert the u-boot master/slave ports to a FLAVORS one and he said that even if it would work this is not what flavors are intended to be used for.

[jhb]

In this case though it isn't the host arch (ARCH) but a target architecture for the binary blob. That does seem like something flavors would handle, but slave ports also work. I thought part of the point of flavors was to make it easier to not have slave ports by encapsulating all the slave ports in a single port.

[bcran]

In D19869#426769, @manu wrote:

As for the installed file I don't know which is better between :
${LOCALBASE}/share/uefi-firmware-${ARCH} and ${LOCALBASE}/share/uefi-firmware/<filename>-${ARCH}
But uefi-firmware seems vague no ? maybe ${LOCALBASE}/share/uefi-edk2-qemu/ ?

Hmm, you're right, maybe share/uefi-firmware will start getting really cluttered soon.

[bcran]

sysutils/uefi-edk2-qemu/Makefile

13

It turns out the BaseTools makefiles were looking for 'aarch64' and not 'arm64' - our 'uname -m' output was causing it to match against 'arm' and try and build as 32-bit.
I've submitted a patch against upstream git master, but we'll need to include a patch in the port for now since we're frozen at edk2-stable201903.

[bcran]

• Add port to sysutils Makefile and split PLIST_FILES over two lines.

Note I still need to work on adding support for the FLAVORS etc.

[bcran]

sysutils/uefi-edk2-qemu-x86_64/Makefile

27 ↗

(On Diff #56054)

Fixed.

[emaste]

sysutils/Makefile

1413 ↗

(On Diff #56082)

this needs to be updated

[bcran]

In D19869#426787, @jhb wrote:

In this case though it isn't the host arch (ARCH) but a target architecture for the binary blob. That does seem like something flavors would handle, but slave ports also work. I thought part of the point of flavors was to make it easier to not have slave ports by encapsulating all the slave ports in a single port.

From what I can see, FLAVORS would work great for the two architectures OVMF runs on, X64 and IA32, but it doesn't work in the situation that someone wants to run X64 OVMF on qemu on their physical ARM64 machine. For that I think I need to figure out the set of cross-tools (aarch64 -> x64) that are needed and add them to the main Makefile.
I've made some progress on that: a BaseTools fix is now upstream (on master), and next will probably be creating a gcc-x64 port to cross-compile OvmfPkg.

[bcran]

In D19869#427632, @bcran wrote:

In D19869#426787, @jhb wrote:

In this case though it isn't the host arch (ARCH) but a target architecture for the binary blob. That does seem like something flavors would handle, but slave ports also work. I thought part of the point of flavors was to make it easier to not have slave ports by encapsulating all the slave ports in a single port.

From what I can see, FLAVORS would work great for the two architectures OVMF runs on, X64 and IA32, but it doesn't work in the situation that someone wants to run X64 OVMF on qemu on their physical ARM64 machine. For that I think I need to figure out the set of cross-tools (aarch64 -> x64) that are needed and add them to the main Makefile.
I've made some progress on that: a BaseTools fix is now upstream (on master), and next will probably be creating a gcc-x64 port to cross-compile OvmfPkg.

But, I think I'd like to have the new port committed before I finish that work, with ONLY_FOR_ARCHS=x64 remaining for the time being, since it's work I'd like to schedule and work on separately.

[emaste]

In D19869#427634, @bcran wrote:

But, I think I'd like to have the new port committed before I finish that work, with ONLY_FOR_ARCHS=x64 remaining for the time being, since it's work I'd like to schedule and work on separately.

I think that's entirely fine.

[bcran]

• Add support for building i386 as a FLAVOR
• Install files to share/uefi-edk2-qemu instead of uefi-firmware

[bcran]

sysutils/Makefile

1413 ↗

(On Diff #56082)

Since the port is now using FLAVORS, I no longer need to change this (I've renamed the port back to uefi-edk2-qemu).

[araujo]

I'm fine with that too! We can have it as it is and later evolve it.

[manu]

There is a lots of portlint error :
FATAL: Makefile: extra item "FLAVORS" placed in the MAINTAINER section.
FATAL: Makefile: extra item "x86_64_PKGNAMESUFFIX" placed in the MAINTAINER section.
FATAL: Makefile: extra item "i386_PKGNAMESUFFIX" placed in the MAINTAINER section.
WARN: Makefile: COMMENT is set externally to this port's Makefile, but this port is not configured as a slave port.
FATAL: Makefile: extra item "MAINTAINER" placed in the LICENSE section.
FATAL: Makefile: extra item "COMMENT" placed in the LICENSE section.
WARN: Makefile: "LICENSE" has to appear earlier.
WARN: Makefile: "ONLY_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "BUILD_DEPENDS" has to appear earlier.
WARN: Makefile: "USES" has to appear earlier.
5 fatal errors and 5 warnings found.

sysutils/uefi-edk2-qemu/Makefile

76

Can't you use BINARY_ALIAS for all those or does the build system really expect those to be present in this sub directory ?

[bcran]

sysutils/uefi-edk2-qemu/Makefile

76

It expects them to be in that directory since it adds that to the start of $PATH. But that's no different really to what BINARY_ALIAS does. I think for now I'd prefer to keep it as it is, to make it more explicit where things are.

[bcran]

In D19869#428001, @manu wrote:

There is a lots of portlint error :
FATAL: Makefile: extra item "FLAVORS" placed in the MAINTAINER section.
FATAL: Makefile: extra item "x86_64_PKGNAMESUFFIX" placed in the MAINTAINER section.
FATAL: Makefile: extra item "i386_PKGNAMESUFFIX" placed in the MAINTAINER section.
WARN: Makefile: COMMENT is set externally to this port's Makefile, but this port is not configured as a slave port.
FATAL: Makefile: extra item "MAINTAINER" placed in the LICENSE section.
FATAL: Makefile: extra item "COMMENT" placed in the LICENSE section.
WARN: Makefile: "LICENSE" has to appear earlier.
WARN: Makefile: "ONLY_FOR_ARCHS" has to appear earlier.
WARN: Makefile: "BUILD_DEPENDS" has to appear earlier.
WARN: Makefile: "USES" has to appear earlier.
5 fatal errors and 5 warnings found.

Oops, thanks. I've fixed all but one of the warnings, which I can't remove without generating _more_ warnings.

[bcran]

• Fix portlint errors

[bcran]

• Make changes learned from feedback on D19875 from @mat .
• Improve pkg-desc.
• Move lines around to better follow handbook.
• Move toolchain setup into post-patch section.

[manu]

LGTM

[araujo]

double accept this revision!

[mat]

sysutils/uefi-edk2-qemu/Makefile

62

This is already on line 48.

64

This is already on line 49.

66

Remove this, not needed.

82

This does not do anything, each line is ran in a separate shell invocation.

[bcran]

sysutils/uefi-edk2-qemu/Makefile

82

Oh, of course. Thanks, it was left over from the bhyve port.

[mat]

sysutils/uefi-edk2-qemu/Makefile

49

Why +=?

[bcran]

sysutils/uefi-edk2-qemu/Makefile

49

I don't know, I carried it over from the bhyve port. Since I don't see any reason for it either, I'll remove it before I commit it.

[bcran]

• Remove duplicated and unneeded lines.

[bcran]

• MAKE_ARGS should be =, not += .

[bcran]

sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c

8–9

I submitted a different fix upstream: https://github.com/tianocore/edk2/commit/1a734ed85fda71630c795832e6d24ea560caf739
We'll pick it up when we update to the next stable tag, which I believe will be edk2-stable201905.

[mat]

Approved with the changes requested.

sysutils/uefi-edk2-qemu/Makefile

17

Python depends should be done on the package name, probably ${PYTHON_PKGNAMEPREFIX}sqlite3>0 here. (maybe with a more restrictive version check.)

34–44

There are local variables, they should go after the options block. See Chapter 15. Order of Variables in Port Makefiles.

[bcran]

In D19869#432543, @mat wrote:

Approved with the changes requested.

Thanks for your help @mat !