Re: [PATCH V2] SbsaQemu: EFI implementation for SbsaQemu platform

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Sun, Dec 15, 2019 at 17:46:25 +0530, Tanmay Jagdale wrote:
> From: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> 
> Linaro enterprise group is coordinating work for adding SBSA compliant
> virtual platform for QEMU. This patch adds initial support for platform
> with nondiscoverable AHCI, VGA and single DRAM window over 32bit
> address space.
> 
> Using FDF to compose EFI flash images with ATF images.
> Flash0 (secure) is used by BL1 and FIP (BL2 + BL31).
> Flash1 contains EFI code and EFI variables.
> 
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> Signed-off-by: Tanmay Jagdale <tanmay.jagdale@linaro.org>
> ---
> This patch adds initial support for Sbsa QEMU model. There will be
> following patches which will add support for ACPI tables.
> 
> Changes in V2:
>  - Fixed capitalization in the Readme.md file 
> 
>  Maintainers.txt                               |  12 +
>  Platform/Qemu/SbsaQemu/Readme.md              | 159 ++++
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc           | 677 ++++++++++++++++++
>  Platform/Qemu/SbsaQemu/SbsaQemu.fdf           | 303 ++++++++
>  Readme.md                                     |   3 +
>  .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c |  55 ++
>  .../SbsaQemuPlatformDxe.inf                   |  41 ++
>  .../Qemu/Library/SbsaQemuLib/SbsaQemuHelper.S |  55 ++
>  .../Qemu/Library/SbsaQemuLib/SbsaQemuLib.c    | 139 ++++
>  .../Qemu/Library/SbsaQemuLib/SbsaQemuLib.inf  |  47 ++
>  .../Qemu/Library/SbsaQemuLib/SbsaQemuMem.c    |  80 +++
>  .../SbsaQemuNorFlashLib/SbsaQemuNorFlashLib.c |  40 ++
>  .../SbsaQemuNorFlashLib.inf                   |  29 +
>  .../SbsaQemuPciHostBridgeLib.c                | 215 ++++++
>  .../SbsaQemuPciHostBridgeLib.inf              |  47 ++
>  Silicon/Qemu/SbsaQemuPkg.dec                  |  39 +
>  16 files changed, 1941 insertions(+)
>  create mode 100644 Platform/Qemu/SbsaQemu/Readme.md
>  create mode 100644 Platform/Qemu/SbsaQemu/SbsaQemu.dsc
>  create mode 100644 Platform/Qemu/SbsaQemu/SbsaQemu.fdf
>  create mode 100644 Silicon/Qemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
>  create mode 100644 Silicon/Qemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuLib/SbsaQemuHelper.S
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuLib/SbsaQemuLib.c
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuLib/SbsaQemuLib.inf
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuLib/SbsaQemuMem.c
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuNorFlashLib/SbsaQemuNorFlashLib.c
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuNorFlashLib/SbsaQemuNorFlashLib.inf
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuPciHostBridgeLib/SbsaQemuPciHostBridgeLib.c
>  create mode 100644 Silicon/Qemu/Library/SbsaQemuPciHostBridgeLib/SbsaQemuPciHostBridgeLib.inf
>  create mode 100644 Silicon/Qemu/SbsaQemuPkg.dec
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index cb7ae0a95d..320b38d049 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -257,3 +257,15 @@ F: Platform/Socionext/
>  F: Silicon/Socionext/
>  M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>  M: Leif Lindholm <leif.lindholm@linaro.org>
> +
> +Silicon/Qemu/
> +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> +M: Leif Lindholm <leif.lindholm@linaro.org>
> +R: Radoslaw Biernacki <rad@semihalf.com>
> +R: Tanmay Jagdale <tanmay.jagdale@linaro.org>
> +
> +Platform/Qemu/SbsaQemu
> +M: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> +M: Leif Lindholm <leif.lindholm@linaro.org>
> +R: Radoslaw Biernacki <rad@semihalf.com>
> +R: Tanmay Jagdale <tanmay.jagdale@linaro.org>
> diff --git a/Platform/Qemu/SbsaQemu/Readme.md b/Platform/Qemu/SbsaQemu/Readme.md
> new file mode 100644
> index 0000000000..fc2bd5a7ff
> --- /dev/null
> +++ b/Platform/Qemu/SbsaQemu/Readme.md
> @@ -0,0 +1,159 @@
> +# Overview
> +
> +This directory holds UEFI implementation for Sbsa-ref machine which is faithful
> +as possible to real hardware. In opposition to existing QEMU Virt machine which
> +is suited for performing workloads, the purpose of this machine is development
> +of firmware and OS for AARCH64 server alike platforms (like in situation where
> +real HW is not available yet or the debugging is easier to control under
> +emulation). The SBSAQemu name, was chosen because the modeled HW is aimed to
> +follow the way that server-style armv8 machines are recommended to be set up.
> +Implementation does not use fw_cfg nor DT provided by QEMU.

(I will refer back here from a comment below.)

