Re: [edk2-devel] [PATCH v1 0/6] Add Raspberry Pi Support to EmulatorPkg

["Ard Biesheuvel" <ardb@kernel.org>]

Hello Nate,

On Mon, 6 Mar 2023 at 01:22, Nate DeSimone
<nathaniel.l.desimone@intel.com> wrote:
>
> This patch series is the result of a fun weekend project that I
> did in my spare time. It implements the changes nessesary for
> EmulatorPkg to run on the Raspberry Pi. It should also work on
> other 32-bit ARM systems, but I only specifically tested on
> Raspberry Pi OS (formerly Raspbian.)
>

It works fine on 64-bit ARM systems that support 32-bit user space as
well. I managed to build and run this on my Qualcomm Snapdragon based
Lenovo Yoga C630 laptop running Debian Linux.

So

Tested-by: Ard Biesheuvel <ardb@kernel.org>

Also, good job!! This is really nice.

One nit: I had to add -mfpu=vfpv3 to the compiler command line in
Host.inf to work around a build error.


> I ran into several interesting issues during the development of
> this patch series and it ended up being a good learning
> experience for me. Some things of note are:
>
>  1 - Assembly Code
>
> There are several pieces of assembly code in EmulatorPkg that need
> to be rewritten when porting to a new machine architecture. This
> code fell into two categories, stack manipulation and "gaskets."
>
>  2 - ABI Differences
>
> The most significant amount of assembly code is the "gasket"
> functions. The gasket functions are responsible for converting from
> the UNIX ABI to the EFI ABI. They enable EmulatorPkg to
> support executing arbitary UEFI binaries without needing
> recompilation specifically for EmulatorPkg. X86 has many ABI
> specifications that vary based on target OS and compiler toolchain.
> There have been several attempts to unify things on the x86 side,
> but they usually just result in the addition of yet another ABI.
> Things are much more uniform on ARM due to wide acceptance of the
> AAPCS. Due to this, the ABI differences between UNIX and EFI boil
> down to passing of variadic arguments and enum values. Neither of
> these cases apply to the function signatures for the gaskets.
> Therefore, on ARM these gaskets can be simple wrapper functions
> written in C since the UNIX and EFI ABIs are identical... at least
> for the set of functions that need gaskets.
>

Glad to hear that. I assume this would apply to AArch64 as well.

>  3 - Instruction Cache Maintenance
>
> Because EmulatorPkg and edk2 in general contains a PE/COFF loader,
> it counts as "self-modifying code". The way self modifying code is
> handled on ARM is considerably more complex than x86. While on x86
> there is no requirement for self-modifying code to flush the
> instruction cache, on ARM failure to do so will result in incorrect
> behavior. Additionally, flushing the cache is considerably more
> difficult on ARM. On x86, one can simply add a CLFLUSH or WBINVD
> instruction; they are perfectly valid to execute while in Ring 3.
> On ARM, flushing the cache requires one to write to system control
> registers, which can only be done in EL1 (kernel mode) or higher.
> Therefore, while flushing the cache can be done on in the x86
> version of EmulatorPkg without any interaction with the OS, on ARM
> we need to invoke OS syscalls.
>
> To accomodate this, I have added a new EMU_IO_THUNK_PROTOCOL that
> implements the CacheMaintenanceLib LibraryClass. This new
> implementation uses the GCC intrinsic function
> __builtin_clear_cache(), which on x86 gets converted into a CLFLUSH
> instruction by the compiler. On ARM, it gets converted into a call
> to the __clear_cache() API in libgcc. The ARM implementation of
> libgcc invokes the ARM_cacheflush syscall (0xF0002), which only
> exists in kernels built for the ARM architecture.
>
> I have added wrapper implementations of the CacheMaintenanceLib
> LibraryClass for PEI and DXE that use the EMU_THUNK infrastructure
> to acquire the EMU_CACHE_THUNK_PROTOCOL and invoke the equivilent
> functions, which in turn will use the GCC intrinsic mentioned
> above.
>
> I have only enable this version of CacheMaintenanceLib on the ARM
> architecture, because it will take several thousand CPU
> instructions to execute, as opposed to about ten on the current x86
> implementation.
>

I think this is reasonable, but I am not a EmulatorPkg maintainer so I
will defer to them to comment on this approach.

