[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4] Preserve the EFI System Resource Table for dom0
On 06.05.2022 12:59, Jan Beulich wrote: > On 05.05.2022 07:38, Demi Marie Obenour wrote: >> @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE >> ImageHandle, EFI_SYSTEM_TABLE *Syste >> if ( EFI_ERROR(status) ) >> PrintErrMesg(L"Cannot obtain memory map", status); >> >> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) >> + { >> + if ( !is_esrt_valid(efi_memmap + i) ) >> + continue; > > Instead of repeating the size calculation below, could you make the > function (with an altered name) simply return the size (and zero if > not [valid] ESRT), simplifying things below? > >> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type != >> + EfiRuntimeServicesData ) >> + { >> + /* ESRT needs to be moved to memory of type >> EfiRuntimeServicesData >> + * so that the memory it is in will not be used for other >> purposes */ > > Nit: Comment style. > >> + size_t esrt_size = offsetof(ESRT, Entries) + >> + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY); >> + void *new_esrt = NULL; >> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, >> esrt_size, &new_esrt); > > Nit: Please have a blank line between declaration(s) and statement(s). > >> + if ( status != EFI_SUCCESS ) >> + { >> + PrintErrMesg(L"Cannot allocate memory for ESRT", >> status); > > Neither this nor ... > >> + break; >> + } >> + memcpy(new_esrt, (void *)esrt, esrt_size); >> + status = efi_bs->InstallConfigurationTable(&esrt_guid, >> new_esrt); >> + if ( status != EFI_SUCCESS ) >> + { >> + PrintErrMesg(L"Cannot install new ESRT", status); >> + efi_bs->FreePool(new_esrt); > > ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg() > ends in blexit(). > >> + } >> + } >> + break; >> + } >> + >> efi_arch_process_memory_map(SystemTable, efi_memmap, >> efi_memmap_size, >> efi_mdesc_size, mdesc_ver); > > The allocation may have altered the memory map and hence invalidated what > was retrieved just before. You'd need to "continue;" without setting > "retry" to true, but then the question is why you make this allocation > after retrieving the memory map in the first place. It's not entirely > clear to me if it can be done _much_ earlier (if it can, doing it earlier > would of course be better), but since you need to do it before > ExitBootServices() anyway, and since you will need to call GetMemoryMap() > afterwards again, you could as well do it before calling GetMemoryMap(). Over lunch I figured that this was partly rubbish. Of course you need to do the check after GetMemoryMap(). But I still think it would be better if you moved this out of this function (or at the very least out of the loop) and not piggy-back on the ExitBootServices() retry mechanism. I'd be afraid this could end up in a single retry not being sufficient. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |