Hi,
On Tue, Aug 09, 2022 at 05:09:42PM +0100, Nikos Nikoleris wrote:
> Hi,
>
> On 09/08/2022 12:16, Alexandru Elisei wrote:
> > Hi,
> >
> > Adding Sean and Zixuan, as they were involved in the initial x86 UEFI
> > support.
> >
> > This version of the UEFI support for arm64 jumps to lib/efi.c::efi_main
> > after performing the relocation. I'll post an abbreviated/simplified
> > version of efi_main() for reference:
> >
> > efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> > {
> > /* Get image, cmdline and memory map parameters from UEFI */
> >
> > efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
> >
> > /* Set up arch-specific resources */
> > setup_efi(&efi_bootinfo);
> >
> > /* Run the test case */
> > ret = main(__argc, __argv, __environ);
> >
> > /* Shutdown the guest VM */
> > efi_exit(ret);
> >
> > /* Unreachable */
> > return EFI_UNSUPPORTED;
> > }
> >
> > Note that the assumption that efi_main() makes is that setup_efi() doesn't
> > change the stack from the stack that the UEFI implementation allocated, in
> > order for setup_efi() to be able to return to efi_main().
> >
> > arm64 requires explicit data cache maintenance to keep the contents of the
> > caches in sync with memory when writing with MMU off/reading with MMU on
> > and viceversa. More details of what is needed is why here [1] and here [2].
> > These operations must also be performed for the stack because the stack is
> > always used when running C code.
> >
> > What this means is that if arm64 wants to be able to run C code when the
> > MMU is disabled and when it is enabled, then it must perform data cache
> > operations for the stack memory. Which is impossible if the stack has been
> > allocated by UEFI, as kvm-unit-tests has no way of knowing its size, as it
> > isn't specified in the UEFI spec*. As a result, either efi_main needs to be
> > changed such that setup_efi() never returns, or arm64 must implement its
> > own version of efi_main() (or however it will end up being called).
> >
>
> I think it's possible to know the size of the stack. In 22/27 "arm64: Use
> code from the gnu-efi when booting with EFI", we change the top of the stack
> we start executing C code. Then at any point we can get the bottom of the
> stack.
For reference, I believe this is the code you are referring to (removed
some parts because they were irrelevant):
diff --git a/arm/efi/crt0-efi-aarch64.S b/arm/efi/crt0-efi-aarch64.S
index d50e78dd1faa..03d29b01cb38 100644
--- a/arm/efi/crt0-efi-aarch64.S
+++ b/arm/efi/crt0-efi-aarch64.S
@@ -111,10 +111,19 @@ section_table:
[..]
+ /* Align sp; this is necessary due to way we store cpu0's thread_info */
mov x29, sp
+ and x29, x29, #THREAD_MASK
+ mov x30, sp
+ mov sp, x29
It looks to me like the code doesn't do anything to detect the size and
top/bottom of the stack. What it looks me, is that the code conveniently
assumes that 1. the stack is bigger than THREAD_SIZE = THREAD_MASK + 1
(which is 16K) and 2. the stack is aligned to THREAD_SIZE. As
under-specified the stack size is, I couldn't find any mention of the stack
alignment in the spec.
But it's more than about knowing the size and address of the stack. It's
about reproducibility and having as much control as possible over the
environment. Today, if the stack overflows/underflows you can detect that
by comparing the stack pointer with edata/stacktop. What happens if that
happens to the UEFI stack? Who knows.
Also, I think we can agree that it is not inconceivable that there can be a
bug with how firmware manages the stack for an app. How can that be
reproduced if the bug manifests itself only on a very particular
combination of hardware and firmware? How can that be debugged? And how can
that be fixed? Fixing it in firmware might be impossible, same as expecting
people to upgrade their firmware, which might not be possible for their
particular setups. Wouldn't it be much simpler to modify kvm-unit-tests to
use its own stack?
That's why I think that arm64 doing what x86 did and replace the UEFI stack
is a good idea.
>
> But I doubt cleaning the stack is sufficient. What about the .data segment.
> Take for example, mem_regions. We populate them with the MMU on. We then use
> them to create page tables. We will need to clean them too. I won't be
> surprised if there is more data we would need to clean.
I agree completely! The setup code should definitely to do dcache
maintenance.
If the UEFI setup code does dcache maintenance, I believe it would look
very strange if the normal setup code (as a kernel inside a VM) doesn't do
any by relying instead on the cache maintenance that KVM does. That's why I
think the best way forward would be to share as much of the setup code as
possible between the two.
Something like this (this is just an example off the top of my head):
- setup_efi() does whatever it's needed before disabling the MMU.
- it branches to the assembly routine that the disables the MMU and
whatever else it needs to do (maybe exceptions_init like start does).
- it calls the C function lib/arm/setup.c::setup(), which can skip some
steps under UEFI (like the mem_regions setup).
- setup_efi() then returns to efi_main().
That way the UEFI code can reuse most of the dcache maintenance that is
proposed in this series [1], and reuse most of the existing boot code. What
do you think?
[1] https://lore.kernel.org/kvmarm/20220809091558.14379-1-alexandru.elisei@xxxxxxx/
Thanks,
Alex
>
> Thanks,
>
> Nikos
>
> > One way to get around this is never to run C code with the MMU off. That's
> > relatively easy to do in the boot code, as the translation tables can be
> > constructed with the MMU on, and then a fairly small assembly sequence is
> > required to install them.
> >
> > But arm64 also has two mechanisms for disabling the MMU:
> >
> > 1. At compile time, the user can request a test to start with the MMU off,
> > by setting the flag AUXINFO_MMU_OFF. So when booting as an UEFI app,
> > kvm-unit-tests must disable the MMU.
> >
> > 2. A function called mmu_disable() which allows a test to explicitly
> > disable the MMU.
> >
> > If we want to keep the UEFI allocated stack, then both mechanism must be
> > forbidden when running under UEFI. I dislike this idea, because those two
> > mechanisms allow kvm-unit-tests to run tests which otherwise wouldn't have
> > been possible with a normal operating system, which, except for the early
> > boot code, runs with the MMU enabled.
> >
> > Any thoughts or comments about this?
> >
> > *UEFI v2.8 states about the stack: "128 KiB or more of available stack
> > space" (page 35), but EDK2 allocates 64KiB [3]. So without any firmware
> > call to query the size of the stack, kvm-unit-tests cannot rely on it being
> > a specific size.
> >
> > [1] https://lore.kernel.org/kvm/20220809091558.14379-19-alexandru.elisei@xxxxxxx/
> > [2] https://lore.kernel.org/kvm/20220809091558.14379-20-alexandru.elisei@xxxxxxx/
> > [3] https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/ArmPlatformPkg.dec#L71
> >
> > Thanks,
> > Alex
> >
> > On Thu, Jun 30, 2022 at 11:02:57AM +0100, Nikos Nikoleris wrote:
> > > Hello,
> > >
> > > This patch series adds initial support for building arm64 tests as EFI
> > > tests and running them under QEMU. Much like x86_64 we import external
> > > dependencies from gnu-efi and adapt them to work with types and other
> > > assumptions from kvm-unit-tests. In addition, this series adds support
> > > for enumerating parts of the system using ACPI.
> > >
> > > The first set of patches moves the existing ACPI code to the common
> > > lib path. Then, it extends definitions and functions to allow for more
> > > robust discovery of ACPI tables. In arm64, we add support for setting
> > > up the PSCI conduit, discovering the UART, timers and cpus via
> > > ACPI. The code retains existing behavior and gives priority to
> > > discovery through DT when one has been provided.
> > >
> > > In the second set of patches, we add support for getting the command
> > > line from the EFI shell. This is a requirement for many of the
> > > existing arm64 tests.
> > >
> > > In the third set of patches, we import code from gnu-efi, make minor
> > > changes and add an alternative setup sequence from arm64 systems that
> > > boot through EFI. Finally, we add support in the build system and a
> > > run script which is used to run an EFI app.
> > >
> > > After this set of patches one can build arm64 EFI tests:
> > >
> > > $> ./configure --enable-efi
> > > $> make
> > >
> > > And use the run script to run an EFI tests:
> > >
> > > $> ./arm/efi/run ./arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256"
> > >
> > > Or all tests:
> > >
> > > $> ./run_tests.sh
> > >
> > > There are a few items that this series does not address but they would
> > > be useful to have:
> > > - Support for booting the system from EL2. Currently, we assume that a
> > > tests starts running at EL1. This the case when we run with EFI, it's
> > > not always the case in hardware.
> > > - Support for reading environment variables and populating __envp.
> > > - Support for discovering the PCI subsystem using ACPI.
> > > - Get rid of other assumptions (e.g., vmalloc area) that don't hold on real HW.
> > >
> > > git branch: https://github.com/relokin/kvm-unit-tests/pull/new/target-efi-upstream-v3-rebased
> > >
> > > v2: https://lore.kernel.org/kvm/20220506205605.359830-1-nikos.nikoleris@xxxxxxx/
> > >
> > > Changes in v3:
> > > - Addressed feedback from Drew, Alex and Ricardo. Many thanks for the reviews!
> > > - Added support for discovering the GIC through ACPI
> > > - Added a missing header file (<elf.h>)
> > > - Added support for correctly parsing the outcome of tests (./run_tests)
> > >
> > > Thanks,
> > >
> > > Nikos
> > >
> > > Alexandru Elisei (1):
> > > lib: arm: Print test exit status
> > >
> > > Andrew Jones (3):
> > > arm/arm64: mmu_disable: Clean and invalidate before disabling
> > > arm/arm64: Rename etext to _etext
> > > arm64: Add a new type of memory type flag MR_F_RESERVED
> > >
> > > Nikos Nikoleris (23):
> > > lib: Fix style for acpi.{c,h}
> > > x86: Avoid references to fields of ACPI tables
> > > lib: Ensure all struct definition for ACPI tables are packed
> > > lib: Add support for the XSDT ACPI table
> > > lib: Extend the definition of the ACPI table FADT
> > > devicetree: Check if fdt is NULL before returning that a DT is
> > > available
> > > arm/arm64: Add support for setting up the PSCI conduit through ACPI
> > > arm/arm64: Add support for discovering the UART through ACPI
> > > arm/arm64: Add support for timer initialization through ACPI
> > > arm/arm64: Add support for cpu initialization through ACPI
> > > arm/arm64: Add support for gic initialization through ACPI
> > > lib/printf: Support for precision modifier in printf
> > > lib/printf: Add support for printing wide strings
> > > lib/efi: Add support for getting the cmdline
> > > lib: Avoid ms_abi for calls related to EFI on arm64
> > > arm/arm64: Add a setup sequence for systems that boot through EFI
> > > arm64: Copy code from GNU-EFI
> > > arm64: Change GNU-EFI imported file to use defined types
> > > arm64: Use code from the gnu-efi when booting with EFI
> > > lib: Avoid external dependency in libelf
> > > x86: Move x86_64-specific EFI CFLAGS to x86_64 Makefile
> > > arm64: Add support for efi in Makefile
> > > arm64: Add an efi/run script
> > >
> > > scripts/runtime.bash | 14 +-
> > > arm/efi/run | 61 +++++++
> > > arm/run | 14 +-
> > > configure | 15 +-
> > > Makefile | 4 -
> > > arm/Makefile.arm | 6 +
> > > arm/Makefile.arm64 | 18 +-
> > > arm/Makefile.common | 48 +++--
> > > x86/Makefile.x86_64 | 4 +
> > > lib/linux/efi.h | 25 +++
> > > lib/arm/asm/setup.h | 3 +
> > > lib/arm/asm/timer.h | 2 +
> > > lib/acpi.h | 348 ++++++++++++++++++++++++++++--------
> > > lib/argv.h | 1 +
> > > lib/elf.h | 57 ++++++
> > > lib/libcflat.h | 1 +
> > > lib/acpi.c | 129 ++++++++-----
> > > lib/argv.c | 2 +-
> > > lib/arm/gic.c | 127 ++++++++++++-
> > > lib/arm/io.c | 29 ++-
> > > lib/arm/mmu.c | 4 +
> > > lib/arm/psci.c | 25 ++-
> > > lib/arm/setup.c | 247 ++++++++++++++++++++-----
> > > lib/arm/timer.c | 79 ++++++++
> > > lib/devicetree.c | 2 +-
> > > lib/efi.c | 102 +++++++++++
> > > lib/printf.c | 194 ++++++++++++++++++--
> > > arm/efi/elf_aarch64_efi.lds | 63 +++++++
> > > arm/flat.lds | 2 +-
> > > arm/cstart.S | 29 ++-
> > > arm/cstart64.S | 28 ++-
> > > arm/efi/crt0-efi-aarch64.S | 143 +++++++++++++++
> > > arm/dummy.c | 4 +
> > > arm/efi/reloc_aarch64.c | 93 ++++++++++
> > > arm/micro-bench.c | 4 +-
> > > arm/timer.c | 10 +-
> > > x86/s3.c | 19 +-
> > > x86/vmexit.c | 2 +-
> > > 38 files changed, 1700 insertions(+), 258 deletions(-)
> > > create mode 100755 arm/efi/run
> > > create mode 100644 lib/elf.h
> > > create mode 100644 lib/arm/timer.c
> > > create mode 100644 arm/efi/elf_aarch64_efi.lds
> > > create mode 100644 arm/efi/crt0-efi-aarch64.S
> > > create mode 100644 arm/dummy.c
> > > create mode 100644 arm/efi/reloc_aarch64.c
> > >
> > > --
> > > 2.25.1
> > >
