Page MenuHomeFreeBSD

Add a new uefi-edk2-qemu port.
ClosedPublic

Authored by bcran on Apr 9 2019, 9:36 PM.

Details

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?

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bcran added inline comments.Apr 10 2019, 2:34 PM
sysutils/uefi-edk2-qemu/Makefile
12 ↗(On Diff #56026)

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 added inline comments.Apr 10 2019, 2:37 PM
sysutils/uefi-edk2-qemu/Makefile
12 ↗(On Diff #56026)

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.:

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

emaste added inline comments.Apr 10 2019, 2:41 PM
sysutils/uefi-edk2-qemu/Makefile
12 ↗(On Diff #56026)

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.

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.

manu added a reviewer: manu.Apr 10 2019, 2:48 PM
emaste added inline comments.Apr 10 2019, 2:50 PM
sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c
7–8 ↗(On Diff #56026)

Is this (going/already) upstream?

bcran updated this revision to Diff 56053.Apr 10 2019, 3:12 PM
  • Rename the port and files to include a -x86_64 suffix

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

  • 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 added inline comments.Apr 10 2019, 3:16 PM
sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c
7–8 ↗(On Diff #56026)

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 updated this revision to Diff 56054.Apr 10 2019, 3:18 PM
  • Rename port
bcran added a comment.Apr 10 2019, 3:19 PM

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

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

araujo accepted this revision.Apr 10 2019, 5:03 PM

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
This revision is now accepted and ready to land.Apr 10 2019, 5:03 PM
manu requested changes to this revision.Apr 10 2019, 5:12 PM

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/ ?

This revision now requires changes to proceed.Apr 10 2019, 5:12 PM
jhb added a comment.Apr 10 2019, 5:32 PM

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 added a subscriber: bapt.Apr 10 2019, 5:39 PM
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 added a comment.Apr 10 2019, 6:03 PM

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 added a comment.Apr 10 2019, 6:07 PM
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 added inline comments.Apr 11 2019, 3:32 AM
sysutils/uefi-edk2-qemu/Makefile
12 ↗(On Diff #56026)

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 updated this revision to Diff 56082.Apr 11 2019, 3:36 AM
  • 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 marked an inline comment as done.Apr 11 2019, 3:37 AM
bcran added inline comments.
sysutils/uefi-edk2-qemu-x86_64/Makefile
27 ↗(On Diff #56054)

Fixed.

emaste added inline comments.Apr 11 2019, 1:05 PM
sysutils/Makefile
1413 ↗(On Diff #56082)

this needs to be updated

bcran marked an inline comment as done.Apr 13 2019, 11:24 PM
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.

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.

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 updated this revision to Diff 56190.Apr 14 2019, 5:02 AM
  • Add support for building i386 as a FLAVOR
  • Install files to share/uefi-edk2-qemu instead of uefi-firmware
bcran edited the summary of this revision. (Show Details)Apr 14 2019, 5:04 AM
bcran marked an inline comment as done.
bcran added inline comments.
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 accepted this revision.Apr 15 2019, 2:26 AM

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

manu added a comment.Apr 15 2019, 5:35 PM

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
75 ↗(On Diff #56190)

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 marked an inline comment as done.Apr 15 2019, 10:48 PM
bcran added inline comments.
sysutils/uefi-edk2-qemu/Makefile
75 ↗(On Diff #56190)

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.

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 updated this revision to Diff 56232.Apr 15 2019, 10:50 PM
  • Fix portlint errors
bcran added a subscriber: mat.EditedApr 19 2019, 2:42 AM
bcran updated this revision to Diff 56371.
  • 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.
bcran marked 13 inline comments as done.Apr 19 2019, 3:19 AM
manu accepted this revision.Apr 19 2019, 7:31 AM

LGTM

This revision is now accepted and ready to land.Apr 19 2019, 7:31 AM
tsoome accepted this revision.Apr 19 2019, 7:33 AM
araujo accepted this revision.Apr 19 2019, 7:46 AM

double accept this revision!

mat added inline comments.Apr 21 2019, 7:12 AM
sysutils/uefi-edk2-qemu/Makefile
61 ↗(On Diff #56371)

This is already on line 48.

63 ↗(On Diff #56371)

This is already on line 49.

65 ↗(On Diff #56371)

Remove this, not needed.

81 ↗(On Diff #56371)

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

bcran marked 4 inline comments as done.Apr 21 2019, 5:49 PM
bcran added inline comments.
sysutils/uefi-edk2-qemu/Makefile
81 ↗(On Diff #56371)

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

mat added inline comments.Apr 23 2019, 5:37 PM
sysutils/uefi-edk2-qemu/Makefile
48 ↗(On Diff #56371)

Why +=?

bcran marked 2 inline comments as done.Apr 23 2019, 5:49 PM
bcran added inline comments.
sysutils/uefi-edk2-qemu/Makefile
48 ↗(On Diff #56371)

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 marked an inline comment as done.Apr 26 2019, 1:37 PM
bcran updated this revision to Diff 56704.
  • Remove duplicated and unneeded lines.
This revision now requires review to proceed.Apr 26 2019, 1:37 PM
bcran updated this revision to Diff 56705.Apr 26 2019, 1:39 PM
  • MAKE_ARGS should be =, not += .
bcran added inline comments.Apr 26 2019, 1:42 PM
sysutils/uefi-edk2-qemu/files/patch-CryptoPkg_Library_OpensslLib_openssl_crypto_uid.c
7–8 ↗(On Diff #56026)

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 accepted this revision.Apr 30 2019, 7:14 AM

Approved with the changes requested.

sysutils/uefi-edk2-qemu/Makefile
16 ↗(On Diff #56705)

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

33–43 ↗(On Diff #56705)

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

This revision is now accepted and ready to land.Apr 30 2019, 7:14 AM
This revision was automatically updated to reflect the committed changes.
bcran added a comment.Apr 30 2019, 4:43 PM
In D19869#432543, @mat wrote:

Approved with the changes requested.

Thanks for your help @mat !