From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0F05080348 for ; Tue, 7 Mar 2017 07:41:26 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6955461D07; Tue, 7 Mar 2017 15:41:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-184.phx2.redhat.com [10.3.116.184]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v27FfNci024826; Tue, 7 Mar 2017 10:41:23 -0500 To: "Gabriel L. Somlo" , edk2-devel@ml01.01.org References: <1488856465-8965-1-git-send-email-gsomlo@gmail.com> Cc: jordan.l.justen@intel.com, reza.jelveh@tuhh.de, agraf@suse.de, kraxel@redhat.com From: Laszlo Ersek Message-ID: <24e01636-68d4-2370-ca87-986ef7069728@redhat.com> Date: Tue, 7 Mar 2017 16:41:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1488856465-8965-1-git-send-email-gsomlo@gmail.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 07 Mar 2017 15:41:26 +0000 (UTC) Subject: Re: [RFC PATCH 0/6] OVMF: HFS+ (and Mac OS X boot) X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Mar 2017 15:41:26 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 >