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

[Andrew Jones <andrew.jones@linux.dev>]

On Sun, Apr 30, 2023 at 01:02:42AM +0100, Nikos Nikoleris wrote:
> 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

Thanks for the quick fixes. Can you update your tree and make an MR? I
no longer use github.com/rhdrjones/kvm-unit-tests. I use

https://gitlab.com/jones-drew/kvm-unit-tests.git

Thanks,
drew

> 
> 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
> > >   }