Page MenuHomeFreeBSD
Differential D19341 Diff 54450 stand/efi/loader/bootinfo.c

Changeset View

Standalone View

View Options

stand/efi/loader/bootinfo.c

Show First 20 Lines • Show All 281 Lines • ▼ Show 20 Linesefi_do_vmap(EFI_MEMORY_DESCRIPTOR *mm, UINTN sz, UINTN mmsz, UINT32 mmver)
free(vmap); free(vmap);
return (ret); return (ret);
} }
static int static int
bi_load_efi_data(struct preloaded_file *kfp) bi_load_efi_data(struct preloaded_file *kfp)
{ {
EFI_MEMORY_DESCRIPTOR *mm; EFI_MEMORY_DESCRIPTOR *mm;
EFI_PHYSICAL_ADDRESS addr; EFI_PHYSICAL_ADDRESS addr = 0;
EFI_STATUS status; EFI_STATUS status;
const char *efi_novmap; const char *efi_novmap;
size_t efisz; size_t efisz;
UINTN efi_mapkey; UINTN efi_mapkey;
UINTN mmsz, pages, retry, sz; UINTN mmsz, pages, retry, sz;
UINT32 mmver; UINT32 mmver;
struct efi_map_header *efihdr; struct efi_map_header *efihdr;
bool do_vmap; bool do_vmap;
Show All 19 Lines#endif
do_vmap = true; do_vmap = true;
efi_novmap = getenv("efi_disable_vmap"); efi_novmap = getenv("efi_disable_vmap");
if (efi_novmap != NULL) if (efi_novmap != NULL)
do_vmap = strcasecmp(efi_novmap, "YES") != 0; do_vmap = strcasecmp(efi_novmap, "YES") != 0;
efisz = (sizeof(struct efi_map_header) + 0xf) & ~0xf; 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 * u-boot which doesn't fill this value when buffer for memory
* descriptors is too small (eg. 0 to obtain memory map size) * descriptors is too small (eg. 0 to obtain memory map size)
*/ */
mmsz = sizeof(EFI_MEMORY_DESCRIPTOR); mmsz = 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.
*/
for (retry = 2; retry > 0; retry--) {
/*
* Allocate enough pages to hold the bootinfo block and the * Allocate enough pages to hold the bootinfo block and the
* memory map EFI will return to us. The memory map has an * memory map EFI will return to us. The memory map has an
* unknown size, so we have to determine that first. Note that * unknown size, so we have to determine that first. Note that
* the AllocatePages call can itself modify the memory map, so * the AllocatePages call can itself modify the memory map, so
* we have to take that into account as well. The changes to * we have to take that into account as well. The changes to
* the memory map are caused by splitting a range of free * the memory map are caused by splitting a range of free
* memory into two (AFAICT), so that one is marked as being * memory into two, so that one is marked as being loader
* loader data. * data.
*/ */
sz = 0; sz = 0;
BS->GetMemoryMap(&sz, NULL, &efi_mapkey, &mmsz, &mmver);
sz += mmsz; for (retry = 10; retry > 0; retry--) {
impUnsubmitted
Done
Inline Actions

/*

  • Matthew...

is the style.

imp: /* * Matthew... is the style.
bcranAuthorUnsubmitted
Done
Inline Actions

Fixed.

bcran: Fixed.
status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &mmsz, &mmver);
if (!EFI_ERROR(status))
break;
if (status != EFI_BUFFER_TOO_SMALL) {
printf("%s: GetMemoryMap error %lu\n", __func__,
EFI_ERROR_CODE(status));
return (EINVAL);
}
if (addr != 0)
BS->FreePages(addr, pages);
/* Add 10 descriptors to the size to allow for
* fragmentation caused by calling AllocatePages */
sz += (10 * mmsz);
sz = (sz + 0xf) & ~0xf; sz = (sz + 0xf) & ~0xf;
pages = EFI_SIZE_TO_PAGES(sz + efisz); pages = EFI_SIZE_TO_PAGES(sz + efisz);
status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData,
pages, &addr); pages, &addr);
if (EFI_ERROR(status)) { if (EFI_ERROR(status)) {
printf("%s: AllocatePages error %lu\n", __func__, printf("%s: AllocatePages error %lu\n", __func__,
EFI_ERROR_CODE(status)); EFI_ERROR_CODE(status));
return (ENOMEM); return (ENOMEM);
} }
tsoomeUnsubmitted
Done
Inline Actions

You can make it simpler - this could be endless loop (while (true)), and we:

  1. return if GetMemoryMap() will return error other than EFI_BUFFER_TOO_SMALL
  2. break if GetMemoryMap() had no error
  3. return if AllocatePages() will fail.
tsoome: You can make it simpler - this could be endless loop (while (true)), and we: 1. return if…
bcranAuthorUnsubmitted
Done
Inline Actions

Hmm, it probably _is_ safe to make it an infinite loop since I don't see a way it could end up looping forever.

bcran: Hmm, it probably _is_ safe to make it an infinite loop since I don't see a way it could end up…
/* /*
* Read the memory map and stash it after bootinfo. Align the * Read the memory map and stash it after bootinfo. Align the
tsoomeUnsubmitted
Done
Inline Actions

If you move if (!EFI_ERROR(status)) check before this line, we can just have if (status != EFI_BUFFER_TOO_SMALL)

tsoome: If you move if (!EFI_ERROR(status)) check before this line, we can just have if (status !=…
* memory map on a 16-byte boundary (the bootinfo block is page * memory map on a 16-byte boundary (the bootinfo block is page
* aligned). * aligned).
*/ */
impUnsubmitted
Done
Inline Actions

can we call printf if we can't get memory?

imp: can we call printf if we can't get memory?
bcranAuthorUnsubmitted
Done
Inline Actions

Yes, because printf etc. use a preallocated 64MB heap.

bcran: Yes, because `printf` etc. use a preallocated 64MB heap.
efihdr = (struct efi_map_header *)(uintptr_t)addr; efihdr = (struct efi_map_header *)(uintptr_t)addr;
mm = (void *)((uint8_t *)efihdr + efisz); mm = (void *)((uint8_t *)efihdr + efisz);
sz = (EFI_PAGE_SIZE * pages) - efisz; sz = (EFI_PAGE_SIZE * pages) - efisz;
}
status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &mmsz, &mmver); if (EFI_ERROR(status) || retry == 0) {
tsoomeUnsubmitted
Done
Inline Actions

Since we did check this already, this if is not needed.

tsoome: Since we did check this already, this if is not needed.
bcranAuthorUnsubmitted
Done
Inline Actions

Good point - I'll fix it.

bcran: Good point - I'll fix it.
if (EFI_ERROR(status)) { printf("%s: GetMemoryMap error: %lu\n", __func__,
printf("%s: GetMemoryMap error %lu\n", __func__,
EFI_ERROR_CODE(status)); EFI_ERROR_CODE(status));
if (addr != 0)
BS->FreePages(addr, pages);
tsoomeUnsubmitted
Done
Inline Actions

This increment is too much, I think (the memory map can be quite large in systems with a lot of memory).

I would suggest to go with smaller steps, like page at a time, also at next line you can use ROUNDUP(sz, EFI_PAGE_SIZE).

tsoome: This increment is too much, I think (the memory map can be quite large in systems with a lot of…
bcranAuthorUnsubmitted
Done
Inline Actions

For systems with a large amount of memory, I'd expect still a relatively small memory map with one entry that covers most of the RAM. It'll be larger on systems like HPE that have lots of firmware utilities that will cause a lot of fragmentation.

In my latest patch I've changed it to add 10 memory descriptors to the size, which I think should be sufficient.

bcran: For systems with a large amount of memory, I'd expect still a relatively small memory map with…
bcranAuthorUnsubmitted
Done
Inline Actions

@tsoome Could you confirm whether the latest patch addresses your comment, please? I'm not entirely sure what increment you were referring to.

bcran: @tsoome Could you confirm whether the latest patch addresses your comment, please? I'm not…
kevansUnsubmitted
Done
Inline Actions

I think the comment was based on slight misreading; at the time of writing, it was being increased by mmsz (whose name would suggest that it might be the Memory Map SiZe, rather than the descriptor size that it is). Allocating for 10 more descriptors seems reasonable here, I think.

kevans: I think the comment was based on slight misreading; at the time of writing, it was being…
return (EINVAL); return (EINVAL);
kevansUnsubmitted
Done
Inline Actions

This can probably just go away, as discussed on IRC. The conversion to pages will take care of any necessary alignment, then we'll be clobbering sz here shortly if we don't error out anyways.

kevans: This can probably just go away, as discussed on IRC. The conversion to pages will take care of…
} }
status = BS->ExitBootServices(IH, efi_mapkey); status = BS->ExitBootServices(IH, efi_mapkey);
if (EFI_ERROR(status) == 0) { if (EFI_ERROR(status)) {
kevansUnsubmitted
Done
Inline Actions

mjg (the other mjg, not our mjg) noted that ExitBootServices can modify the memory map then subsequently fail, forcing you to replay the GetMemoryMap dance to actually exit -- but the subsequent retry will succeed, so I was clearly wrong about dropping the outer loop (and now that I re-read the comment, whoops...).

kevans: mjg (the other mjg, not our mjg) noted that ExitBootServices can modify the memory map then…
bcranAuthorUnsubmitted
Done
Inline Actions

Do you have a link to the note/discussion, or more information? I haven't read anything in the spec about ExitBootServices modifying the memory map. Is it a bug in some implementations?

bcran: Do you have a link to the note/discussion, or more information? I haven't read anything in the…
impUnsubmitted
Done
Inline Actions

ditto

imp: ditto
bcranAuthorUnsubmitted
Done
Inline Actions

We use a block of memory that was already allocated, so no problems calling printf here.

bcran: We use a block of memory that was already allocated, so no problems calling printf here.
printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status));
BS->FreePages(addr, pages);
return (EINVAL);
impUnsubmitted
Done
Inline Actions

I'd be tempted to do this BEFORE the printf call.

imp: I'd be tempted to do this BEFORE the printf call.
bcranAuthorUnsubmitted
Done
Inline Actions

Changed.

bcran: Changed.
}
/* /*
* This may be disabled by setting efi_disable_vmap in * This may be disabled by setting efi_disable_vmap in
* loader.conf(5). By default we will setup the virtual * loader.conf(5). By default we will setup the virtual
* map entries. * map entries.
*/ */
if (do_vmap) if (do_vmap)
efi_do_vmap(mm, sz, mmsz, mmver); efi_do_vmap(mm, sz, mmsz, mmver);
efihdr->memory_size = sz; efihdr->memory_size = sz;
efihdr->descriptor_size = mmsz; efihdr->descriptor_size = mmsz;
efihdr->descriptor_version = mmver; efihdr->descriptor_version = mmver;
file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz,
efihdr); efihdr);
return (0); return (0);
}
BS->FreePages(addr, pages);
}
printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status));
return (EINVAL);
} }
/* /*
* Load the information expected by an amd64 kernel. * Load the information expected by an amd64 kernel.
* *
* - The 'boothowto' argument is constructed. * - The 'boothowto' argument is constructed.
* - The 'bootdev' argument is constructed. * - The 'bootdev' argument is constructed.
* - The 'bootinfo' struct is constructed, and copied into the kernel space. * - The 'bootinfo' struct is constructed, and copied into the kernel space.
▲ Show 20 Lines • Show All 131 Lines • Show Last 20 Lines