Date: Fri, 27 Sep 2019 05:12:29 +0000 (UTC) From: Rebecca Cran <bcran@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r352788 - stable/12/stand/efi/loader Message-ID: <201909270512.x8R5CT35045417@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bcran Date: Fri Sep 27 05:12:28 2019 New Revision: 352788 URL: https://svnweb.freebsd.org/changeset/base/352788 Log: MFC r344839: Add retry loop around GetMemoryMap call to fix fragmentation bug The call to BS->AllocatePages can cause the memory map to become framented, causing BS->GetMemoryMap to return EFI_BUFFER_TOO_SMALL more than once. For example this can happen on the MinnowBoard Turbot, causing the boot to stop with an error. Avoid this by calling GetMemoryMap in a loop. Modified: stable/12/stand/efi/loader/bootinfo.c stable/12/stand/efi/loader/copy.c Modified: stable/12/stand/efi/loader/bootinfo.c ============================================================================== --- stable/12/stand/efi/loader/bootinfo.c Fri Sep 27 02:09:20 2019 (r352787) +++ stable/12/stand/efi/loader/bootinfo.c Fri Sep 27 05:12:28 2019 (r352788) @@ -287,12 +287,12 @@ static int bi_load_efi_data(struct preloaded_file *kfp) { EFI_MEMORY_DESCRIPTOR *mm; - EFI_PHYSICAL_ADDRESS addr; + EFI_PHYSICAL_ADDRESS addr = 0; EFI_STATUS status; const char *efi_novmap; size_t efisz; UINTN efi_mapkey; - UINTN mmsz, pages, retry, sz; + UINTN dsz, pages, retry, sz; UINT32 mmver; struct efi_map_header *efihdr; bool do_vmap; @@ -323,76 +323,94 @@ bi_load_efi_data(struct preloaded_file *kfp) efisz = (sizeof(struct efi_map_header) + 0xf) & ~0xf; /* - * Assgin size of EFI_MEMORY_DESCRIPTOR to keep compatible with + * Assign size of EFI_MEMORY_DESCRIPTOR to keep compatible with * u-boot which doesn't fill this value when buffer for memory * descriptors is too small (eg. 0 to obtain memory map size) */ - mmsz = sizeof(EFI_MEMORY_DESCRIPTOR); + dsz = sizeof(EFI_MEMORY_DESCRIPTOR); /* - * It is possible that the first call to ExitBootServices may change - * the map key. Fetch a new map key and retry ExitBootServices in that - * case. + * Allocate enough pages to hold the bootinfo block and the + * memory map EFI will return to us. The memory map has an + * unknown size, so we have to determine that first. Note that + * the AllocatePages call can itself modify the memory map, so + * we have to take that into account as well. The changes to + * the memory map are caused by splitting a range of free + * memory into two, so that one is marked as being loader + * data. */ + + sz = 0; + + /* + * Matthew Garrett has observed at least one system changing the + * memory map when calling ExitBootServices, causing it to return an + * error, probably because callbacks are allocating memory. + * So we need to retry calling it at least once. + */ for (retry = 2; retry > 0; retry--) { - /* - * Allocate enough pages to hold the bootinfo block and the - * memory map EFI will return to us. The memory map has an - * unknown size, so we have to determine that first. Note that - * the AllocatePages call can itself modify the memory map, so - * we have to take that into account as well. The changes to - * the memory map are caused by splitting a range of free - * memory into two (AFAICT), so that one is marked as being - * loader data. - */ - sz = 0; - BS->GetMemoryMap(&sz, NULL, &efi_mapkey, &mmsz, &mmver); - sz += mmsz; - sz = (sz + 0xf) & ~0xf; - pages = EFI_SIZE_TO_PAGES(sz + efisz); - status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, - pages, &addr); - if (EFI_ERROR(status)) { - printf("%s: AllocatePages error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (ENOMEM); - } + for (;;) { + status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &dsz, &mmver); + if (!EFI_ERROR(status)) + break; - /* - * Read the memory map and stash it after bootinfo. Align the - * memory map on a 16-byte boundary (the bootinfo block is page - * aligned). - */ - efihdr = (struct efi_map_header *)(uintptr_t)addr; - mm = (void *)((uint8_t *)efihdr + efisz); - sz = (EFI_PAGE_SIZE * pages) - efisz; + if (status != EFI_BUFFER_TOO_SMALL) { + printf("%s: GetMemoryMap error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (EINVAL); + } - status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &mmsz, &mmver); - if (EFI_ERROR(status)) { - printf("%s: GetMemoryMap error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (EINVAL); - } - status = BS->ExitBootServices(IH, efi_mapkey); - if (EFI_ERROR(status) == 0) { + if (addr != 0) + BS->FreePages(addr, pages); + + /* Add 10 descriptors to the size to allow for + * fragmentation caused by calling AllocatePages */ + sz += (10 * dsz); + pages = EFI_SIZE_TO_PAGES(sz + efisz); + status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, + pages, &addr); + if (EFI_ERROR(status)) { + printf("%s: AllocatePages error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (ENOMEM); + } + /* - * This may be disabled by setting efi_disable_vmap in - * loader.conf(5). By default we will setup the virtual - * map entries. + * Read the memory map and stash it after bootinfo. Align the + * memory map on a 16-byte boundary (the bootinfo block is page + * aligned). */ - if (do_vmap) - efi_do_vmap(mm, sz, mmsz, mmver); - efihdr->memory_size = sz; - efihdr->descriptor_size = mmsz; - efihdr->descriptor_version = mmver; - file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, - efihdr); - return (0); + efihdr = (struct efi_map_header *)(uintptr_t)addr; + mm = (void *)((uint8_t *)efihdr + efisz); + sz = (EFI_PAGE_SIZE * pages) - efisz; } + + status = BS->ExitBootServices(IH, efi_mapkey); + if (!EFI_ERROR(status)) + break; + } + + if (retry == 0) { BS->FreePages(addr, pages); + printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); + return (EINVAL); } - printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); - return (EINVAL); + + /* + * This may be disabled by setting efi_disable_vmap in + * loader.conf(5). By default we will setup the virtual + * map entries. + */ + + if (do_vmap) + efi_do_vmap(mm, sz, dsz, mmver); + efihdr->memory_size = sz; + efihdr->descriptor_size = dsz; + efihdr->descriptor_version = mmver; + file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, + efihdr); + + return (0); } /* Modified: stable/12/stand/efi/loader/copy.c ============================================================================== --- stable/12/stand/efi/loader/copy.c Fri Sep 27 02:09:20 2019 (r352787) +++ stable/12/stand/efi/loader/copy.c Fri Sep 27 05:12:28 2019 (r352788) @@ -95,7 +95,7 @@ static void efi_verify_staging_size(unsigned long *nr_pages) { UINTN sz; - EFI_MEMORY_DESCRIPTOR *map, *p; + EFI_MEMORY_DESCRIPTOR *map = NULL, *p; EFI_PHYSICAL_ADDRESS start, end; UINTN key, dsz; UINT32 dver; @@ -104,17 +104,28 @@ efi_verify_staging_size(unsigned long *nr_pages) unsigned long available_pages = 0; sz = 0; - status = BS->GetMemoryMap(&sz, 0, &key, &dsz, &dver); - if (status != EFI_BUFFER_TOO_SMALL) { - printf("Can't determine memory map size\n"); - return; - } - map = malloc(sz); - status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); - if (EFI_ERROR(status)) { - printf("Can't read memory map\n"); - goto out; + for (;;) { + status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); + if (!EFI_ERROR(status)) + break; + + if (status != EFI_BUFFER_TOO_SMALL) { + printf("Can't read memory map: %lu\n", + EFI_ERROR_CODE(status)); + goto out; + } + + free(map); + + /* Allocate 10 descriptors more than the size reported, + * to allow for any fragmentation caused by calling + * malloc */ + map = malloc(sz + (10 * dsz)); + if (map == NULL) { + printf("Unable to allocate memory\n"); + goto out; + } } ndesc = sz / dsz;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201909270512.x8R5CT35045417>