Re: [RFC PATCH 0/6] OVMF: HFS+ (and Mac OS X boot)

[Laszlo Ersek <lersek@redhat.com>]

On 03/07/17 04:14, Gabriel L. Somlo wrote:
> This series is intended to restart the conversation re. adding
> boot support for Mac OS X to QEMU via OVMF.

I have no way to test this, nor any interest to use it, so on the
community level, my only role here (for OvmfPkg) is to recommend a way
in which this (quite specific) feature can be accommodated without a
high risk of regressions and/or complexity increase. From reading the
commit messages, it looks like the OSX boot flow doesn't conform to UEFI
2.x, so any damage this feature does to OVMF (even in human
understanding) should be contained.

(1) Please introduce a new build flag, such as -D APPLE_BOOT_ENABLE or
-D OSX_BOOT_ENABLE, with a FALSE default value.

This build option should control:

(1a) whether FswHfsPlus and DataHubDxe are included in the DSC/FDF,

(1b) the value of a new FeaturePCD (also to be introduced). For the
name, I suggest gUefiOvmfPkgTokenSpaceGuid.PcdAppleBootEnable, or
...PcdOsxBootEnable.

In turn, the feature PCD should control code that's being added to
modules that we unconditionally build into OVMF. The prime examples for
this are the InitializeAppleSupport() and BdsBootApple() function calls
in PlatformBootManagerLib.

(1c) how the AppleSupportLib class is resolved. If the PCD / build flag
are false (the default), the lib class should be resolved to a Null
instance.

The point is, introducing a dependency on EdkCompatibilityPkg is
*incredibly* ugly, and if OVMF is built with the build flag disabled
(the default), I don't want that dependency to exist even at build time.
Hence the Null library instance, with no functionality and no dependencies.

(2) Please feed all the patches to:

  python BaseTools/Scripts/PatchCheck.py

because a number of coding style problems can be spotted. I'll comment
on those individually, as I notice them, but PatchCheck.py should help
in general.