>  Testing Done:
>
> Boots to shell on Raspberry Pi OS "bullseye."
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
>
> Nate DeSimone (6):
>   ArmPkg: Add ArmMmuNullLib

Given the recent focus on memory protections - could we turn this into
a EmuPkg specific one that calls the OS's mprotect() under the hood?


>   EmulatorPkg: Add ARM Build Target
>   EmulatorPkg: Fix PosixFileSystem function misspellings
>   EmulatorPkg: Add ARM support to UNIX Host App
>   EmulatorPkg: Add ARM support to EmuSec
>   EmulatorPkg: Add EmuCacheMaintenanceLib
>
>  ArmPkg/ArmPkg.dsc                             |    2 +
>  ArmPkg/Library/ArmMmuNullLib/ArmMmuNullLib.c  |   84 ++
>  .../Library/ArmMmuNullLib/ArmMmuNullLib.inf   |   36 +
>  EmulatorPkg/EmulatorPkg.dec                   |    4 +-
>  EmulatorPkg/EmulatorPkg.dsc                   |   29 +-
>  EmulatorPkg/EmulatorPkg.fdf                   |    6 +-
>  EmulatorPkg/Include/Protocol/EmuCache.h       |  217 ++++
>  .../DxeEmuCacheMaintenanceLib.c               |  337 +++++
>  .../DxeEmuCacheMaintenanceLib.inf             |   37 +
>  .../PeiEmuCacheMaintenanceLib.c               |  344 ++++++
>  .../PeiEmuCacheMaintenanceLib.inf             |   39 +
>  EmulatorPkg/Sec/Arm/SwitchRam.S               |   32 +
>  EmulatorPkg/Sec/Arm/TempRam.c                 |   58 +
>  EmulatorPkg/Sec/Sec.inf                       |    9 +-
>  EmulatorPkg/Unix/Host/Arm/Gasket.c            |  895 ++++++++++++++
>  .../Unix/Host/Arm/GasketFunctionDefinitions.h | 1092 +++++++++++++++++
>  EmulatorPkg/Unix/Host/Arm/SwitchStack.S       |   39 +
>  EmulatorPkg/Unix/Host/CacheMaintenance.c      |  284 +++++
>  EmulatorPkg/Unix/Host/Gasket.h                |   12 +-
>  EmulatorPkg/Unix/Host/Host.c                  |    5 +-
>  EmulatorPkg/Unix/Host/Host.h                  |    2 +
>  EmulatorPkg/Unix/Host/Host.inf                |   14 +-
>  EmulatorPkg/Unix/Host/Ia32/Gasket.S           |   31 +-
>  EmulatorPkg/Unix/Host/PosixFileSystem.c       |   22 +-
>  EmulatorPkg/Unix/Host/X64/Gasket.S            |   31 +-
>  EmulatorPkg/build.sh                          |   13 +-
>  26 files changed, 3617 insertions(+), 57 deletions(-)
>  create mode 100644 ArmPkg/Library/ArmMmuNullLib/ArmMmuNullLib.c
>  create mode 100644 ArmPkg/Library/ArmMmuNullLib/ArmMmuNullLib.inf
>  create mode 100644 EmulatorPkg/Include/Protocol/EmuCache.h
>  create mode 100644 EmulatorPkg/Library/DxeEmuCacheMaintenanceLib/DxeEmuCacheMaintenanceLib.c
>  create mode 100644 EmulatorPkg/Library/DxeEmuCacheMaintenanceLib/DxeEmuCacheMaintenanceLib.inf
>  create mode 100644 EmulatorPkg/Library/PeiEmuCacheMaintenanceLib/PeiEmuCacheMaintenanceLib.c
>  create mode 100644 EmulatorPkg/Library/PeiEmuCacheMaintenanceLib/PeiEmuCacheMaintenanceLib.inf
>  create mode 100644 EmulatorPkg/Sec/Arm/SwitchRam.S
>  create mode 100644 EmulatorPkg/Sec/Arm/TempRam.c
>  create mode 100644 EmulatorPkg/Unix/Host/Arm/Gasket.c
>  create mode 100644 EmulatorPkg/Unix/Host/Arm/GasketFunctionDefinitions.h
>  create mode 100644 EmulatorPkg/Unix/Host/Arm/SwitchStack.S
>  create mode 100644 EmulatorPkg/Unix/Host/CacheMaintenance.c
>
> --
> 2.30.2
>
>
>
> 
>
>