Re: [kvm-unit-tests PATCH v5 00/29] EFI and ACPI support for arm64

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

On 29/04/2023 17:21, Andrew Jones wrote:
> On Sat, Apr 29, 2023 at 06:18:25PM +0200, Andrew Jones wrote:
>> On Fri, Apr 28, 2023 at 01:03:36PM +0100, Nikos Nikoleris wrote:
>>> Hello,
>>>
>>> This series adds initial support for building arm64 tests as EFI
>>> apps 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. We add support for setting up the PSCI
>>> conduit, discovering the UART, timers, GIC 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
>>>     test starts EL1. This will be required to run EFI tests on sytems
>>>     that implement EL2.
>>>   - 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.
>>>   - Various fixes related to cache maintaince to better support turn the
>>>     MMU off.
>>>   - Switch to a new stack and avoid relying on the one provided by EFI.
>>>
>>> git branch: https://github.com/relokin/kvm-unit-tests/pull/new/target-efi-upstream-v5
>>>
>>> v4: https://lore.kernel.org/kvmarm/20230213101759.2577077-1-nikos.nikoleris@arm.com/
>>> v3: https://lore.kernel.org/all/20220630100324.3153655-1-nikos.nikoleris@arm.com/
>>> v2: https://lore.kernel.org/kvm/20220506205605.359830-1-nikos.nikoleris@arm.com/
>>>
>>> Changes in v5:
>>>   - Minor style changes (thanks Shaoqin).
>>>   - Avoid including lib/acpi.o to cflatobjs twice (thanks Drew).
>>>   - Increase NR_INITIAL_MEM_REGIONS to avoid overflows and add check when
>>>     we run out of space (thanks Shaoqin).
>>>
>>> Changes in v4:
>>>   - Removed patch that reworks cache maintenance when turning the MMU
>>>     off. This is not strictly required for EFI tests running with tcg and
>>>     will be addressed in a separate series by Alex.
>>>   - Fix compilation for arm (Alex).
>>>   - Convert ACPI tables to Linux style (Alex).
>>>
>>> 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!
>>
>> I'd like to get an ack from either Paolo or Sean on the changes to ACPI,
>> as they're shared with x86, and there are also some x86 code changes.
> 
> Actually, there are two build pipeline failures with the new ACPI code.
> Please take a look at
> 
> https://gitlab.com/jones-drew/kvm-unit-tests/-/pipelines/852864569
> 

Thanks for reviewing the series!

I think this fixes the compilation issues:

diff --git a/lib/acpi.h b/lib/acpi.h
index 202d832e..c330c877 100644
--- a/lib/acpi.h
+++ b/lib/acpi.h
@@ -292,7 +292,8 @@ struct acpi_table_gtdt {
         u32 platform_timer_offset;
  };

-#pragma pack(0)
+/* Reset to default packing */
+#pragma pack()

  void set_efi_rsdp(struct acpi_table_rsdp *rsdp);
  void *find_acpi_table_addr(u32 sig);
diff --git a/lib/acpi.c b/lib/acpi.c
index 760cd8b2..0440cddb 100644
--- a/lib/acpi.c
+++ b/lib/acpi.c
@@ -70,7 +70,7 @@ void *find_acpi_table_addr(u32 sig)
                 return rsdt;

         if (rsdp->revision >= 2) {
-               xsdt = (void *)rsdp->xsdt_physical_address;
+               xsdt = (void *)(ulong) rsdp->xsdt_physical_address;
                 if (xsdt && xsdt->signature != XSDT_SIGNATURE)
                         xsdt = NULL;
         }

> Thanks,
> drew
> 
>>
>> Also,
>>
>>    1) It'd be nice if this worked with DT, too. We can use UEFI with DT
>>       when adding '-no-acpi' to the QEMU command line. setup_efi() needs
>>       to learn how to find the dtb and most the '#ifdef CONFIG_EFI's
>>       would need to change to a new CONFIG_ACPI guard.
>>

