Re: [kvm-unit-tests PATCH v2 04/17] x86 UEFI: Boot from UEFI

From: Zixuan Wang <zxwang42@gmail.com>
To: Andrew Jones <drjones@redhat.com>
Cc: Zixuan Wang <zixuanwang@google.com>,
	kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Marc Orr <marcorr@google.com>,
	"Hyunwook (Wooky) Baek" <baekhw@google.com>,
	Tom Roeder <tmroeder@google.com>,
	erdemaktas@google.com, rientjes@google.com, seanjc@google.com,
	brijesh.singh@amd.com, Thomas.Lendacky@amd.com,
	varad.gautam@suse.com, jroedel@suse.de, bp@suse.de
Subject: Re: [kvm-unit-tests PATCH v2 04/17] x86 UEFI: Boot from UEFI
Date: Mon, 4 Oct 2021 14:30:31 -0700	[thread overview]
Message-ID: <CAEDJ5ZScDTVaWa9EP-k-_tkv8orYiYFuwoQwPoPqiQ23rSfsvg@mail.gmail.com> (raw)
In-Reply-To: <20211004125541.bf76snknn3umwsfe@gator>

On Mon, Oct 4, 2021 at 5:58 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Aug 27, 2021 at 03:12:09AM +0000, Zixuan Wang wrote:
> > This commit provides initial support for x86 test cases to boot from
> > -unsigned long __efiapi efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> > +efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> >  {
> > +     int ret;
> > +
> >       efi_system_table = sys_tab;
> >
> > -     return 0;
> > +     setup_efi();
> > +     ret = main(__argc, __argv, __environ);
>
> In my new AArch64 PoC I call an 'efi_setup' from 'start' in cstart64.S
> and then when that returns 'main' is called like normal.
>

x86 is a bit tricky: the cstart64.S 'start' code is compiled to run in
real mode, but C functions are compiled for long mode. From my
experience, calling a long mode function from real mode can crash the
guest VM. So we developed a new start-up assembly that directly starts
from long mode.

I remember Varad's original patchset [1] calls the EFI setup function
from cstart64.S, which introduces several ifdefs to the file.

[1] https://lore.kernel.org/kvm/20210819113400.26516-1-varad.gautam@suse.com/

> > -#include <efi.h>
> > -#include <efilib.h>
> > -
> > +#include "linux/uefi.h"
> > +#include "efi.h"
> >  #include <elf.h>
> >
> > -EFI_STATUS _relocate (long ldbase, Elf64_Dyn *dyn,
> > -                   EFI_HANDLE image EFI_UNUSED,
> > -                   EFI_SYSTEM_TABLE *systab EFI_UNUSED)
> > +efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t handle, efi_system_table_t *sys_tab)
>
> I think these conversions should be done as separate patches after the
> import patches.
>

Got it! I will add this to the next version.

> > +EFI_RUN=y \
> > +"$TEST_DIR/run" \
> > +     -drive file="$EFI_UEFI",format=raw,if=pflash \
>
> Also need the 'readonly' property on this drive.
>

I didn't notice this. I will add it to the next version. Thanks for
pointing it out.

> > --
> > 2.33.0.259.gc128427fd7-goog
> >
>
> Thanks,
> drew
>

Best regards,
Zixuan