Gerd, the NVR above designates upstream git commit 31583f9. Please rebase to git commit aa47e52 instead (minimally):

  OvmfPkg: Convert to using FatPkg in the EDK II tree

I'm swamped by review requests, but I'll try to find the time to look at this.

Thanks!

spec updated.
New srpm uploading atm
https://kraxel.fedorapeople.org/review/edk2/edk2-0-20160408.g3dc5c1a.el7.src.rpm

srpm upload finished now.
Kicked copr build.
https://copr.fedorainfracloud.org/coprs/kraxel/edk2/build/173984/

rpmlint output:

edk2.src:31: W: macro-in-comment %ifarch
edk2.src:61: W: unversioned-explicit-provides OVMF
edk2.src:71: W: unversioned-explicit-provides AAVMF
edk2.src: W: invalid-url Source0: edk2-20160408-3dc5c1a.tar.gz
edk2-ovmf.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/edk2-ovmf-0/README
edk2-tools.x86_64: W: no-manual-page-for-binary EfiRom
edk2-tools.x86_64: W: no-manual-page-for-binary VolInfo

The OVMF and AAVMF provides are there as convenience feature,
b/c the rhel packages named that way, so 'dnf install OVMF' works.

Sorry, still swamped. :(

Two questions / requests until I manage to look at the package:

- is IPv6 support enabled? It should be, to parallel iPXE bug 1280318
  (= for SeaBIOS VMs). The OVMF build flag in question is
  "-D NETWORK_IP6_ENABLE". (AAVMF doesn't support this flag.)

- Could you please delay the packaging just a bit longer? A user reported
  an IDE PIO performance regression four days ago, for edk2, and I posted
  v2 of the patch that fixes it just this morning:

  http://thread.gmane.org/gmane.comp.bios.edk2.devel/10511/focus=10512

  We could open a separate bug for it later, but I think (hope) that patch will
  be upstream next week.

Thanks!

fc23 aarch64 pkgs added to https://kraxel.fedorapeople.org/review/edk2/
Hmm, time that copr gets some arm builders ...

(In reply to Laszlo Ersek from comment #5)
> Sorry, still swamped. :(

Added Cole to Cc b/c of that.

> Two questions / requests until I manage to look at the package:
> 
> - is IPv6 support enabled? It should be, to parallel iPXE bug 1280318
>   (= for SeaBIOS VMs). The OVMF build flag in question is
>   "-D NETWORK_IP6_ENABLE". (AAVMF doesn't support this flag.)

Not there right now, but can add that, sure.  Simple one-liner.

> - Could you please delay the packaging just a bit longer? A user reported
>   an IDE PIO performance regression four days ago, for edk2, and I posted
>   v2 of the patch that fixes it just this morning:

Updating is as simple as running the update-tarball.sh script included in the src rpm (at least as long as none of the patches needs a rebase).

So, actually committing an updated package to distgit is easy.

The review and distgit process will take some time _anyway_, so I don't think we have to delay it because of the bug.

There's already an edk2 fedora package in distgit (it just builds the tools) so the fedora admin side of the review process isn't required here. Re-assigning this to the edk2 package, I haven't looked at the changes in detail yet

FWIW those RPMs fail to build on f23, hits several errors in BaseTools. I'll attach output

Also, can you provide some comments on all the patches that are in there? Are they waiting on review, or stuff that will never be upstream, or something else?

Created attachment 1145313 [details]
Failed rebuild output

> + make -C BaseTools
> Stop using make, use the m alias! Sleeping 1...

What's this?

Also the log looks like a parallel build basetools is attempted.
That doesn't work.

> Also, can you provide some comments on all the patches that are in there?
> Are they waiting on review, or stuff that will never be upstream, or
> something else?

Most of them attempted to get upstream but failed.
Laszlo should be able to list that in detail for each patch.

Update uploaded to https://kraxel.fedorapeople.org/review/edk2/

Changes:
 - updated tarball.
 - added all C tools to -tools package, not only EfiRom and VolInfo.
 - changed versioning scheme to match current package.

/me wonders why the current package has a separate -tools-doc subpackage?

(In reply to Gerd Hoffmann from comment #11)
> > + make -C BaseTools
> > Stop using make, use the m alias! Sleeping 1...
> 
> What's this?
> 
> Also the log looks like a parallel build basetools is attempted.
> That doesn't work.

Heh, just a shell wrapper for 'make' I have to try and train myself to use an 'm' alias instead, please ignore. It may be responsible for the test errors too, I'll confirm

The separate -tools-doc subpackage was requested during the package review, the Fedora package guidelines suggest doing this if the documentation is large.

(Click "Unwrap comments" near the top, for reading this comment.)

For the spec file from "edk2-20160411git4da1ebf-0.el7.src.rpm":

> %global edk2_date        20160411
> %global edk2_githash     4da1ebf
> %global openssl_version  1.0.2g
>
> Name:           edk2
> Version:        %{edk2_date}git%{edk2_githash}
> Release:        0%{dist}
> Summary:        EFI Development Kit II
>
> Group:          Applications/Emulators
> License:        BSD

The license is incorrect; it is not pure BSD if OpenSSL (i.e., Secure Boot)
is included in the binaries. The right term is

  BSD and OpenSSL

This is based on
<https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios>,
which describes how the "and" operator can be used, and also based on
<https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses>, which lists
"OpenSSL" as short name for the OpenSSL License, and "BSD" for the BSD
License (two clause).

Namely (with the FatPkg relicensing), the License.txt files under all *Pkg
directories whose modules go into the OVMF and AAVMF binaries are the
2-clause BSDL, except for CryptoPkg/Library/OpensslLib/openssl-*/LICENSE,
which is the OpenSSL License.

Since we have two subpackages, and AAVMF won't include the Secure Boot
feature, the subpackages also need their separate License fields. And, AAVMF
should state plain "BSD".

> URL:            http://www.tianocore.org/edk2/
> Source0:        edk2-%{edk2_date}-%{edk2_githash}.tar.gz
> Source1:        https://www.openssl.org/source/openssl-%{openssl_version}.tar.gz
> Source3:        build-iso.sh
> Source9:        update-tarball.sh
>
> Patch1:         0001-pick-up-any-display-device-not-only-vga.patch

This patch is from Gerd; it is so that OVMF is willing to drive
"secondary-vga" devices of QEMU. This patch is not upstream because we
haven't submitted it yet. If Gerd submits it, I'll ACK it, but a good
argument should be made for it in the commit message. (I guess it should be
a description of the secondary-vga device, why it is useful etc.)

> Patch2:         0001-MdeModulePkg-TerminalDxe-add-other-text-resolutions.patch

This patch is un-upstreamable. We tried, we failed. It resulted in a 3-month
long off-list discussion that produced no results.

> Patch3:         0001-EXCLUDE_SHELL_FROM_FD.patch

We have this patch because Peter Jones advised us that hardware vendors
exclude the UEFI shell from firmware that contains the Secure Boot feature.
Namely, if the Secure Boot operating mode is enabled, the shell could still
be launched without problems if the shell binary was part of the system
firmware, and then the Secure Boot operating mode could be circumvented from
the shell (using internal shell commands).

Thus, we provide the UEFI shell binary on an ISO image, as an external UEFI
application. This binary is not signed, hence when the Secure Boot operating
mode is enabled, the shell can't be launched. When the Secure Boot operating
mode is disabled, the shell can be launched from the ISO image as the
default boot application.

> Patch4:         0001-OvmfPkg-disable-multi-processor-support-for-boot-tim.patch

This used to be a workaround for a UefiCpuPkg/CpuDxe bug, but it is no
longer necessary. Please drop this patch.

I vaguely suspect that we may have kept this patch to work around the
MTRR-related problems that predated Linux v4.4 (on the host), but it really
shouldn't be necessary now. I haven't been using this patch myself for a
very long time now.

> Patch10:        0001-OvmfPkg-SmbiosPlatformDxe-install-legacy-QEMU-tables.patch
> Patch11:        0002-OvmfPkg-SmbiosPlatformDxe-install-patch-default-lega.patch
> Patch12:        0003-OvmfPkg-SmbiosPlatformDxe-install-patch-default-lega.patch

These patches support the "old" SMBIOS guest interface, on machine types
that are smaller than 2.1. (Gabriel Somlo's "new" SMBIOS guest interface
appeared in QEMU 2.1, and he also wrote the OVMF patches for it.) These
patches for the "old" interface are from yours truly, and upstream never
accepted them -- Jordan asked me to factor out SMBIOS support from a number
of other modules (I think even EmulatorPkg?) as a prerequisite, which I
thought was an unreasonable request, and I abandoned upstreaming these
patches.

I guess in Fedora noone uses such old machine types (i.e., machtypes before
2.1), so I guess these could be removed, if someone is bothered by them.

>
> Patch20:        0001-OvmfPkg-EnrollDefaultKeys-application-for-enrolling-.patch

This UEFI application (also present on the ISO image that we prepare, beside
the UEFI shell binary) streamlines the enrollment of the three most common
Microsoft Secure Boot keys. It also enables the Secure Boot operating mode.

This is not upstream because (a) there are about a dozen divergent solutions
for helping with key enrollment, none of them upstream, (b) UEFI-2.6 defines
facilities (AuditMode and DeployedMode) in which the enrollment is delegated
to the first boot of the OS (very roughly). Ultimately this patch should be
replaced with userspace Linux utilities that work on physical UEFI machines
too.

> %ifarch aarch64
> %package aarch64
> Summary:        Arm Virtual Machine Firmware

I think it would be more appropriate to say AARCH64 here.

> Provides:       AAVMF
> BuildArch:      noarch
> %description aarch64
> EFI Development Kit II
> AARCH64 UEFI Firmware
> %endif

> # common features
> CC_FLAGS="${CC_FLAGS} -D HTTP_BOOT_ENABLE"
> CC_FLAGS="${CC_FLAGS} -D NETWORK_IP6_ENABLE"

These flags are OVMF specific; "ArmVirtPkg/ArmVirtQemu.dsc" doesn't know
about them.

For OVMF, they are appropriate.

Furthermore, please add the following two to the common build options:

  -b DEBUG
  --cmd-len=65536

The first makes sure we build for the DEBUG target, regardless of what
"BaseTools/Conf/target.template" says.

The second one speeds up the build. (Background: the compiler command line
can grow very large for some modules of edk2, and it brakes various windows
tools that only support 4KB command lines. This got worked around recently
in BaseTools by generating an option file (= @file) for each invocation of
the compiler, by default. This works on Linux too, but it is not necessary,
and to me it seemed to slow down the build quite a bit. The --cmd-len switch
above makes sure the @file thing won't be activated unless the command line
exceeds 64KB.)

> # secure boot build features
> SB_FLAGS="${CC_FLAGS}"
> SB_FLAGS="${SB_FLAGS} -D SECURE_BOOT_ENABLE"
> SB_FLAGS="${SB_FLAGS} -D SMM_REQUIRE"
> SB_FLAGS="${SB_FLAGS} -D EXCLUDE_SHELL_FROM_FD"
>
> # arm firmware features
> ARM_FLAGS="${CC_FLAGS}"
> ARM_FLAGS="${ARM_FLAGS} -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F"

Right, this is good.

Actually, I think DEBUG_VERBOSE should also be enabled for OVMF too; given
that the package will be available in Fedora to a large number of users,
we'll want as much debug output as possible. I have five more patches for
that (they are all downstream-only, as this kind of setting is not expected
to be massaged in upstream); I'll mail them to Gerd for the next build.

>
> make -C BaseTools #%{?_smp_mflags}

BaseTools should not be built concurrently; as discussed above.

> %ifarch aarch64
> # build arm/aarch64 firmware
> mkdir -p aarch64
> build $ARM_FLAGS -a AARCH64 -p ArmVirtPkg/ArmVirtQemu.dsc
> cp Build/ArmVirtQemu-AARCH64/DEBUG_*/FV/*.fd aarch64
> dd of="aarch64/QEMU_EFI-pflash.raw" if="/dev/zero" bs=1M count=64
> dd of="aarch64/QEMU_EFI-pflash.raw" if="aarch64/QEMU_EFI.fd" conv=notrunc

I think this could be simplified as

  cat .../QEMU_EFI.fd /dev/zero | head -c 64M > ...

but I don't feel strongly about it.

> %ifarch x86_64
> %files ovmf
> %license OvmfPkg/License.txt

Please include CryptoPkg/Library/OpensslLib/openssl-*/LICENSE here, as a
separate file.

Thanks
Laszlo

>   BSD and OpenSSL
> 
> Since we have two subpackages, and AAVMF won't include the Secure Boot
> feature, the subpackages also need their separate License fields. And, AAVMF
> should state plain "BSD".

So I guess we should attach that license to the ovmf subpackage only ...

> > make -C BaseTools #%{?_smp_mflags}
> 
> BaseTools should not be built concurrently; as discussed above.

It's commented out for that reason ;)
(And left in that way to indicate _smp_mflags wasn't simply forgotten).

New build uploaded, should have all fixed, also picked up some stuff from the existing package (separate -doc, package python tools too, changelog, ...).  Also updated to latest git again.

The spec file in "edk2-20160412gitba83f7e-0.el7.src.rpm" looks good to me.

However, I'm noticing two things I should have noticed earlier (sorry!):

- We probably should not ship the unified "/usr/share/edk2/ovmf/OVMF.fd" file,
  only the split files.

- The aarch64 build seems to have disappeared, after 2016-04-08, from
  <https://kraxel.fedorapeople.org/review/edk2>. Is this intentional?

Thanks.

(In reply to Laszlo Ersek from comment #18)
> The spec file in "edk2-20160412gitba83f7e-0.el7.src.rpm" looks good to me.
> 
> However, I'm noticing two things I should have noticed earlier (sorry!):
> 
> - We probably should not ship the unified "/usr/share/edk2/ovmf/OVMF.fd"
> file,
>   only the split files.

Hmm, why?  Can be useful for throwaway virtual machines, such as booting
a live iso with efi.  Without that you have to copy the varstore to some
temp file, boot, then delete the tmpfile even if you don't need the efi
variable persistance.

> - The aarch64 build seems to have disappeared, after 2016-04-08, from
>   <https://kraxel.fedorapeople.org/review/edk2>. Is this intentional?

The old ones are still there.  But didn't do a fresh aarch64 build, so
there are no updated aarch64 rpms.  /me goes kick a build ...

(In reply to Gerd Hoffmann from comment #19)
> (In reply to Laszlo Ersek from comment #18)
> > The spec file in "edk2-20160412gitba83f7e-0.el7.src.rpm" looks good to me.
> > 
> > However, I'm noticing two things I should have noticed earlier (sorry!):
> > 
> > - We probably should not ship the unified "/usr/share/edk2/ovmf/OVMF.fd"
> > file,
> >   only the split files.
> 
> Hmm, why?  Can be useful for throwaway virtual machines, such as booting
> a live iso with efi.  Without that you have to copy the varstore to some
> temp file, boot, then delete the tmpfile even if you don't need the efi
> variable persistance.

The setup you describe is valid for throwaway VMs; it parallels the case when QEMU is started without a writeable hard disk.

It's just that I've seen reports where a user uses -bios (or an equivalent "hand-crafted" domain XML) for VMs that are meant to be persistent. The resultant confusion is not good.

Personally, I never use the unified file, not even for throwaway VMs. I do the copying (in scripts) the way you describe. It's not a big burden, and the originally unneeded persistence actually turns out welcome in some cases.

I also found it inconsistent that the build without the Secure Boot feature provides a unified binary, while the build with the SB feature doesn't. If the argument is throwaway VMs, then I guess it does make sense (you can't enable SB without enrolling keys and rebooting the VM, hence SB only applies to non-throwaway VMs).

Anyway, up to you.

> > - The aarch64 build seems to have disappeared, after 2016-04-08, from
> >   <https://kraxel.fedorapeople.org/review/edk2>. Is this intentional?
> 
> The old ones are still there.  But didn't do a fresh aarch64 build, so
> there are no updated aarch64 rpms.  /me goes kick a build ...

Looks good, thanks.

> It's just that I've seen reports where a user uses -bios (or an equivalent 
> "hand-crafted" domain XML) for VMs that are meant to be persistent. The 
> resultant confusion is not good.

But "qemu-system-x86_64 -bios /usr/share/OVMF/OVMF_CODE.fd" works (boots) anyway, so what would you gain from not distributing OVMF.fd?

Fewer possibilities to choose from, less confusion.

Anyway, I don't insist, it was just a thought.

Is it possible to have both the unified file and the non-unified ones available as options to install and use?

(In reply to Laszlo Ersek from comment #22)
> Fewer possibilities to choose from, less confusion.

Less confusion is good, dropped it.

And if just using "-bios /usr/share/OVMF/OVMF_CODE.fd" works (comment 21) for those who don't want fiddle with varstore setup I can see no gain from having OVMF.fd too.

New build uploaded without it.
The usual tarball update too.
And I've made the doc package noarch (missed that before).

How to go forward now?  Paolo, do you want pick this up and update the package?
If not I can also ask for commit access in pkgdb, then do it myself.

FWIW I'm happy to give you access and let you update at your discretion

(In reply to Cole Robinson from comment #25)
> FWIW I'm happy to give you access and let you update at your discretion

Not sure how much my vote counts, but I definitely support this!

distgit updated & builds done for rawhide and f24.

(In reply to Gerd Hoffmann from comment #28)
> distgit updated & builds done for rawhide and f24.

Can we get the f24 build pushed as an update. It would be useful for aarch64

edk2-20160418gita8c39ba-0.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-9f7573e344

I submitted an f24 update now

edk2-20160418gita8c39ba-0.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-9f7573e344

(In reply to Cole Robinson from comment #31)
> I submitted an f24 update now

Thanks, I totally forgot that one.

Wasn't there a remainder email in the past after updates where sitting in -testing for two or three weeks?

> Wasn't there a remainder email in the past after updates where sitting in
> -testing for two or three weeks?

You need to have submitted it as an update to bodhi for that ;-)

edk2-20160418gita8c39ba-0.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.