> +
> +# How to build (Linux Environment)
> +
> +## Prerequisites
> +
> +Build process for Sbsa-ref uses FDF file for flash image composition. This is
> +different to what some might expect as you need to first build the ATF before
> +building EDK2.
> +Flash0 (secure) is used by BL1 and FIP (BL2 + BL31).
> +Flash1 contains EFI code and EFI variables.
> +For rest of the build process, typical prerequisites apply.
> +
> +## Obtaining source code
> +
> +1. Create a new directory on your local development machine for use as your
> +   workspace. This example uses `~/workspace`, modify as appropriate for your
> +   needs. In the next steps we will use `WORKSPACE` environment variable for
> +   reference to this directory.
> +
> +  ```
> +  cd ~/
> +  mkdir workspace
> +  cd workspace
> +  export WORKSPACE=$PWD
> +  ```

We don't need this detailed instructions - top-level should tell us
how to build EDK2 - just point out where the .dsc lives and that the
build requires edk2-non-osi in PACKAGES_PATH for TF-A.

> +
> +1. Into that folder, clone:
> +  1. [qemu](https://github.com/qemu/qemu.git)
> +  2. [edk2](https://github.com/tianocore/edk2)
> +  3. [edk2-platforms](https://github.com/tianocore/edk2-platforms)
> +  4. [edk2-non-osi](https://github.com/tianocore/edk2-non-osi)

Correct.

> +  5. [atf](https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git)
> +
> +  ```
> +  cd $WORKSPACE
> +  git clone https://github.com/qemu/qemu.git
> +  git clone https://github.com/tianocore/edk2
> +  git clone https://github.com/tianocore/edk2-platforms
> +  git clone https://git.linaro.org/people/tanmay.jagdale/edk2-non-osi.git

Not correct.
(This is another reason why it's often not really helpful to duplicate
basic build instructions.)

> +  git clone https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git
> +  ```
> +  For Edk2 we need to initialize sub-modules (like OpenSSL)
> +  ```
> +  cd edk2
> +  git submodule init
> +  git submodule update
> +  ```
> +
> +## Manual building
> +
> +1. Compile QEMU
> +
> +  Sbsa-ref machine is fully SW emulated SBSA machine (ARM64). It aims to emulate
> +  the real HW as close as possible. It's purpose it to enable new feature
> +  development when HW for those features are not yet present on the market.
> +  It allows poking the HW to do all kinds of things (including errors) which are
> +  beyond control on real HW. It also allow easy simulations and debugging of
> +  FW-to-HW interactions.
> +
> +  Keep in mind that all of the above is possible as this machine is fully
> +  emulated in SW. Sbsa-ref machine does not use any HW acceleration of your
> +  platform, even if you run it on ARM64 platform. The EL3 (and ARM TrustZone)
> +  is also emulated in SW.

On reading this, I think it would make a lot more sense to move this
introductory text to the start of the file. This whole block could
replace the current introduction statement (which goes into a bit too
much detail on the development history of the platform).

> +
> +  Sbsa-ref machine was added into QEMU in v4.1.0
> +  If your distribution package provides an earlier version then you need to
> +  compile QEMU from the source. Below is a short instruction how to compile
> +  without referring to QEMU docs.
> +  The `qemu-inst` is the install directory for QEMU in `workspace`.
> +  Modify those as appropriate for your needs.
> +
> +  ```
> +  cd $WORKSPACE

I don't think we need to proscribe where people install things on
their local machine. Suggest replacing this with something like
INSTALL_PATH set to /usr/local, ~/local or whatever.

> +  mkdir qemu-inst
> +  cd qemu
> +  mkdir -p build-native
> +  cd build-native
> +  ../configure --target-list=aarch64-softmmu --prefix=$WORKSPACE/qemu-inst

--prefix=$INSTALL_PATH

> +  make -j $NUM_CPUS install

Where is NUM_CPUS set?
I think the -j bit is redundant - we're showing people how to build
*this*, not how to drive make. (Applies throughout.)

> +  ```
> +
> +  QEMU should be installed now in `$WORKSPACE/qemu-inst`

$INSTALL_PATH

> +
> +1. Compile ATF

I think we're supposed to call it TF-A these days.

> +
> +  This step is only needed is users want to compile a custom ATF binary. Else, the edk2-non-osi directory contains prebuilt bl1.bin and fip.bin binaries which will be automatically used in the build process.
> +  As noted before, for Sbsa-ref machine we use FDF to compose two flash images. Those flash images need BL1, BL2 and BL31 from ATF in form of two files `bl1.bin` and `fip.bin`. Follow the instructions below to get those artifacts.
> +
> +  ```
> +  cd $WORKSPACE/atf
> +  export CROSS_COMPILE=aarch64-linux-gnu-

Please drop above line. Cross compilation is adequately described
elsewhere.

> +  make PLAT=sbsa all fip
> +  ```
> +  Than copy `bl1.bin` and `fip.bin` to the top `workspace` directory:
> +  ```
> +  cp build/sbsa/release/bl1.bin ../
> +  cp build/sbsa/release/fip.bin ../
> +  ```

The above is now actively incorrect - if someone rebuilds their own
TF-A images, they need to drop them into the edk2-non-osi directory(*).

> +
> +1. edk2-platforms - SBSA QEMU UEFI implementation
> +
> +  Compilation of BaseTools and preparation:
> +
> +  ```
> +  cd $WORKSPACE
> +  export PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms:$WORKSPACE/edk2-non-osi
> +  make -C edk2/BaseTools
> +  export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-

See previous comment about cross compilation - drop above line.

> +  . edk2/edksetup.sh
> +  ```
> +
> +  Compilation UEFI for Sbsa QEMU:
> +
> +  ```
> +  cd $WORKSPACE
> +  build -n $NUM_CPUS -b RELEASE -a AARCH64 -t GCC5 -p edk2-platforms/Platform/Qemu/SbsaQemu/SbsaQemu.dsc

You can drop the -n $NUM_CPUS here.
(Not for the same reason as for make above, the edk2 build system may
be new to people, but simply because it now automates this anyway.)

> +  ```
> +  Copy SBSA_FLASH0.fd and SBSA_FLASH0.fd to top `workspace` directory. Than extend the file size to match the machine flash size.
> +  ```
> +  cp Build/SbsaQemu/RELEASE_GCC5/FV/SBSA_FLASH[01].fd .
> +  truncate -s 256M SBSA_FLASH[01].fd
> +  ```
> +
> +1. Testing

Testing is what *we* do before we submit patches.
For anyone else, this step is called running.

> +
> +  The resulting SBSA_FLASH0.fd file will contain Secure flash0 image (ATF code).
> +  The SBSA_FLASH1.fd will contain Non-secure UEFI code and UEFI variables.
> +
> +  You will boot to the UEFI console with following QEMU command line:
> +  ```
> +  qemu-inst/bin/qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M sbsa-ref -pflash SBSA_FLASH0.fd -pflash SBSA_FLASH1.fd -serial stdio

Drop the qemu-inst/... bit.

Why is there a -cpu flag in there? That should be completely redundant
for sbsa-ref.

> +  ```
> +  You can add XHCI controller with keyboard and mouse by:
> +  ```
> +  -device qemu-xhci -device usb-mouse -device usb-kbd
> +  ```
> +  You can add the hard drive to platform AHCI controller by `hda` parameter:
> +  ```
> +  -hda disk1.img
> +  ```
> +  For TEE and other secure development you might get use of secure serial which would require following commands. First create `secure_serial` fifo and read it from separate terminal (open new terminal emulator window for it):
> +  ```
> +  mkfifo secure_serial
> +  tail -f secure_serial
> +  ```
> +  Than on first console:
> +  ```
> +  qemu-system-aarch64 -m 1024 -M sbsa-ref -pflash SBSA_FLASH0.fd -pflash SBSA_FLASH1.fd -serial stdio -hda disk1.img -serial file:secure_serial
> +  ```


> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> new file mode 100644
> index 0000000000..34efa4d83f
> --- /dev/null
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> @@ -0,0 +1,303 @@
> +#
> +#  Copyright (c) 2019, Linaro Limited. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +################################################################################
> +#
> +# FD Section for FLASH0
> +#
> +# The [FD] Section is made up of the definition statements and a
> +# description of what goes into  the Flash Device Image.  Each FD section
> +# defines one flash "device" image.  A flash device image may be one of
> +# the following: Removable media bootable image (like a boot floppy
> +# image,) an Option ROM image (that would be "flashed" into an add-in
> +# card,) a System "Flash"  image (that would be burned into a system's
> +# flash) or an Update ("Capsule") image that will be used to update and
> +# existing system flash.
> +#
> +################################################################################
> +
> +[FD.SBSA_FLASH0]
> +BaseAddress   = 0x00000000
> +Size          = 0x00200000
> +ErasePolarity = 1
> +BlockSize     = 0x00001000
> +NumBlocks     = 0x200
> +
> +################################################################################
> +#
> +# Following are lists of FD Region layout which correspond to the locations of different
> +# images within the flash device.
> +#
> +# Regions must be defined in ascending order and may not overlap.
> +#
> +# A Layout Region start with a eight digit hex offset (leading "0x" required) followed by
> +# the pipe "|" character, followed by the size of the region, also in hex with the leading
> +# "0x" characters. Like:
> +# Offset|Size
> +# PcdOffsetCName|PcdSizeCName
> +# RegionType <FV, DATA, or FILE>
> +#
> +################################################################################
> +## Place for Trusted Firmware
> +# flash0 is secure so we put here the BL1
> +0x00000000|0x00008000
> +FILE = bl1.bin

What? No!

This breaks the functionality that was there in the 4 December
submission and completely ignores edk2-non-osi. Please go through and
diff the two contributions and ensure *only* what should be different
is. Then resubmit that, with above documentation feedback addressed,
as v3.

And locally, ensure each patchset revision has its own working branch
- any other workflow will guarantee this sort of thing happening.

Regards,

Leif