On 12/08/2022 15:55, Alexandru Elisei wrote:
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.
The code assumes that the stack is big enough. You've pointed to manuals
that support this. It doesn't assume that stack pointer is aligned, it
aligns the stack pointed as per kvm-unit-tests arm64 requirements.
For the purposes of cleaning the stack, the value of sp computed by this
code is top of the stack. The bottom of the used stack is the value of
sp when we call asm_mmu_disable.
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.
Can you point me to the code where we check for stack
overflow/underflows? I couldn't find it.
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?
If we start assuming that edk2/kvm/qemu/kvmtool might be buggy in all
sort of ways then it will be hard to reason about many more things.
But I am not opposed to the idea of defining our own stack. In the next
revision, I will switch to a custom 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().
Yes that a good idea, I will try to merge the two setup() functions.
Thanks,
Nikos
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