Also please double-check if the new files are all CRLF terminated (I
didn't check this, just making sure).

(3) With the recent feature divergence in OvmfPkg (see also Xen PVH!),
it is high time we introduced maintainers / reviewers on the module (or
even file) level.

This would be a first for edk2, but in the mid-term, I'd strongly like
to assign Xen patch review and OSX boot patch review to dedicated
reviewers. No review from those reviewers --> no commit. The new
reviewers would not (immediately?) get push access, but their review
on-list would be of primary importance for pushing any patches.

Jordan, can you please help with figuring out the necessary format for
Maintainers.txt? If OSX boot has to be the feature that pioneers this,
so be it. In fact, such a detailed directory of maintainers would have
been useful for MdePkg and MdeModulePkg for years.

> The *last* three patches represent what's left of Reza's excellent
> work on getting OVMF to support booting OS X. They are rebased to
> the point where they still apply cleanly and work, but I haven't
> made any significant changes to them beyond that.
> 
> The *first* three patches (1-3 of 6) provide the BSD-licensed HFS+
> filesystem support required by Reza's work:

This is a filesystem driver which is not closely related to OVMF. It
should not go under OvmfPkg, but under a new top level package. I think
someone (maybe Jordan?) recommended FileSystemPkg/ earlier. Of course,
this would need a new entry in Maintainers.txt as well, and then, for
uniformity, FatPkg should be moved there as well.

> 
>     - patch 1/6: After a bit of archaeology, I managed to isolate
>                  the original EFI "File System Wrapper" (FSW) files
>                  released under the BSD by the "rEFIt" project; One
>                  additional file, also BSD-licensed, was imported
>                  from the "rEFInd" project.
>                  The FSW is an abstraction layer that facilitates
>                  the creation of a file system driver by providing
>                  a set methods for mount/unmount/readdir/etc.

So, technically speaking, since I've just suggested moving the driver
from under OvmfPkg to FileSystemPkg/, I no longer have any jurisdiction
over the driver. I will comment nonetheless, like any other contributor:

I disagree with the inclusion of any such external wrapper. In my mind
this is similar to how the Linux project rejects vendor-specific middle
layers. It might make sense for the rEFIt project to abstract the UEFI
interfaces even further, but having those one-off abstractions within
the edk2 tree does not make sense, in my opinion. Please code the driver
directly against UEFI interfaces. The reasoning is the same as the Linux
argument: anyone else looking at this code will expect to have to deal
with UEFI-level abstractions, and the filesystem spec, not another
(external) project's abstractions.

And, similarly to the AMD middle layer thing for Linux, the middle layer
being introduced here (in patch #1) is huge: the diffstat lists 3784
lines (the diffstat for the full series is 5287 lines). Please let's not
do this.

Again, this is just my opinion.

> 
>     - patch 2/6: fix a few compiler errors and other discrepancies to
>                  facilitate integrating FSW into EDK2/OVMF.
> 
>     - patch 3/6: A BSD-licensed implementation of an FSW HFS+ driver.
>                  Based on Apple's HFS+ specification (TN1150), this is
>                  a minimalistic, bare-bones set of FSW methods capable
>                  of locating and loading files from an HFS+ volume.
>                  Lots of functionality (e.g. stat, readdir, hard/sym
>                  links, or accessing files with more than 8 fragments)
>                  is unimplemented at this time.

This ties in nicely with my above argument. "stat", "readdir",
"hardlinks" and "symlinks" are concepts outside of the UEFI
specification, that is, outside of EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and
EFI_FILE_PROTOCOL.

These primitives might make sense for rEFIt, but they make no sense for
a filesystem driver residing within the edk2 tree.

> I only implemented the
>                  methods necessary to support loading Apple's boot.efi
>                  and kernel.
> 
>                  For any extra features (such as stat, readdir, etc.)
>                  I'd need to figure out how to navigate around on a
>                  mounted volume (e.g. from the UEFI shell ?) to cause
>                  the rest of the methods to be called, so I could test
>                  whether they work or not. Particularly support for
>                  fragmented files -- I'd have to create one somehow
>                  (against OS X and HFS's best efforts to prevent it),
>                  then cause it to be accessed from the UEFI shell to
>                  test that whatever I'm implementing actually works.
>                  Any "EDK2 filesystem developer's primer" on how to
>                  do those things would be immensely helpful at this point.

If you implement a pure EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and
EFI_FILE_PROTOCOL (with Revision==EFI_FILE_PROTOCOL_REVISION, that is,
not revision 2), you will see, from the UEFI spec, the member functions:
- Open(),
- Close(),
- Delete(),
- Read(),
- Write(),
- SetPosition(),
- GetPosition(),
- GetInfo(),
- SetInfo(),
- Flush().

Since this is (I assume?) a read-only driver, you can return
EFI_ACCESS_DENIED or EFI_WRITE_PROTECTED from Write(), SetInfo(), and
Flush(), as appropriate. Delete() should return EFI_WARN_DELETE_FAILURE.
Open() and Read() can be trivially tested in the UEFI shell (copy a file
to another, writeable (fat32) filesystem). SetPosition() and
GetPosition() can likely be implemented trivially. GetInfo() can be
tested with directory listing commands in the shell (dir, ls, AFAIR).

You can see a synthetic, read-only filesystem implementation in
"ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c".

> 
> Also, I would like some feedback and advice on where to go from here.
> 
> At this time, neither the FSW wrapper nor the HFS+ driver I wrote comply
> with EDK2's coding standards. Before I start working on that, it would
> be helpful to find out whether the FSW layer is of any interest to EDK2
> as a way to facilitate adding support for arbitrary new filesystems.

Ah, it's great that you ask this question explicitly! Please see my
answer above.

Regardless, there are some coding standard issues with the OvmfPkg
patches too (see my point (2) above); I shall comment on those elsewhere.

Thanks!
Laszlo

> 
> If not, it might be worth factoring out the common bits and roll the
> the whole thing up into a standalone HFS+ driver, which would be a
> significantly different direction to go.
> 
> The series is also available on GitHub, in my "macboot" branch:
> 
> 	https://github.com/gsomlo/edk2/tree/macboot
> 
> A set of instructions on how to get things working is available here:
> 
> 	http://www.contrib.andrew.cmu.edu/~somlo/OSXKVM/
> 
> Thanks in advance for any feedback, advice, etc.
>   --Gabriel
> 
> Gabriel L. Somlo (6):
>   FswHfsPlus: add File System Wrapper (FSW) interface code
>   FswHfsPlus: connect FSW code to EDK2, fix compile discrepancies
>   FswHfsPlus: implement FSW driver for the HFS+ file system
>   EdkCompatibilityPkg: allow ConsoleControl protocol to be used
>   OvmfPkg: add Apple boot support
>   OvmfPkg: enable AppleSupport library for Ovmf firmware
> 
>  EdkCompatibilityPkg/EdkCompatibilityPkg.dec        |    2 +
>  OvmfPkg/FswHfsPlus/FswHfsPlus.inf                  |   57 ++
>  OvmfPkg/FswHfsPlus/fsw_base.h                      |  158 +++
>  OvmfPkg/FswHfsPlus/fsw_core.c                      |  929 +++++++++++++++++
>  OvmfPkg/FswHfsPlus/fsw_core.h                      |  517 ++++++++++
>  OvmfPkg/FswHfsPlus/fsw_efi.c                       | 1040 ++++++++++++++++++++
>  OvmfPkg/FswHfsPlus/fsw_efi.h                       |  102 ++
>  OvmfPkg/FswHfsPlus/fsw_efi_base.h                  |   81 ++
>  OvmfPkg/FswHfsPlus/fsw_efi_edk2_base.h             |   76 ++
>  OvmfPkg/FswHfsPlus/fsw_efi_lib.c                   |  129 +++
>  OvmfPkg/FswHfsPlus/fsw_hfsplus.c                   |  535 ++++++++++
>  OvmfPkg/FswHfsPlus/fsw_hfsplus.h                   |  238 +++++
>  OvmfPkg/FswHfsPlus/fsw_lib.c                       |  294 ++++++
>  OvmfPkg/FswHfsPlus/fsw_strfunc.h                   |  453 +++++++++
>  OvmfPkg/Include/Library/AppleSupportLib.h          |   28 +
>  OvmfPkg/Library/AppleSupportLib/AppleSupport.c     |  107 ++
>  .../Library/AppleSupportLib/AppleSupportLib.inf    |   50 +
>  OvmfPkg/Library/AppleSupportLib/Bds.c              |  151 +++
>  OvmfPkg/Library/AppleSupportLib/Bds.h              |   21 +
>  OvmfPkg/Library/AppleSupportLib/Common.h           |   24 +
>  OvmfPkg/Library/AppleSupportLib/Console.c          |   86 ++
>  OvmfPkg/Library/AppleSupportLib/Console.h          |   28 +
>  OvmfPkg/Library/AppleSupportLib/Datahub.c          |  104 ++
>  OvmfPkg/Library/AppleSupportLib/Datahub.h          |   32 +
>  .../Library/PlatformBootManagerLib/BdsPlatform.c   |   10 +
>  .../Library/PlatformBootManagerLib/BdsPlatform.h   |    1 +
>  .../PlatformBootManagerLib.inf                     |    1 +
>  OvmfPkg/OvmfPkgIa32.dsc                            |    8 +
>  OvmfPkg/OvmfPkgIa32.fdf                            |    3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    8 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                         |    3 +
>  OvmfPkg/OvmfPkgX64.dsc                             |    8 +
>  OvmfPkg/OvmfPkgX64.fdf                             |    3 +
>  33 files changed, 5287 insertions(+)
>  create mode 100644 OvmfPkg/FswHfsPlus/FswHfsPlus.inf
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_base.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_core.c
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_core.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_efi.c
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_efi.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_efi_base.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_efi_edk2_base.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_efi_lib.c
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_hfsplus.c
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_hfsplus.h
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_lib.c
>  create mode 100644 OvmfPkg/FswHfsPlus/fsw_strfunc.h
>  create mode 100644 OvmfPkg/Include/Library/AppleSupportLib.h
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/AppleSupport.c
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/AppleSupportLib.inf
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Bds.c
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Bds.h
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Common.h
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Console.c
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Console.h
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Datahub.c
>  create mode 100644 OvmfPkg/Library/AppleSupportLib/Datahub.h
>