On Mon, Oct 4, 2021 at 6:23 AM Andrew Jones <drjones@xxxxxxxxxx> 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
