UEFI GetMemoryMap returns INVALID_PARAMETER

[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

[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);

[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);
}

[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.

[kzinti]

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.

[zaval]

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.

[kzinti]

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...

[NucleaTNT]

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:

[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);

[NucleaTNT]

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;

[NucleaTNT]

Unfortunately, changing the header does not seem to have fixed the issue.

[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?

[zaval]

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?

[kzinti]

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

[NucleaTNT]

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;
}