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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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: