Page 1 of 2
UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 2:06 pm
by NucleaTNT
Hello,
I am currently following
this tutorial to get access to the memory map via UEFI Boot Services. Currently, my code is able to retrieve the memory map in virtual environments like QEMU and on actual hardware (my desktop PC) but my laptop (has UEFI support) returns an INVALID_PARAMETER error instead of the expected BUFFER_TOO_SMALL and I have little to no clue as to why. From there it successfully manages to set my memory map in the allocated pool however I find that the memory map is full of garbage values that then ruin the rest of the code in my
PageFrameAllocator.
(The addresses depicted in the pictures are the results of ten RequestPage() calls to my PageFrameAllocator)
Here is what it looks like when it works (QEMU):
And here is the non-working version (Laptop):
(Currently those addresses can also be 0x000000...)
Despite looking around for months I've found very few resources to do with the topic, and any of which I do find seem to have no known instances of this problem.
Helpful links:
Github
Files associated with memory (excluding bootloader)
Bootloader
Kernel
Yes, I am aware that the organization situation is quite dire, I had hoped to fix it once this issue is resolved :D
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 6:37 pm
by zaval
In your loader, line 198, you declare MemMapSize variable and then pass it without setting it up, to 0, judging by the logic of your code. That is, GetMemoryMap() gets the situation, when
The MemoryMap buffer is not too small and MemoryMap is NULL.
Better do allocate some real buffer first and pass its size to GetMemoryMap() and only then free it and reallocate a bigger one, if that's not enough. But anyway, uninitialized variable is the cause of the bug.
You omit all the status checks...
Edit 2, I was wrong about being wrong, the code is correct, see below post. The code returned:
Code: Select all
/* allocating the buffer for the memory map */
#define STUPID_EXTRA_BEHIND_DESCRIPTOR 8 /* FW is known to put extra garbage behind the descr. */
DescriptorSize = (sizeof(EFI_MEMORY_DESCRIPTOR) + STUPID_EXTRA_BEHIND_DESCRIPTOR);
MemoryMapSize = DescriptorSize << 7; /* let it be 128 entries first */
AddendumShift = 3;
A: Status = pbs->AllocatePool(EfiLoaderData, MemoryMapSize, &MemoryMap);
if(Status != EFI_SUCCESS){
pout->OutputString(pout, L"ERROR:AllocatePool()\r\n");
MemoryMap = NULL;
goto YYY;
}
/* getting the memory map */
Status = pbs->GetMemoryMap(&MemoryMapSize, MemoryMap, &MapKey, &DescriptorSize, &DescriptorVersion);
if(Status == EFI_SUCCESS)goto B;
else if(Status == EFI_BUFFER_TOO_SMALL){
if(AddendumShift < 10){
AddendumShift++;
}else{
pout->OutputString(pout, L"ERROR:GetMemoryMap() too many attempts to guess the buffer size\r\n");
goto YYY;
}
pbs->FreePool(MemoryMap); /* freeing the small buffer */
if(DescriptorSize){
MemoryMapSize += DescriptorSize << AddendumShift;
}else{
MemoryMapSize += (sizeof(EFI_MEMORY_DESCRIPTOR) + STUPID_EXTRA_BEHIND_DESCRIPTOR) << AddendumShift;
}
goto A; /* retrying */
}else{
pout->OutputString(pout, L"ERROR:GetMemoryMap() failed\r\n");
goto YYY;
}
B: /* doing further work */
/* we return to the FW, but the loader doesn't have to */
YYY: if(MemoryMap)pbs->FreePool(MemoryMap);
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 7:16 pm
by kzinti
You are not supposed to guess the size of descriptors like you are doing with STUPID_EXTRA_BEHIND_DESCRIPTOR. You are supposed to first call GetMemoryMap() with 0 size and null buffer to obtain the descriptor size and then use that to allocate memory.
There is also no need for this exponential buffer growth thing you are doing with a shift... GetMemoryMap() is returning the size it wants in the first parameter. There is no need to guess here.
And yes, you need to initialize your variables otherwise you will get EFI_INVALID_PARAMETER.
You can also get EFI_INVALID_PARAMETER when calling ExitBootServices(). This can happen if ExitBootServices() itself modifies the memory map, in which case the key your obtained isn't valid anymore.
Here is how I do it:
Code: Select all
void EfiBoot::Exit(MemoryMap& memoryMap)
{
UINTN size = 0;
EFI_MEMORY_DESCRIPTOR* descriptors = nullptr;
UINTN memoryMapKey = 0;
UINTN descriptorSize = 0;
UINT32 descriptorVersion = 0;
// 1) Retrieve the memory map from the firmware
EFI_STATUS status;
std::vector<char> buffer;
while ((status = g_efiBootServices->GetMemoryMap(&size, descriptors, &memoryMapKey, &descriptorSize, &descriptorVersion)) == EFI_BUFFER_TOO_SMALL)
{
// Add some extra space. There are few reasons for this:
// a) Allocating memory for the buffer can increase the size of the memory map itself.
// Adding extra space will prevent an infinite loop.
// b) We want to try to prevent a "partial shutdown" when calling ExitBootServices().
// See comment below about what a "partial shutdown" is.
// c) If a "partial shutdown" does happen, we won't be able to allocate more memory!
// Having some extra space now should mitigate the issue.
size += descriptorSize * 10;
buffer.resize(size);
descriptors = (EFI_MEMORY_DESCRIPTOR*)buffer.data();
}
if (EFI_ERROR(status))
{
Fatal("Failed to retrieve the EFI memory map: %p\n", status);
}
// 2) Exit boot services - it is possible for the firmware to modify the memory map
// during a call to ExitBootServices(). A so-called "partial shutdown".
// When that happens, ExitBootServices() will return EFI_INVALID_PARAMETER.
while ((status = g_efiBootServices->ExitBootServices(g_efiImage, memoryMapKey)) == EFI_INVALID_PARAMETER)
{
// Memory map changed during ExitBootServices(), the only APIs we are allowed to
// call at this point are GetMemoryMap() and ExitBootServices().
size = buffer.size(); // Probably not needed, but let's play safe since EFI could change that value behind our back (you never know!)
status = g_efiBootServices->GetMemoryMap(&size, descriptors, &memoryMapKey, &descriptorSize, &descriptorVersion);
if (EFI_ERROR(status))
{
Fatal("Failed to retrieve the EFI memory map: %p\n", status);
}
}
if (EFI_ERROR(status))
{
Fatal("Failed to exit boot services: %p\n", status);
}
BuildMemoryMap(memoryMap, descriptors, size / descriptorSize, descriptorSize);
}
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 7:29 pm
by zaval
It's not buggy, that calculation is made just to avoid 2 GMM calls, and it often succeeds, that's all. Of course, the code uses returned memory descriptor size. But the example was just as a helper to understand the logic of getting it right, because just 3 sequeantial calls: GMM, AP, GMM is not right.
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 7:43 pm
by kzinti
zaval wrote:It's not buggy, that calculation is made just to avoid 2 GMM calls, and it often succeeds, that's all.
Why would you want to avoid making 2 calls? This is how the API is supposed to be used.
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 7:56 pm
by zaval
kzinti wrote:zaval wrote:It's not buggy, that calculation is made just to avoid 2 GMM calls, and it often succeeds, that's all.
Why would you want to avoid making 2 calls? This is how the API is supposed to be used.
What's wrong with providing to the first GMM call a valid buffer? GMM happily will fill it at the first call if the buffer is not small. size matters.
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Tue Mar 30, 2021 10:30 pm
by kzinti
zaval wrote:What's wrong with providing to the first GMM call a valid buffer? GMM happily will fill it at the first call if the buffer is not small. size matters.
Nothing wrong I suppose... But that means your code is non-deterministic in that sometimes it will make one call (if you guessed a big buffer enough) or two calls if you didn't. In the case where you make two calls, you will be have to resize your buffer and that probably means freeing the buffer and allocating a new one.
With the way I do it, it's deterministic: I always make exactly two calls (and only one allocation).
So if your goal was to "save calls", you are not always succeeding. In fact you can't tell how often your guess was right vs wrong.
None of this matters, just teasing you...
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 12:31 pm
by NucleaTNT
kzinti wrote:You are not supposed to guess the size of descriptors like you are doing with STUPID_EXTRA_BEHIND_DESCRIPTOR. You are supposed to first call GetMemoryMap() with 0 size and null buffer to obtain the descriptor size and then use that to allocate memory.
There is also no need for this exponential buffer growth thing you are doing with a shift... GetMemoryMap() is returning the size it wants in the first parameter. There is no need to guess here.
And yes, you need to initialize your variables otherwise you will get EFI_INVALID_PARAMETER.
You can also get EFI_INVALID_PARAMETER when calling ExitBootServices(). This can happen if ExitBootServices() itself modifies the memory map, in which case the key your obtained isn't valid anymore.
Here is how I do it:
Code: Select all
void EfiBoot::Exit(MemoryMap& memoryMap)
{
UINTN size = 0;
EFI_MEMORY_DESCRIPTOR* descriptors = nullptr;
UINTN memoryMapKey = 0;
UINTN descriptorSize = 0;
UINT32 descriptorVersion = 0;
// 1) Retrieve the memory map from the firmware
EFI_STATUS status;
std::vector<char> buffer;
while ((status = g_efiBootServices->GetMemoryMap(&size, descriptors, &memoryMapKey, &descriptorSize, &descriptorVersion)) == EFI_BUFFER_TOO_SMALL)
{
// Extra space to try to prevent "partial shutdown" when calling ExitBootServices().
// See comment below about what a "partial shutdown" is.
size += descriptorSize * 10;
buffer.resize(size);
descriptors = (EFI_MEMORY_DESCRIPTOR*)buffer.data();
}
if (EFI_ERROR(status))
{
Fatal("Unable to retrieve the EFI memory map: %p\n", status);
}
// 2) Exit boot services - it is possible for the firmware to modify the memory map
// during a call to ExitBootServices(). A so-called "partial shutdown".
// When that happens, ExitBootServices() will return EFI_INVALID_PARAMETER.
while ((status = g_efiBootServices->ExitBootServices(g_efiImage, memoryMapKey)) == EFI_INVALID_PARAMETER)
{
// Memory map changed during ExitBootServices(), the only APIs we are allowed to
// call at this point are GetMemoryMap() and ExitBootServices().
size = buffer.size(); // Probably not needed, but let's play safe since EFI could change that value behind our back (you never know!)
status = g_efiBootServices->GetMemoryMap(&size, descriptors, &memoryMapKey, &descriptorSize, &descriptorVersion);
if (EFI_ERROR(status))
{
break;
}
}
if (EFI_ERROR(status))
{
Fatal("Unable to exit boot services: %p\n", status);
}
memoryMap.Build(descriptors, size / descriptorSize, descriptorSize);
}
Cheers, I've applied this in my code (see below) and it passes the memory map through as it did before, however the values are still garbled.
Code: Select all
Print(L"Getting Memory Map and Exiting Boot Services... ");
EFI_MEMORY_DESCRIPTOR* memMap = NULL;
UINTN memMapSize = 0, memMapKey = 0, descriptorSize = 0;
UINT32 descriptorVer = 0;
{
while ((status = SystemTable->BootServices->GetMemoryMap(&memMapSize, memMap, &memMapKey, &descriptorSize, &descriptorVer)) == EFI_BUFFER_TOO_SMALL) {
memMapSize += descriptorSize * 10;
if (memMap != NULL) SystemTable->BootServices->FreePool(memMap);
SystemTable->BootServices->AllocatePool(EfiLoaderData, memMapSize, (void**)&memMap);
}
if (EFI_ERROR(status)) {
Print(L"FATAL - Unable to retrieve EFI memory map [%d]\n\r", status);
while (1);
}
while ((status = SystemTable->BootServices->ExitBootServices(ImageHandle, memMapKey)) == EFI_INVALID_PARAMETER) {
status = SystemTable->BootServices->GetMemoryMap(&memMapSize, memMap, &memMapKey, &descriptorSize, &descriptorVer);
if (EFI_ERROR(status)) break;
}
if (EFI_ERROR(status)) {
Print(L"FATAL - Unable to exit boot services [%d]\n\r", status);
while (1);
}
}
It still yields this:
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 12:58 pm
by kzinti
Can you show your definition for EFI_MEMORY_DESCRIPTOR?
I recall there is a bug in some UEFI headers provided by Intel when using them with GCC. Basically the header assumes MS ABI packing rules and that fails on GCC.
I had to modify the header to fix this by adding the "Padding" field (see below). I also added a static_assert<> to ensure it doesn't happen again.
Code: Select all
typedef struct {
UINT32 Type;
UINT32 Padding; // Padding to ensure the next field is 64-bits aligned as per MS packing rules
EFI_PHYSICAL_ADDRESS PhysicalStart;
EFI_VIRTUAL_ADDRESS VirtualStart;
UINT64 NumberOfPages;
} EFI_MEMORY_DESCRIPTOR;
// Intel's UEFI header does not properly define EFI_MEMORY_DESCRIPTOR for GCC.
// This check ensures that it is.
static_assert(offsetof(EFI_MEMORY_DESCRIPTOR, PhysicalStart) == 8);
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:17 pm
by NucleaTNT
kzinti wrote:Can you show your definition for EFI_MEMORY_DESCRIPTOR?
I recall there is a bug in some UEFI headers provided by Intel when using them with GCC (x86_64). Basically the header assumes MS ABI packing rules and that fails on GCC.
I had to modify the header to fix this by adding the "Padding" field (see below). I also added a static_assert<> to ensure it doesn't happen again.
Code: Select all
typedef struct {
UINT32 Type;
UINT32 Padding; // Padding to ensure the next field is 64-bits aligned as per MS packing rules
EFI_PHYSICAL_ADDRESS PhysicalStart;
EFI_VIRTUAL_ADDRESS VirtualStart;
UINT64 NumberOfPages;
} EFI_MEMORY_DESCRIPTOR;
// Intel's UEFI header does not properly define EFI_MEMORY_DESCRIPTOR for GCC.
// This check ensures that it is.
static_assert(offsetof(EFI_MEMORY_DESCRIPTOR, PhysicalStart) == 8);
Here's the one defined in my kernel:
Code: Select all
struct EFI_MEMORY_DESCRIPTOR {
uint32_t type;
void* physAddress, *virtAddress;
uint64_t pageCount, attributes;
};
I'll try adding __attribute__((packed)) and that padding you mentioned. I've just checked the definition in efidef.h (see below) and it includes that padding - hopefully this sorts it!
Code: Select all
// efidef.h
typedef struct {
UINT32 Type; // Field size is 32 bits followed by 32 bit pad
UINT32 Pad;
EFI_PHYSICAL_ADDRESS PhysicalStart; // Field size is 64 bits
EFI_VIRTUAL_ADDRESS VirtualStart; // Field size is 64 bits
UINT64 NumberOfPages; // Field size is 64 bits
UINT64 Attribute; // Field size is 64 bits
} EFI_MEMORY_DESCRIPTOR;
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:25 pm
by NucleaTNT
Unfortunately, changing the header does not seem to have fixed the issue.
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:38 pm
by kzinti
There might be a bug in the way you calculate available/used memory.
Can you dump the descriptors themselves after ExitBootServices() so that we can verify if they are looking good or not?
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:45 pm
by zaval
kzinti wrote:
There is also no need for this exponential buffer growth thing you are doing with a shift... GetMemoryMap() is returning the size it wants in the first parameter. There is no need to guess here.
I remembered why my code was doing that, unfortunately, - only when turned the computer off.
So back only now. The code is doing it right and you might want to take this into account in your code. Know why? Here is why:
If the MemoryMap buffer is too small, the EFI_BUFFER_TOO_SMALL error code is returned and the MemoryMapSize value contains the size of the buffer needed to contain the current memory map. The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize, since allocation of the new buffer may potentially increase memory map size.
After buffer too small, GMM returns the current memory map, but you allocate new buffer and that changes the map, so you need to take that into account, adding some extra. That extra, my code adds is exactly it - it takes the reported map size and adds a bit more space for possible new entries. And it retries to add more of them if the guess wasn't right. That's btw yet one argument for guessing the buffer for the first GMM call - you would need to guess it anyway.
To the OP - did you fix your uninitialized local varible (memMapSize) bug, in the line 198 of your loader?
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:52 pm
by kzinti
zaval wrote:
If the MemoryMap buffer is too small, the EFI_BUFFER_TOO_SMALL error code is returned and the MemoryMapSize value contains the size of the buffer needed to contain the current memory map. The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize, since allocation of the new buffer may potentially increase memory map size.
Yes this is why I am adding "descriptorSize * 10"... I am adding extra space for new entries. My point is that you don't need exponential growth here. Adding space for a few entries (10 in my case) ought to be enough. Each EFI allocation typically adds one entry to the memory map.
So my algo does this:
1) Ask the current memory map size (with no buffer)
2) Add space for 10 extra descriptors to the memory map size returned by #1
3) Allocate a buffer (which might indeed grow the memory map size)
4) Get the memory map. If that fails for some reason, goto #2
Re: UEFI GetMemoryMap returns INVALID_PARAMETER
Posted: Wed Mar 31, 2021 1:56 pm
by NucleaTNT
kzinti wrote:There might be a bug in the way you calculate available/used memory.
Can you dump the descriptors themselves after ExitBootServices() so that we can verify if they are looking good or not?
Here you go :D I'm probably just missing something super obvious
[Edit: here's how I get the total memory, I think the static variable may be getting overwritten as I've just checked and despite being initialized as 0 it has garbage values in it]
Code: Select all
uint64_t GetMemorySize(EFI_MEMORY_DESCRIPTOR* memMap, uint64_t memMapEntries, uint64_t memMapDescSize) {
static uint64_t memSizeBytes = 0;
if (memSizeBytes > 0) return memSizeBytes;
for (int i = 0; i < memMapEntries; i++) {
EFI_MEMORY_DESCRIPTOR* desc = (EFI_MEMORY_DESCRIPTOR*)((uint64_t)memMap + (i * memMapDescSize));
memSizeBytes += desc->pageCount * 4096;
}
return memSizeBytes;
}