On Fri, May 06, 2022 at 12:59:05PM +0200, Jan Beulich wrote: > On 05.05.2022 07:38, Demi Marie Obenour wrote: > > @@ -1056,13 +1091,11 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > > EFI_STATUS status; > > UINTN info_size = 0, map_key; > > bool retry; > > -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP > > unsigned int i; > > -#endif > > > > efi_bs->GetMemoryMap(&info_size, NULL, &map_key, > > &efi_mdesc_size, &mdesc_ver); > > - info_size += 8 * efi_mdesc_size; > > + info_size += 8 * (efi_mdesc_size + 1); > > I think I did ask on an earlier version already why you're making this > change. It continues to look to me like a leftover which was needed by > an early version only. Will revert in v5. > > @@ -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? Will fix in v5. > > + 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. Will fix in v5. > > + 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). Will fix in v5. > > + 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(). Whoops! I did not realized PrintErrMsg() was fatal. > > + } > > + } > > + 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(). This would mean the allocation would need to be unconditional. Right now, I avoid the allocation if it is not necessary. > > --- a/xen/common/efi/efi.h > > +++ b/xen/common/efi/efi.h > > @@ -10,6 +10,23 @@ > > #include > > #include > > > > +typedef struct _ESRT_ENTRY { > > + EFI_GUID FwClass; > > + UINT32 FwType; > > + UINT32 FwVersion; > > + UINT32 FwLowestSupportedVersion; > > + UINT32 FwCapsuleFlags; > > + UINT32 FwLastAttemptVersion; > > + UINT32 FwLastAttemptStatus; > > +} ESRT_ENTRY; > > + > > +typedef struct _ESRT { > > + UINT32 Count; > > + UINT32 Max; > > + UINT64 Version; > > + ESRT_ENTRY Entries[]; > > +} ESRT; > > I'm pretty sure I did indicate before that types used in just a single > source file should be put in that source file, unless we obtain them > by importing a header (e.g. the ones in include/efi/) from elsewhere. Will fix in v5. > > --- a/xen/common/efi/runtime.c > > +++ b/xen/common/efi/runtime.c > > @@ -269,7 +269,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > > case XEN_FW_EFI_MEM_INFO: > > for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > > { > > - EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > + const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT; > > > > if ( info->mem.addr >= desc->PhysicalStart && > > With the restructured approach I don't think this stray change should > be left in here anymore. Or am I overlooking anything requiring this > adjustment? It’s a trivial cleanup but I can get rid of it. > > --- a/xen/include/efi/efiapi.h > > +++ b/xen/include/efi/efiapi.h > > @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES { > > #define SAL_SYSTEM_TABLE_GUID \ > > { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} } > > > > +#define ESRT_GUID \ > > + { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} } > > Like above I'm pretty sure I did ask that you do not alter this > imported header. If gnu-efi now has these definitions, we should > import them all in one go (i.e. then the two struct declarations > would also want to go into their appropriate place under include/efi/. > Otherwise this wants putting next to the other GUIDs defined in > boot.c. Will fix in v5. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab