| Message ID | 20220630100324.3153655-1-nikos.nikoleris@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | EFI and ACPI support for arm64 | expand |
Hi,
I've been trying to test the seris and I've come across some issues.
I've been using the target-efi-upstream-v3-rebased branch.
When compiling, I encounter this error:
gcc -mstrict-align -mno-outline-atomics -std=gnu99 -ffreestanding -O2 -I /path/to/kvm-unit-tests/lib -I /path/to/kvm-unit-tests/lib/libfdt -I lib -g -MMD -MF lib/arm/.timer.d -fno-strict-aliasing -fno-common -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Wno-missing-braces -Werror -fomit-frame-pointer -fno-stack-protector -Wno-frame-address -fno-pic -no-pie -Wclobbered -Wunused-but-set-parameter -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes -c -o lib/arm/timer.o lib/arm/timer.c
lib/arm/gic.c: In function ‘gic_init_acpi’:
lib/arm/gic.c:241:21: error: the comparison will always evaluate as ‘true’ for the address of ‘redist_base’ will never be NULL [-Werror=address]
241 | if (!gicv3_data.redist_base)
| ^
In file included from /path/to//kvm-unit-tests/lib/asm/gic-v3.h:1,
from /path/to//kvm-unit-tests/lib/asm/../../arm/asm/gic.h:43,
from /path/to//kvm-unit-tests/lib/asm/gic.h:1,
from lib/arm/gic.c:8:
/path/to//kvm-unit-tests/lib/asm/../../arm/asm/gic-v3.h:82:15: note: ‘redist_base’ declared here
82 | void *redist_base[NR_CPUS];
| ^~~~~~~~~~~
This happens with --enable-efi both set and unset (the above snippet is
from when I didn't specify --enable-efi).
For reference:
$ gcc --version
gcc (GCC) 12.1.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
I managed to fix the compilation error by commenting out the
gicv3_acpi_parse_madt_gicc call:
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 69521c3fde4f..66066ca84a96 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -179,6 +179,7 @@ static int gicv2_acpi_parse_madt_dist(struct acpi_subtable_header *header)
return 0;
}
+/*
static int gicv3_acpi_parse_madt_gicc(struct acpi_subtable_header *header)
{
struct acpi_madt_generic_interrupt *gicc = (void *)header;
@@ -195,6 +196,7 @@ static int gicv3_acpi_parse_madt_gicc(struct acpi_subtable_header *header)
return 0;
}
+*/
static int gicv3_acpi_parse_madt_dist(struct acpi_subtable_header *header)
{
@@ -238,9 +240,11 @@ static int gic_init_acpi(void)
gicv3_acpi_parse_madt_dist);
acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
gicv3_acpi_parse_madt_redist);
+ /*
if (!gicv3_data.redist_base)
acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
gicv3_acpi_parse_madt_gicc);
+ */
acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
gicv3_acpi_parse_madt_its);
I don't think this is the right fix, but I made the changes to get
kvm-unit-test to build.
The second error I'm encountering is when I try the selftest-setup test:
[..]
ProtectUefiImageCommon - 0x4D046040
- 0x000000004BEC4000 - 0x000000000001F600
SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008)
SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008)
SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008)
InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8
SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008)
Load address: 4bec4000
PC: 4beca400 PC offset: 6400
Unhandled exception ec=0x25 (DABT_EL1)
Vector: 4 (el1h_sync)
ESR_EL1: 96000000, ec=0x25 (DABT_EL1)
FAR_EL1: 0000fffffffff0f8 (valid)
Exception frame registers:
pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5
sp : 000000004f7ffe40
x29: 000000004f7ffff0 x28: 0000000000000000
x27: 000000004d046040 x26: 0000000000000000
x25: 0000000000000703 x24: 0000000000000050
x23: 0000000009011000 x22: 0000000000000000
x21: 000000000000001f x20: 0000fffffffff000
x19: 0000000043f92000 x18: 0000000000000000
x17: 00000000ffffa6ab x16: 000000004f513ebc
x15: 0000000000000002 x14: 000000004bed5000
x13: 000000004bee4000 x12: 000000004bed4000
x11: 000000004bec4000 x10: 000000004c03febc
x9 : 000000004bee2938 x8 : 0000000000000000
x7 : 0000000000000000 x6 : 000000004bee2900
x5 : 000000004bee2908 x4 : 0000000048000000
x3 : 0000000048000000 x2 : 000000004bee2928
x1 : 0000000000000003 x0 : ffffffffffffffff
EXIT: STATUS=127
The preceding lines were omitted for brevity, the entire log can be found
at [1] (expires in 6 months).
Command used to launch the test:
$ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256"
qemu has been built from source, tag v7.0.0, configured with:
$ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf
EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi
Wpa3 support in WifiConnectManager"):
$ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG
I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there
were no debug symbols in the output and it was impossible to figure what is
going on.
[1] https://pastebin.com/0mcap1BU
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@arm.com/
>
> 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
>
Hi Alex, On 19/07/2022 16:28, Alexandru Elisei wrote: > Hi, > > I've been trying to test the seris and I've come across some issues. > > I've been using the target-efi-upstream-v3-rebased branch. > > When compiling, I encounter this error: > > gcc -mstrict-align -mno-outline-atomics -std=gnu99 -ffreestanding -O2 -I /path/to/kvm-unit-tests/lib -I /path/to/kvm-unit-tests/lib/libfdt -I lib -g -MMD -MF lib/arm/.timer.d -fno-strict-aliasing -fno-common -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Wno-missing-braces -Werror -fomit-frame-pointer -fno-stack-protector -Wno-frame-address -fno-pic -no-pie -Wclobbered -Wunused-but-set-parameter -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes -c -o lib/arm/timer.o lib/arm/timer.c > lib/arm/gic.c: In function ‘gic_init_acpi’: > lib/arm/gic.c:241:21: error: the comparison will always evaluate as ‘true’ for the address of ‘redist_base’ will never be NULL [-Werror=address] > 241 | if (!gicv3_data.redist_base) > | ^ > In file included from /path/to//kvm-unit-tests/lib/asm/gic-v3.h:1, > from /path/to//kvm-unit-tests/lib/asm/../../arm/asm/gic.h:43, > from /path/to//kvm-unit-tests/lib/asm/gic.h:1, > from lib/arm/gic.c:8: > /path/to//kvm-unit-tests/lib/asm/../../arm/asm/gic-v3.h:82:15: note: ‘redist_base’ declared here > 82 | void *redist_base[NR_CPUS]; > | ^~~~~~~~~~~ > > This happens with --enable-efi both set and unset (the above snippet is > from when I didn't specify --enable-efi). > > For reference: > > $ gcc --version > gcc (GCC) 12.1.0 > Copyright (C) 2022 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > > I managed to fix the compilation error by commenting out the > gicv3_acpi_parse_madt_gicc call: > > diff --git a/lib/arm/gic.c b/lib/arm/gic.c > index 69521c3fde4f..66066ca84a96 100644 > --- a/lib/arm/gic.c > +++ b/lib/arm/gic.c > @@ -179,6 +179,7 @@ static int gicv2_acpi_parse_madt_dist(struct acpi_subtable_header *header) > return 0; > } > > +/* > static int gicv3_acpi_parse_madt_gicc(struct acpi_subtable_header *header) > { > struct acpi_madt_generic_interrupt *gicc = (void *)header; > @@ -195,6 +196,7 @@ static int gicv3_acpi_parse_madt_gicc(struct acpi_subtable_header *header) > > return 0; > } > +*/ > > static int gicv3_acpi_parse_madt_dist(struct acpi_subtable_header *header) > { > @@ -238,9 +240,11 @@ static int gic_init_acpi(void) > gicv3_acpi_parse_madt_dist); > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > gicv3_acpi_parse_madt_redist); > + /* > if (!gicv3_data.redist_base) > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > gicv3_acpi_parse_madt_gicc); > + */ > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > gicv3_acpi_parse_madt_its); > > I don't think this is the right fix, but I made the changes to get > kvm-unit-test to build. > Thanks, that's obviously a bug. I have a fix and I will include it in the next revision. This branch: https://github.com/relokin/kvm-unit-tests/tree/target-efi-upstream-v4 contains fixes for the feedback I received and I've been able to address so far. > The second error I'm encountering is when I try the selftest-setup test: > > [..] > ProtectUefiImageCommon - 0x4D046040 > - 0x000000004BEC4000 - 0x000000000001F600 > SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008) > SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008) > SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008) > InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8 > SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008) > SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008) > Load address: 4bec4000 > PC: 4beca400 PC offset: 6400 > Unhandled exception ec=0x25 (DABT_EL1) > Vector: 4 (el1h_sync) > ESR_EL1: 96000000, ec=0x25 (DABT_EL1) > FAR_EL1: 0000fffffffff0f8 (valid) > Exception frame registers: > pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5 > sp : 000000004f7ffe40 > x29: 000000004f7ffff0 x28: 0000000000000000 > x27: 000000004d046040 x26: 0000000000000000 > x25: 0000000000000703 x24: 0000000000000050 > x23: 0000000009011000 x22: 0000000000000000 > x21: 000000000000001f x20: 0000fffffffff000 > x19: 0000000043f92000 x18: 0000000000000000 > x17: 00000000ffffa6ab x16: 000000004f513ebc > x15: 0000000000000002 x14: 000000004bed5000 > x13: 000000004bee4000 x12: 000000004bed4000 > x11: 000000004bec4000 x10: 000000004c03febc > x9 : 000000004bee2938 x8 : 0000000000000000 > x7 : 0000000000000000 x6 : 000000004bee2900 > x5 : 000000004bee2908 x4 : 0000000048000000 > x3 : 0000000048000000 x2 : 000000004bee2928 > x1 : 0000000000000003 x0 : ffffffffffffffff > > > EXIT: STATUS=127 > > The preceding lines were omitted for brevity, the entire log can be found > at [1] (expires in 6 months). > > Command used to launch the test: > > $ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256" > > qemu has been built from source, tag v7.0.0, configured with: > > $ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf > > EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi > Wpa3 support in WifiConnectManager"): > > $ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG > > I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there > were no debug symbols in the output and it was impossible to figure what is > going on. > > [1] https://pastebin.com/0mcap1BU I haven't been to able to reproduce this. I've build from source qemu and EDK2 from source (the revisions you provided) and I've used gcc-10 to compile KUT but selftest-smp passes. Thanks, Nikos
Hi Nikos, On Fri, Jul 22, 2022 at 11:57:09AM +0100, Nikos Nikoleris wrote: > Hi Alex, > > On 19/07/2022 16:28, Alexandru Elisei wrote: > > Hi, > > > > I've been trying to test the seris and I've come across some issues. [..] > > > > The second error I'm encountering is when I try the selftest-setup test: > > > > [..] > > ProtectUefiImageCommon - 0x4D046040 > > - 0x000000004BEC4000 - 0x000000000001F600 > > SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008) > > SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008) > > SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008) > > InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8 > > SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008) > > SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008) > > Load address: 4bec4000 > > PC: 4beca400 PC offset: 6400 > > Unhandled exception ec=0x25 (DABT_EL1) > > Vector: 4 (el1h_sync) > > ESR_EL1: 96000000, ec=0x25 (DABT_EL1) > > FAR_EL1: 0000fffffffff0f8 (valid) > > Exception frame registers: > > pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5 > > sp : 000000004f7ffe40 > > x29: 000000004f7ffff0 x28: 0000000000000000 > > x27: 000000004d046040 x26: 0000000000000000 > > x25: 0000000000000703 x24: 0000000000000050 > > x23: 0000000009011000 x22: 0000000000000000 > > x21: 000000000000001f x20: 0000fffffffff000 > > x19: 0000000043f92000 x18: 0000000000000000 > > x17: 00000000ffffa6ab x16: 000000004f513ebc > > x15: 0000000000000002 x14: 000000004bed5000 > > x13: 000000004bee4000 x12: 000000004bed4000 > > x11: 000000004bec4000 x10: 000000004c03febc > > x9 : 000000004bee2938 x8 : 0000000000000000 > > x7 : 0000000000000000 x6 : 000000004bee2900 > > x5 : 000000004bee2908 x4 : 0000000048000000 > > x3 : 0000000048000000 x2 : 000000004bee2928 > > x1 : 0000000000000003 x0 : ffffffffffffffff > > > > > > EXIT: STATUS=127 > > > > The preceding lines were omitted for brevity, the entire log can be found > > at [1] (expires in 6 months). > > > > Command used to launch the test: > > > > $ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256" > > > > qemu has been built from source, tag v7.0.0, configured with: > > > > $ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf > > > > EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi > > Wpa3 support in WifiConnectManager"): > > > > $ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG > > > > I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there > > were no debug symbols in the output and it was impossible to figure what is > > going on. > > > > [1] https://pastebin.com/0mcap1BU > > I haven't been to able to reproduce this. I've build from source qemu and > EDK2 from source (the revisions you provided) and I've used gcc-10 to > compile KUT but selftest-smp passes. That's weird, I've compiled kvm-unit-tests with gcc 10.3.0 [1] and I'm still seeing the error (tried it on my x86 machine), for both selftest-setup selftest-smp. Did you compile qemu and edk2 with gcc 10.3.0? Or did you use some other compiler? [1] https://developer.arm.com/-/media/Files/downloads/gnu-a/10.3-2021.07/binrel/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu.tar.xz Thanks, Alex > > Thanks, > > Nikos >
Hi Alex, On 22/07/2022 15:41, Alexandru Elisei wrote: > Hi Nikos, > > On Fri, Jul 22, 2022 at 11:57:09AM +0100, Nikos Nikoleris wrote: >> Hi Alex, >> >> On 19/07/2022 16:28, Alexandru Elisei wrote: >>> Hi, >>> >>> I've been trying to test the seris and I've come across some issues. > [..] >>> >>> The second error I'm encountering is when I try the selftest-setup test: >>> >>> [..] >>> ProtectUefiImageCommon - 0x4D046040 >>> - 0x000000004BEC4000 - 0x000000000001F600 >>> SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008) >>> SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008) >>> SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008) >>> InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8 >>> SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008) >>> SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008) >>> Load address: 4bec4000 >>> PC: 4beca400 PC offset: 6400 >>> Unhandled exception ec=0x25 (DABT_EL1) >>> Vector: 4 (el1h_sync) >>> ESR_EL1: 96000000, ec=0x25 (DABT_EL1) >>> FAR_EL1: 0000fffffffff0f8 (valid) >>> Exception frame registers: >>> pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5 >>> sp : 000000004f7ffe40 >>> x29: 000000004f7ffff0 x28: 0000000000000000 >>> x27: 000000004d046040 x26: 0000000000000000 >>> x25: 0000000000000703 x24: 0000000000000050 >>> x23: 0000000009011000 x22: 0000000000000000 >>> x21: 000000000000001f x20: 0000fffffffff000 >>> x19: 0000000043f92000 x18: 0000000000000000 >>> x17: 00000000ffffa6ab x16: 000000004f513ebc >>> x15: 0000000000000002 x14: 000000004bed5000 >>> x13: 000000004bee4000 x12: 000000004bed4000 >>> x11: 000000004bec4000 x10: 000000004c03febc >>> x9 : 000000004bee2938 x8 : 0000000000000000 >>> x7 : 0000000000000000 x6 : 000000004bee2900 >>> x5 : 000000004bee2908 x4 : 0000000048000000 >>> x3 : 0000000048000000 x2 : 000000004bee2928 >>> x1 : 0000000000000003 x0 : ffffffffffffffff >>> >>> >>> EXIT: STATUS=127 >>> >>> The preceding lines were omitted for brevity, the entire log can be found >>> at [1] (expires in 6 months). >>> >>> Command used to launch the test: >>> >>> $ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256" >>> >>> qemu has been built from source, tag v7.0.0, configured with: >>> >>> $ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf >>> >>> EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi >>> Wpa3 support in WifiConnectManager"): >>> >>> $ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG >>> >>> I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there >>> were no debug symbols in the output and it was impossible to figure what is >>> going on. >>> >>> [1] https://pastebin.com/0mcap1BU >> >> I haven't been to able to reproduce this. I've build from source qemu and >> EDK2 from source (the revisions you provided) and I've used gcc-10 to >> compile KUT but selftest-smp passes. > > That's weird, I've compiled kvm-unit-tests with gcc 10.3.0 [1] and I'm still > seeing the error (tried it on my x86 machine), for both selftest-setup > selftest-smp. > > Did you compile qemu and edk2 with gcc 10.3.0? Or did you use some other > compiler? > > [1] https://developer.arm.com/-/media/Files/downloads/gnu-a/10.3-2021.07/binrel/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu.tar.xz > Thanks, I managed to reproduce this and found that with this configuration there are more mem_regions than the assumed max. To work around it you can change NR_EXTRA_MEM_REGIONS in lib/arm/setup.c to #define NR_EXTRA_MEM_REGIONS 32 Doubling it would hopefully be enough to run tests for now but I will also try to find out a better way to do this. Thanks, Nikos
Hi, Doubling the number of memory regions worked, thanks. Like you've said, that's just a band-aid, as there could be platforms out there for which UEFI reports more regions than the static value that kvm-unit-tests assumes. If you don't mind a suggestion, you could run two passes on the UEFI memory map: the first pass finds the largest available memory region and uses that for initializing the memory allocators (could also count the number of memory regions that it finds, for example), the second pass creates the mem_regions array by allocating it dynamically (the allocators have been initialized in the previous pass). The same approach could be used when booting without UEFI. Or you can just set NR_EXTRA_MEM_REGIONS to something very large and call it a day :) Thanks, Alex On Mon, Aug 01, 2022 at 07:23:05PM +0100, Nikos Nikoleris wrote: > Hi Alex, > > On 22/07/2022 15:41, Alexandru Elisei wrote: > > Hi Nikos, > > > > On Fri, Jul 22, 2022 at 11:57:09AM +0100, Nikos Nikoleris wrote: > > > Hi Alex, > > > > > > On 19/07/2022 16:28, Alexandru Elisei wrote: > > > > Hi, > > > > > > > > I've been trying to test the seris and I've come across some issues. > > [..] > > > > > > > > The second error I'm encountering is when I try the selftest-setup test: > > > > > > > > [..] > > > > ProtectUefiImageCommon - 0x4D046040 > > > > - 0x000000004BEC4000 - 0x000000000001F600 > > > > SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008) > > > > SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008) > > > > SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008) > > > > InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8 > > > > SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008) > > > > SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008) > > > > Load address: 4bec4000 > > > > PC: 4beca400 PC offset: 6400 > > > > Unhandled exception ec=0x25 (DABT_EL1) > > > > Vector: 4 (el1h_sync) > > > > ESR_EL1: 96000000, ec=0x25 (DABT_EL1) > > > > FAR_EL1: 0000fffffffff0f8 (valid) > > > > Exception frame registers: > > > > pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5 > > > > sp : 000000004f7ffe40 > > > > x29: 000000004f7ffff0 x28: 0000000000000000 > > > > x27: 000000004d046040 x26: 0000000000000000 > > > > x25: 0000000000000703 x24: 0000000000000050 > > > > x23: 0000000009011000 x22: 0000000000000000 > > > > x21: 000000000000001f x20: 0000fffffffff000 > > > > x19: 0000000043f92000 x18: 0000000000000000 > > > > x17: 00000000ffffa6ab x16: 000000004f513ebc > > > > x15: 0000000000000002 x14: 000000004bed5000 > > > > x13: 000000004bee4000 x12: 000000004bed4000 > > > > x11: 000000004bec4000 x10: 000000004c03febc > > > > x9 : 000000004bee2938 x8 : 0000000000000000 > > > > x7 : 0000000000000000 x6 : 000000004bee2900 > > > > x5 : 000000004bee2908 x4 : 0000000048000000 > > > > x3 : 0000000048000000 x2 : 000000004bee2928 > > > > x1 : 0000000000000003 x0 : ffffffffffffffff > > > > > > > > > > > > EXIT: STATUS=127 > > > > > > > > The preceding lines were omitted for brevity, the entire log can be found > > > > at [1] (expires in 6 months). > > > > > > > > Command used to launch the test: > > > > > > > > $ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256" > > > > > > > > qemu has been built from source, tag v7.0.0, configured with: > > > > > > > > $ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf > > > > > > > > EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi > > > > Wpa3 support in WifiConnectManager"): > > > > > > > > $ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG > > > > > > > > I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there > > > > were no debug symbols in the output and it was impossible to figure what is > > > > going on. > > > > > > > > [1] https://pastebin.com/0mcap1BU > > > > > > I haven't been to able to reproduce this. I've build from source qemu and > > > EDK2 from source (the revisions you provided) and I've used gcc-10 to > > > compile KUT but selftest-smp passes. > > > > That's weird, I've compiled kvm-unit-tests with gcc 10.3.0 [1] and I'm still > > seeing the error (tried it on my x86 machine), for both selftest-setup > > selftest-smp. > > > > Did you compile qemu and edk2 with gcc 10.3.0? Or did you use some other > > compiler? > > > > [1] https://developer.arm.com/-/media/Files/downloads/gnu-a/10.3-2021.07/binrel/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu.tar.xz > > > > Thanks, I managed to reproduce this and found that with this configuration > there are more mem_regions than the assumed max. To work around it you can > change NR_EXTRA_MEM_REGIONS in lib/arm/setup.c to > > #define NR_EXTRA_MEM_REGIONS 32 > > Doubling it would hopefully be enough to run tests for now but I will also > try to find out a better way to do this. > > Thanks, > > Nikos
On Tue, Aug 02, 2022 at 11:19:00AM +0100, Alexandru Elisei wrote: > Hi, > > Doubling the number of memory regions worked, thanks. Like you've said, > that's just a band-aid, as there could be platforms out there for which > UEFI reports more regions than the static value that kvm-unit-tests > assumes. > > If you don't mind a suggestion, you could run two passes on the UEFI memory > map: the first pass finds the largest available memory region and uses that > for initializing the memory allocators (could also count the number of > memory regions that it finds, for example), the second pass creates the > mem_regions array by allocating it dynamically (the allocators have been > initialized in the previous pass). The same approach could be used when > booting without UEFI. > > Or you can just set NR_EXTRA_MEM_REGIONS to something very large and call > it a day :) Yeah, let's just bump it to something large. We should also ensure it's easy to debug when we hit the limit though. We have an assert() in mem_region_add() already, but maybe we don't have an assert() in the EFI code paths? Thanks, drew > > Thanks, > Alex > > On Mon, Aug 01, 2022 at 07:23:05PM +0100, Nikos Nikoleris wrote: > > Hi Alex, > > > > On 22/07/2022 15:41, Alexandru Elisei wrote: > > > Hi Nikos, > > > > > > On Fri, Jul 22, 2022 at 11:57:09AM +0100, Nikos Nikoleris wrote: > > > > Hi Alex, > > > > > > > > On 19/07/2022 16:28, Alexandru Elisei wrote: > > > > > Hi, > > > > > > > > > > I've been trying to test the seris and I've come across some issues. > > > [..] > > > > > > > > > > The second error I'm encountering is when I try the selftest-setup test: > > > > > > > > > > [..] > > > > > ProtectUefiImageCommon - 0x4D046040 > > > > > - 0x000000004BEC4000 - 0x000000000001F600 > > > > > SetUefiImageMemoryAttributes - 0x000000004BEC4000 - 0x0000000000001000 (0x0000000000004008) > > > > > SetUefiImageMemoryAttributes - 0x000000004BEC5000 - 0x0000000000010000 (0x0000000000020008) > > > > > SetUefiImageMemoryAttributes - 0x000000004BED5000 - 0x000000000000F000 (0x0000000000004008) > > > > > InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 4F8014E8 > > > > > SetUefiImageMemoryAttributes - 0x000000004F640000 - 0x0000000000040000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004C2D0000 - 0x0000000000040000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004C280000 - 0x0000000000040000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004C230000 - 0x0000000000040000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004C140000 - 0x0000000000040000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004F600000 - 0x0000000000030000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004C040000 - 0x0000000000030000 (0x0000000000000008) > > > > > SetUefiImageMemoryAttributes - 0x000000004BFC0000 - 0x0000000000030000 (0x0000000000000008) > > > > > Load address: 4bec4000 > > > > > PC: 4beca400 PC offset: 6400 > > > > > Unhandled exception ec=0x25 (DABT_EL1) > > > > > Vector: 4 (el1h_sync) > > > > > ESR_EL1: 96000000, ec=0x25 (DABT_EL1) > > > > > FAR_EL1: 0000fffffffff0f8 (valid) > > > > > Exception frame registers: > > > > > pc : [<000000004beca400>] lr : [<000000004beca42c>] pstate: 400002c5 > > > > > sp : 000000004f7ffe40 > > > > > x29: 000000004f7ffff0 x28: 0000000000000000 > > > > > x27: 000000004d046040 x26: 0000000000000000 > > > > > x25: 0000000000000703 x24: 0000000000000050 > > > > > x23: 0000000009011000 x22: 0000000000000000 > > > > > x21: 000000000000001f x20: 0000fffffffff000 > > > > > x19: 0000000043f92000 x18: 0000000000000000 > > > > > x17: 00000000ffffa6ab x16: 000000004f513ebc > > > > > x15: 0000000000000002 x14: 000000004bed5000 > > > > > x13: 000000004bee4000 x12: 000000004bed4000 > > > > > x11: 000000004bec4000 x10: 000000004c03febc > > > > > x9 : 000000004bee2938 x8 : 0000000000000000 > > > > > x7 : 0000000000000000 x6 : 000000004bee2900 > > > > > x5 : 000000004bee2908 x4 : 0000000048000000 > > > > > x3 : 0000000048000000 x2 : 000000004bee2928 > > > > > x1 : 0000000000000003 x0 : ffffffffffffffff > > > > > > > > > > > > > > > EXIT: STATUS=127 > > > > > > > > > > The preceding lines were omitted for brevity, the entire log can be found > > > > > at [1] (expires in 6 months). > > > > > > > > > > Command used to launch the test: > > > > > > > > > > $ QEMU=/path/to/qemu/build/qemu-system-aarch64 EFI_UEFI=/path/to/QEMU_EFI.fd taskset -c 4-5 arm/efi/run arm/selftest.efi -smp 2 -m 256 -append "setup smp=2 mem=256" > > > > > > > > > > qemu has been built from source, tag v7.0.0, configured with: > > > > > > > > > > $ ./configure --target-list=aarch64-softmmu --disable-vnc --disable-gtk --disable-bpf > > > > > > > > > > EDK2 image has been built from commit e1eef3a8b01a ("NetworkPkg: Add Wi-Fi > > > > > Wpa3 support in WifiConnectManager"): > > > > > > > > > > $ build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc -b DEBUG > > > > > > > > > > I tried to disassemble selftest.efi: $ objdump -d selftest.efi, but there > > > > > were no debug symbols in the output and it was impossible to figure what is > > > > > going on. > > > > > > > > > > [1] https://pastebin.com/0mcap1BU > > > > > > > > I haven't been to able to reproduce this. I've build from source qemu and > > > > EDK2 from source (the revisions you provided) and I've used gcc-10 to > > > > compile KUT but selftest-smp passes. > > > > > > That's weird, I've compiled kvm-unit-tests with gcc 10.3.0 [1] and I'm still > > > seeing the error (tried it on my x86 machine), for both selftest-setup > > > selftest-smp. > > > > > > Did you compile qemu and edk2 with gcc 10.3.0? Or did you use some other > > > compiler? > > > > > > [1] https://developer.arm.com/-/media/Files/downloads/gnu-a/10.3-2021.07/binrel/gcc-arm-10.3-2021.07-x86_64-aarch64-none-linux-gnu.tar.xz > > > > > > > Thanks, I managed to reproduce this and found that with this configuration > > there are more mem_regions than the assumed max. To work around it you can > > change NR_EXTRA_MEM_REGIONS in lib/arm/setup.c to > > > > #define NR_EXTRA_MEM_REGIONS 32 > > > > Doubling it would hopefully be enough to run tests for now but I will also > > try to find out a better way to do this. > > > > Thanks, > > > > Nikos
On 02/08/2022 11:46, Andrew Jones wrote: > On Tue, Aug 02, 2022 at 11:19:00AM +0100, Alexandru Elisei wrote: >> Hi, >> >> Doubling the number of memory regions worked, thanks. Like you've said, >> that's just a band-aid, as there could be platforms out there for which >> UEFI reports more regions than the static value that kvm-unit-tests >> assumes. >> >> If you don't mind a suggestion, you could run two passes on the UEFI memory >> map: the first pass finds the largest available memory region and uses that >> for initializing the memory allocators (could also count the number of >> memory regions that it finds, for example), the second pass creates the >> mem_regions array by allocating it dynamically (the allocators have been >> initialized in the previous pass). The same approach could be used when >> booting without UEFI. >> >> Or you can just set NR_EXTRA_MEM_REGIONS to something very large and call >> it a day :) > > Yeah, let's just bump it to something large. We should also ensure it's > easy to debug when we hit the limit though. We have an assert() in > mem_region_add() already, but maybe we don't have an assert() in the > EFI code paths? > > Thanks, > drew > Thank you both. I'll make these two changes in the next revision of the series. By the way, I'm tracking changes on top of v3 here: https://github.com/relokin/kvm-unit-tests/tree/target-efi-upstream-v3-fixups Thanks, Nikos
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).
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@arm.com/
[2] https://lore.kernel.org/kvm/20220809091558.14379-20-alexandru.elisei@arm.com/
[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@arm.com/
>
> 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
>
On Tue, Aug 09, 2022, 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(). On the x86 side, efi_main() now runs with a KUT-controlled stack since commit d316d12a ("x86: efi: Provide a stack within testcase memory") > 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. Agreed. IMO, KUT should stop using UEFI-controlled data as early as possible. The original x86 behavior was effectively a temporary solution to get UEFI working without needing to simultaneously rework the common early boot flows. Side topic, I think the x86 code now has a benign bug. The old code contained an adjustment to RSP to undo some stack shenanigans (can't figure out why those shenanigans exist), but now the adjustment happens on the KUT stack, which doesn't need to be fixed up. It's a moot point since efi_main() should never return, but it looks odd. And it seems like KUT should intentionally explode if efi_main() returns, e.g. do this over two patches: diff --git a/x86/efi/crt0-efi-x86_64.S b/x86/efi/crt0-efi-x86_64.S index 1708ed55..e62891bc 100644 --- a/x86/efi/crt0-efi-x86_64.S +++ b/x86/efi/crt0-efi-x86_64.S @@ -62,10 +62,7 @@ _start: lea stacktop(%rip), %rsp call efi_main - addq $8, %rsp - -.exit: - ret + ud2 // hand-craft a dummy .reloc section so EFI knows it's a relocatable executable:
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. 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. 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@arm.com/ > [2] https://lore.kernel.org/kvm/20220809091558.14379-20-alexandru.elisei@arm.com/ > [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@arm.com/ >> >> 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 >>
Hi Sean, On Tue, Aug 09, 2022 at 03:29:58PM +0000, Sean Christopherson wrote: > On Tue, Aug 09, 2022, 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(). > > On the x86 side, efi_main() now runs with a KUT-controlled stack since commit > > d316d12a ("x86: efi: Provide a stack within testcase memory") > > > 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. > > Agreed. IMO, KUT should stop using UEFI-controlled data as early as possible. > The original x86 behavior was effectively a temporary solution to get UEFI working > without needing to simultaneously rework the common early boot flows. Yes, this is also what I am thinking, the stack is poorly specified in the specification because the specification doesn't expect an application to keep using it after calling EFI_BOOT_SERVICES.Exit(). Plus, using the UEFI allocated stack makes the test less reproducible, as even EDK2 today diverges from the spec wrt the stack, and other UEFI implementations might do things differently. And with just like all software, there might be bugs in the firmware. IMO, the more control kvm-unit-tests has over its resources, the more robust the tests are. What I was thinking is rewriting efi_main to return setup_efi(), something like this: void 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, not expected to return. */ setup_efi(&efi_bootinfo); } Which would allow all architectures to change their environment as they see fit, as setup_efi() is not expected to return. Architectures would have to be made aware of the efi_exit() function though. If you like that approach, I can give it a go, though I'm very rusty when it comes to x86. Thanks, Alex > > Side topic, I think the x86 code now has a benign bug. The old code contained an > adjustment to RSP to undo some stack shenanigans (can't figure out why those > shenanigans exist), but now the adjustment happens on the KUT stack, which doesn't > need to be fixed up. > > It's a moot point since efi_main() should never return, but it looks odd. And it > seems like KUT should intentionally explode if efi_main() returns, e.g. do this > over two patches: > > diff --git a/x86/efi/crt0-efi-x86_64.S b/x86/efi/crt0-efi-x86_64.S > index 1708ed55..e62891bc 100644 > --- a/x86/efi/crt0-efi-x86_64.S > +++ b/x86/efi/crt0-efi-x86_64.S > @@ -62,10 +62,7 @@ _start: > lea stacktop(%rip), %rsp > > call efi_main > - addq $8, %rsp > - > -.exit: > - ret > + ud2 > > // hand-craft a dummy .reloc section so EFI knows it's a relocatable executable:
On Wed, Aug 10, 2022, Alexandru Elisei wrote: > Hi Sean, > > On Tue, Aug 09, 2022 at 03:29:58PM +0000, Sean Christopherson wrote: > > On Tue, Aug 09, 2022, Alexandru Elisei wrote: > > > 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(). > > > > On the x86 side, efi_main() now runs with a KUT-controlled stack since commit > > > > d316d12a ("x86: efi: Provide a stack within testcase memory") > > > > > 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. > > > > Agreed. IMO, KUT should stop using UEFI-controlled data as early as possible. > > The original x86 behavior was effectively a temporary solution to get UEFI working > > without needing to simultaneously rework the common early boot flows. > > Yes, this is also what I am thinking, the stack is poorly specified in the > specification because the specification doesn't expect an application to > keep using it after calling EFI_BOOT_SERVICES.Exit(). Plus, using the UEFI > allocated stack makes the test less reproducible, as even EDK2 today > diverges from the spec wrt the stack, and other UEFI implementations might > do things differently. And with just like all software, there might be bugs > in the firmware. IMO, the more control kvm-unit-tests has over its > resources, the more robust the tests are. > > What I was thinking is rewriting efi_main to return setup_efi(), > something like this: > > void 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, not expected to return. */ > setup_efi(&efi_bootinfo); > } > > Which would allow all architectures to change their environment as they see > fit, as setup_efi() is not expected to return. Architectures would have to > be made aware of the efi_exit() function though. And they'd also have to invoke the test's main(). Why can't ARM switch to a KUT stack before calling efi_main()? The stack is a rather special case, I don't think it's unreasonable to require architectures to handle that before efi_main().
Hi Sean, On Wed, Aug 10, 2022 at 02:58:15PM +0000, Sean Christopherson wrote: > On Wed, Aug 10, 2022, Alexandru Elisei wrote: > > Hi Sean, > > > > On Tue, Aug 09, 2022 at 03:29:58PM +0000, Sean Christopherson wrote: > > > On Tue, Aug 09, 2022, Alexandru Elisei wrote: > > > > 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(). > > > > > > On the x86 side, efi_main() now runs with a KUT-controlled stack since commit > > > > > > d316d12a ("x86: efi: Provide a stack within testcase memory") > > > > > > > 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. > > > > > > Agreed. IMO, KUT should stop using UEFI-controlled data as early as possible. > > > The original x86 behavior was effectively a temporary solution to get UEFI working > > > without needing to simultaneously rework the common early boot flows. > > > > Yes, this is also what I am thinking, the stack is poorly specified in the > > specification because the specification doesn't expect an application to > > keep using it after calling EFI_BOOT_SERVICES.Exit(). Plus, using the UEFI > > allocated stack makes the test less reproducible, as even EDK2 today > > diverges from the spec wrt the stack, and other UEFI implementations might > > do things differently. And with just like all software, there might be bugs > > in the firmware. IMO, the more control kvm-unit-tests has over its > > resources, the more robust the tests are. > > > > What I was thinking is rewriting efi_main to return setup_efi(), > > something like this: > > > > void 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, not expected to return. */ > > setup_efi(&efi_bootinfo); > > } > > > > Which would allow all architectures to change their environment as they see > > fit, as setup_efi() is not expected to return. Architectures would have to > > be made aware of the efi_exit() function though. > > And they'd also have to invoke the test's main(). Why can't ARM switch to a KUT > stack before calling efi_main()? The stack is a rather special case, I don't think > it's unreasonable to require architectures to handle that before efi_main(). Sorry, missed the part from your earlier reply about x86 switching to a kvm-unit-tests controlled stack. I was just going to say that, when I saw your reply. I agree, it's entirely feasible for arm64 to switch to its own stack before calling efi_main(), in which case it can call efi_main() without any changes to efi_main(). Thanks for your input. Alex
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@arm.com/ 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@arm.com/ > > [2] https://lore.kernel.org/kvm/20220809091558.14379-20-alexandru.elisei@arm.com/ > > [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@arm.com/ > > > > > > 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 > > >
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@arm.com/ > > 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@arm.com/ >>> [2] https://lore.kernel.org/kvm/20220809091558.14379-20-alexandru.elisei@arm.com/ >>> [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@arm.com/ >>>> >>>> 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 >>>>
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@arm.com/ 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