Re: [kvm-unit-tests PATCH v2 08/17] x86 UEFI: Set up RSDP after UEFI boot up

From: Zixuan Wang <zxwang42@gmail.com>
To: Andrew Jones <drjones@redhat.com>
Cc: 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 08/17] x86 UEFI: Set up RSDP after UEFI boot up
Date: Mon, 4 Oct 2021 14:58:29 -0700	[thread overview]
Message-ID: <CAEDJ5ZRy8ywXjujoMVHem8igBq-2s0HzN2Gt6f76cLbZzCkr9g@mail.gmail.com> (raw)
In-Reply-To: <20211004132102.lfsyabk2daeiejkx@gator>

On Mon, Oct 4, 2021 at 6:23 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Aug 27, 2021 at 03:12:13AM +0000, Zixuan Wang wrote:
> > Root system description pointer (RSDP) is a data structure used in the
> > ACPI programming interface. In BIOS, RSDP is located within a
> > predefined memory area, so a program can scan the memory area and find
> > RSDP. But in UEFI, RSDP may not appear in that memory area, instead, a
> > program should find it in the EFI system table.
> >
> > +typedef union {
> > +     struct {
> > +             efi_guid_t guid;
> > +             void *table;
> > +     };
> > +     efi_config_table_32_t mixed_mode;
>
> Can't we drop all the mixed_mode stuff? Or do we really want to support
> 32-bit UEFI kvm-unit-tests?

We are currently not considering the 32-bit UEFI support. I will drop
the mixed_mode code in the next version.

> > +void setup_efi_rsdp(struct rsdp_descriptor *rsdp) {
> > +     efi_rsdp = rsdp;
> > +}
>
> { on its own line please

Got it! I will fix it in the next version.

> > +
> > +static struct rsdp_descriptor *get_rsdp(void) {
> > +     if (efi_rsdp == NULL) {
> > +             printf("Can't find RSDP from UEFI, maybe setup_efi_rsdp() was not called\n");
> > +     }
> > +     return efi_rsdp;
> > +}
> > +#else
> > +static struct rsdp_descriptor *get_rsdp(void) {
> > +    struct rsdp_descriptor *rsdp;
> > +    unsigned long addr;
> > +    for(addr = 0xf0000; addr < 0x100000; addr += 16) {
> > +     rsdp = (void*)addr;
> > +     if (rsdp->signature == RSDP_SIGNATURE_8BYTE)
> > +          break;
> > +    }
>
> When moving code please take the opportunity to clean up its style.

Got it! I will fix this in the next version.

> > +#ifdef TARGET_EFI
> > +void setup_efi_rsdp(struct rsdp_descriptor *rsdp);
> > +#endif /* TARGET_EFI */
>
> Unnecessary ifdef.

I previously thought it's safer to use ifdef here, as the function is
only defined in the UEFI kvm-unit-tests.

I will drop it in the next version. Dropping it does not affect the
compilation of Multiboot kvm-unit-tests.

> > @@ -255,6 +267,7 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
> >       enable_x2apic();
> >       smp_init();
> >       phys_alloc_init(efi_bootinfo->free_mem_start, efi_bootinfo->free_mem_size);
> > +     setup_efi_rsdp(efi_bootinfo->rsdp);
>
> What memory region is this table in? We should make sure it's reserved or
> copy the table out to somewhere that is reserved.

I will double-check it in the next version. I will add a comment to
this line if it's already in reserved memory. Otherwise, I will copy
it to a reserved location.

> >  }
> >
> >  #endif /* TARGET_EFI */
> > --
> > 2.33.0.259.gc128427fd7-goog
> >
>
> I'd much prefer we avoid too much of this split setup where we have a bit
> of setup in a common efi lib and then an x86 specific part that populates
> an x86 specific info structure before exiting boot services and then more
> x86 specific setup that uses that later...
>
> Can't we do almost everything in lib/efi.c and only call out once into an
> arch_efi_setup function after exiting boot services?
>
> Thanks,
> drew
>

Indeed, I will refactor this part in the next version. I will move the
arch-neutral functions to lib/efi.c and pass an EFI system info data
structure to the arch-specific function for additional setup.

Best regards,
Zixuan