I had a quick look at it at some point and it didn't look 
straightforward but I'll check again.

>>    2) The debug bp and ss tests fail with EFI, but not without, for me.
>>

I think, I've found the problem, the patch below fixes it for me.

diff --git a/arm/debug.c b/arm/debug.c
index b3e9749c..126fa267 100644
--- a/arm/debug.c
+++ b/arm/debug.c
@@ -292,11 +292,14 @@ static noinline void test_hw_bp(bool migrate)
         hw_bp_idx = 0;

         /* Trap on up to 16 debug exception unmask instructions. */
-       asm volatile("hw_bp0:\n"
-            "msr daifclr, #8; msr daifclr, #8; msr daifclr, #8; msr 
daifclr, #8\n"
-            "msr daifclr, #8; msr daifclr, #8; msr daifclr, #8; msr 
daifclr, #8\n"
-            "msr daifclr, #8; msr daifclr, #8; msr daifclr, #8; msr 
daifclr, #8\n"
-            "msr daifclr, #8; msr daifclr, #8; msr daifclr, #8; msr 
daifclr, #8\n");
+       asm volatile(
+               ".globl hw_bp0\n"
+               "hw_bp0:\n"
+                       "msr daifclr, #8; msr daifclr, #8; msr daifclr, 
#8; msr daifclr, #8\n"
+                       "msr daifclr, #8; msr daifclr, #8; msr daifclr, 
#8; msr daifclr, #8\n"
+                       "msr daifclr, #8; msr daifclr, #8; msr daifclr, 
#8; msr daifclr, #8\n"
+                       "msr daifclr, #8; msr daifclr, #8; msr daifclr, 
#8; msr daifclr, #8\n"
+               );

         for (i = 0, addr = (uint64_t)&hw_bp0; i < num_bp; i++, addr += 4)
                 report(hw_bp_addr[i] == addr, "hw breakpoint: %d", i);
@@ -367,11 +370,14 @@ static noinline void test_ss(bool migrate)

         asm volatile("msr daifclr, #8");

-       asm volatile("ss_start:\n"
+       asm volatile(
+               ".globl ss_start\n"
+               "ss_start:\n"
                         "mrs x0, esr_el1\n"
                         "add x0, x0, #1\n"
                         "msr daifset, #8\n"
-                       : : : "x0");
+                       : : : "x0"
+               );

         report(ss_addr[0] == (uint64_t)&ss_start, "single step");
  }

>>    3) The timer test runs (and succeeds) when run with
>>       './arm/efi/run ./arm/timer.efi', but not when run with
>>       './run_tests.sh -g timer'. This is because UEFI takes
>>       up all the given timeout time (10s), and then the test times out.
>>       The hackyish fix below resolves it for me. I'll consider posting it
>>       as a real patch
>>

I see. I didn't hit the timeout on my test machine but I tried on a 
slower machine and I did.

New branch with the fixups here:

https://github.com/relokin/kvm-unit-tests/pull/new/target-efi-upstream-v5

Many thanks,

Nikos

>> Thanks,
>> drew
>>
>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>> index 51e4b97b27d1..72ce718b1170 100644
>> --- a/scripts/arch-run.bash
>> +++ b/scripts/arch-run.bash
>> @@ -94,7 +94,17 @@ run_qemu_status ()
>>   
>>   timeout_cmd ()
>>   {
>> +	local s
>> +
>>   	if [ "$TIMEOUT" ] && [ "$TIMEOUT" != "0" ]; then
>> +		if [ "$CONFIG_EFI" = 'y' ]; then
>> +			s=${TIMEOUT: -1}
>> +			if [ "$s" = 's' ]; then
>> +				TIMEOUT=${TIMEOUT:0:-1}
>> +				((TIMEOUT += 10)) # Add 10 seconds for booting UEFI
>> +				TIMEOUT="${TIMEOUT}s"
>> +			fi
>> +		fi
>>   		echo "timeout -k 1s --foreground $TIMEOUT"
>>   	fi
>>   }