Having trouble with EFI's ExitBootServices

[azzi777]

Hello, all!
This is my first post on the forum, but I've been tinkering with OS development on and off for some years now --- often making use of the many great resources on this site. So thank you all for that

So, on to my problem, and the reason I finally post on this forum. I'm currently trying to make a simple 64-bit kernel, and using EFI to boot it. I got booting and "hello world"-ing to work, but I am stuck with a bug that I just can't explain. It is related to getting the memory map using GetMemoryMap, and later exiting the boot services. Before I explain any further, let me post my code.

Code: Select all

typedef struct {
    EFI_MEMORY_DESCRIPTOR* map_ptr;
    UINTN map_size;
    UINTN map_key;
    UINTN desc_size;
    UINT32 desc_ver;
} memory_map_t;


void memory_init(EFI_HANDLE img_handle, EFI_SYSTEM_TABLE* sys_table) {
    EFI_STATUS status;
    volatile memory_map_t memory_map = {0};

    EFI_BOOT_SERVICES* boot_srv = sys_table->BootServices;
    EFI_RUNTIME_SERVICES* run_srv = sys_table->RuntimeServices;

    status = boot_srv->GetMemoryMap(
        (UINTN*)&memory_map.map_size,
        NULL, NULL, NULL, NULL
    );
    if(status != EFI_BUFFER_TOO_SMALL) {
        Print(L"Error (%d) obtaining memory map.\r\n", status);
        return;
    }

    while(1) {
        Print(L"Trying to allocate size 0x%X\r\n", memory_map.map_size);
        status = boot_srv->AllocatePool(
            EfiLoaderData,
            memory_map.map_size,
            (void**)&memory_map.map_ptr
        );
        if(status != EFI_SUCCESS) {
            Print(L"Error (%d) allocating memory.\r\n", status);
            return;
        }

        status = boot_srv->GetMemoryMap(
            (UINTN*)&memory_map.map_size,
            (EFI_MEMORY_DESCRIPTOR*)&memory_map.map_ptr,
            (UINTN*)&memory_map.map_key,
            (UINTN*)&memory_map.desc_size,
            (UINT32*)&memory_map.desc_ver
        );
        Print(L"Map key: 0x%X: %d\r\n", &memory_map.map_key, memory_map.map_key);

        if(status == EFI_BUFFER_TOO_SMALL) {
            status = boot_srv->FreePool((void*)memory_map.map_ptr);
            if(status != EFI_SUCCESS) {
                Print(L"Error (%d) freeing memory: 0x%X\r\n", status, memory_map.map_ptr);
                return;
            }
            Print(L"Too small! Freed.\r\n");
        } else if(status != EFI_SUCCESS) {
            Print(L"Error (%d) obtaining memory map.\r\n", status);
            return;
        } else {
            Print(L"Big enough --- Location 0x%X\r\n", memory_map.map_ptr);
            break;
        }
    }

    Print(L"Obtained memory map:\r\n");

    Print(L"    map_size  0x%X\r\n", memory_map.map_size);
    Print(L"    map_ptr   0x%X\r\n", memory_map.map_ptr);
    Print(L"    map_key   0x%X\r\n", memory_map.map_key);
    Print(L"    desc_size 0x%X\r\n", memory_map.desc_size);
    Print(L"    desc_ver  0x%X\r\n\n", memory_map.desc_ver);

    status = boot_srv->ExitBootServices(img_handle, memory_map.map_key);
    if(status != EFI_SUCCESS) {
        Print(L"Error (%d) exiting boot services\r\n", status);
        return;
    } else {
        Print(L"Success\r\n");
    }

    run_srv->SetVirtualAddressMap(
        memory_map.map_size,
        memory_map.desc_size,
        memory_map.desc_ver,
        memory_map.map_ptr
    );

    Print(L"Exited boot services\r\n");
}

The problem with the code as written above is that the call to ExitBootServices returns EFI_INVALID_PARAMETER. From the documentation, it seems like it most likely complains about the map key.

As can be seen, I have created a structure to hold the memory map data. I create an instance of it on the stack at the beginning of the function, before I use GetMemoryMap to obtain the required size of the memory map. I allocate the size and check if it is big enough, if it isn't, I try again until it can fit the memory map (each iteration freeing the unused memory).
However, it seems that the order of the variable definitions ends up having an effect on the program (even though it in theory shouldn't). If I move the declaration of the memory_map variable down two lines (to just below the definition of boot_srv and run_srv), my program crashes when calling ExitBootServices, in other words, it prints neither a success nor an error message. My first thought was that the compiler might be doing something to "optimize" my code, but the optimization should be off (default is -O0), and I have marked my memory_map variably as volatile.

I am really intrigued by this behaviour, but I can't for the life of me work out why the code behaves that way. Anyone have any ideas or experience with similar behaviour?

All that my efi_main function does is set the graphics mode and call memory_init. I have added both my efi_main and makefile below for completeness.

Code: Select all

EFI_STATUS efi_main(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE* SystemTable) {
    EFI_STATUS status;
    InitializeLib(ImageHandle, SystemTable);

    SIMPLE_TEXT_OUTPUT_INTERFACE* TextOut = SystemTable->ConOut;
    TextOut->Reset(TextOut, 0);

    UINTN Colums;
    UINTN Rows;
    int i = 100;

    do {
        i--;
        status = TextOut->QueryMode(TextOut, i, &Colums, &Rows);
    } while(status == EFI_UNSUPPORTED);

    status = TextOut->SetMode(TextOut, i);

    TextOut->SetAttribute(TextOut, EFI_WHITE);
    TextOut->OutputString(TextOut, L"Welcome to ");
    TextOut->SetAttribute(TextOut, EFI_LIGHTCYAN);
    TextOut->OutputString(TextOut, L"MatteOS");
    TextOut->SetAttribute(TextOut, EFI_WHITE);
    TextOut->OutputString(TextOut, L" UEFI!\r\n\n");

    Print(L"TTY size: %dx%d\r\n\n", Colums, Rows);

    memory_init(ImageHandle, SystemTable);

    while(1);

    return EFI_SUCCESS;
}

Code: Select all

SRC_DIR = src
OBJ_DIR = obj
OUT_DIR = out
STATIC = $(OBJ_DIR)/static
DYNAMIC = $(OBJ_DIR)/dynamic

INC = $(shell find . -name *.h -printf '%P\n')
SRC = $(shell find . -name *.c -printf '%P\n')
OBJ = $(patsubst $(SRC_DIR)/%.c, $(STATIC)/%.o, $(SRC))

OUT_KERN = $(OUT_DIR)/kern.efi
OUT_IMG = $(OUT_DIR)/disk.img

EFI_INC = -I/usr/include/efi/ -I/usr/include/efi/x86_64/ \
		  -I/usr/include/efi/protocol/
CFLAGS = -fshort-wchar -Wall -fpic -nostdlib -fno-builtin -nodefaultlibs \
		 -ffreestanding -fno-exceptions -fno-stack-protector \
		 -mno-red-zone -DHAVE_USE_MS_ABI -DEFI_FUNCTION_WRAPPER
EFI_LIB = -L /usr/lib/gnuefi -L /usr/lib /usr/lib/crt0-efi-x86_64.o
LDFLAGS = -nostdlib -znocombreloc -T /usr/lib/elf_x86_64_efi.lds -shared \
		  -Bsymbolic $(EFI_LIB)
LIBS = -lefi -lgnuefi
OBJFLAGS = -j .text -j .sdata -j .data -j .dynamic -j .dynsym -j .rel -j .rela \
		   -j .reloc --target=efi-app-x86_64
QEMUFLAGS = -cpu qemu64 -bios OVMF.fd -enable-kvm -m 64 -device VGA -net none \
			-display gtk -ctrl-grab

all: dirs $(OUT_KERN)

dirs:
	mkdir -p $(OBJ_DIR) $(OUT_DIR)

clean:
	rm -rf $(OBJ_DIR) $(OUT_DIR)

$(OUT_KERN): $(DYNAMIC)/kern.so
	objcopy $(OBJFLAGS) $< $@

$(DYNAMIC)/kern.so: $(OBJ)
	mkdir -p $(shell dirname $@)
	ld $(LDFLAGS) $^ -o $@ $(LIBS)

$(OBJ): $(STATIC)/%.o: $(SRC_DIR)/%.c $(INC)
	mkdir -p $(shell dirname $@)
	clang $(EFI_INC) $(CFLAGS) -c -o $@ $<

run: all $(OUT_IMG)
	qemu-system-x86_64 -drive file=$(OUT_IMG),if=ide $(QEMUFLAGS)

$(OUT_IMG): $(OUT_KERN)
	dd if=/dev/zero of=$@ bs=4k count=16k
	mkfs.fat $@
	mmd -i $@ ::/EFI
	mmd -i $@ ::/EFI/BOOT
	mcopy -i $@ $^ ::/EFI/BOOT/BOOTX64.efi

[MichaelFarthing]

What is the line:

while(1);

supposed to do?

[..and I suspect it might take quite a while to do it].

[EDIT This is in your main function and looking again perhaps you never intend to reach this line]

[BenLunt]

The problem with the code as written above is that the call to ExitBootServices returns EFI_INVALID_PARAMETER.

Without looking over your code, this really tells me that you have tried to allocate or free memory between the time you get the memory map and use ExitBootServices.

The memory map must be unchanged from the last time you retrieve the memory map and the time you call the ExitBootServices call. This means, no allocate and/or freeing of memory between the two.

Ben

[zaval]

Maybe it hates how much you screwed up the UEFI coding style.

Now more seriously. This:

Code: Select all

    status = boot_srv->GetMemoryMap(
        (UINTN*)&memory_map.map_size,
        NULL, NULL, NULL, NULL
    );

Are you kidding? Right?
Where did you read that the address of the buffer where the memory map should have been placed, could be NULL?
The same goes to the remaining NULLs in your call - they are all invalid, but the first one is just. I don't know. You should reread GetMemoryMap() description. You didn't get at all the meaning of all those parameters and the function overall.

You need to supply correct parameters at the first call too. And if the beffer you have allocated for this call is small, the function returns "too small" error. Only then, you should allocate new buffers freeing previous. Everytime supplying correct parameters, not NULLs for output buffers.
I suspect there are a lot of other mistakes in your code. I just don't have time to check it all. Read the function description 100 times more before starting to run your code. It sucks at the too damn high level.

[azzi777]

Thanks for your input guys.

[EDIT This is in your main function and looking again perhaps you never intend to reach this line]

Correct, it is only there so that the system does not reboot immediately.

The problem with the code as written above is that the call to ExitBootServices returns EFI_INVALID_PARAMETER.

Without looking over your code, this really tells me that you have tried to allocate or free memory between the time you get the memory map and use ExitBootServices.

The memory map must be unchanged from the last time you retrieve the memory map and the time you call the ExitBootServices call. This means, no allocate and/or freeing of memory between the two.

Ben

Yeah I thought so at first as well, but there should not be any call to free/allocate after the correct memory map is obtained. That being said, the EFI_INVALID_PARAMETER status value is not the main issue either, but rather the fact that reordering some lines of code that should not impact the running of the program does indeed affect the outcome.

Maybe it hates how much you screwed up the UEFI coding style.

Now more seriously. This:

Code: Select all

    status = boot_srv->GetMemoryMap(
        (UINTN*)&memory_map.map_size,
        NULL, NULL, NULL, NULL
    );

Are you kidding? Right?
Where did you read that the address of the buffer where the memory map should have been placed, could be NULL?
The same goes to the remaining NULLs in your call - they are all invalid, but the first one is just. I don't know. You should reread GetMemoryMap() description. You didn't get at all the meaning of all those parameters and the function overall.

You need to supply correct parameters at the first call too. And if the beffer you have allocated for this call is small, the function returns "too small" error. Only then, you should allocate new buffers freeing previous. Everytime supplying correct parameters, not NULLs for output buffers.
I suspect there are a lot of other mistakes in your code. I just don't have time to check it all. Read the function description 100 times more before starting to run your code. It sucks at the too damn high level.

Yeah, I agree it might not be the correct way to do things, but it worked previously, so I did not think any more of it. As long as the initial size supplied to the function is 0, it returns EFI_BUFFER_TOO_SMALL, regardless if the other parameters supplied. As such, there seemed to be no difference between passing a real variable or NULL as the other parameters. I am only using it to obtain the initial memory map size anyway. Changing the line to use actual variables instead if NULL has no effect, the problem is still there.

I feel that I might be compiling the code wrongly, in that some code might be omitted or something similar, but I can't spot anything unusual with objdump/strings. All the strings, at least, end up in the final compiled efi binary file.

[zaval]

In fact, it should not even work if the address of the buffer is NULL, or the Key variable address is NULL, because there is no buffer at all nor the place for storing the Key. My implementation would return EFI_INVALID_PARAMETER in that case. It just happens that this fw you work with keeps looking at the size even when the addresses are NULL. The point is the specification doesn't allow to get the size of the map this way, with NULLs.
Next, you try to allocate a buffer with the same exact size as GetMemoryMap() returned, whereas you should allocate more - the specification says this clearly. And as Ben outlined - all allocations should happen before the last successful GetMemoryMap() call, and yet - because you do allocations, the map is changing since you called it last time. Because you allocate yet memory for the bigger buffer. You should add a little more to the buffer size after "too small" return from GetMemoryMap(). And once you have successful return from GetMemoryMap(), you cannot allocate memory, otherwise your Key gets invalid.

[JAAman]

In fact, it should not even work if the address of the buffer is NULL, or the Key variable address is NULL, because there is no buffer at all nor the place for storing the Key. My implementation would return EFI_INVALID_PARAMETER in that case. It just happens that this fw you work with keeps looking at the size even when the addresses are NULL. The point is the specification doesn't allow to get the size of the map this way, with NULLs.

look at the bottom of page 160 (my copy is version 2.6, dated January 2016... might be different page on other versions) -- it very clearly states that the function must never return EFI_INVALID_PARAMETER if the buffer is nullptr and the size is too low

it clearly and specifically states that this is allowed (using buffer == nullptr size == 0 for the initial call) and that this will never result in EFI_INVALID_PARAMETER -- if your implementation does this, it is in violation of the UEFI specification

[azzi777]

Next, you try to allocate a buffer with the same exact size as GetMemoryMap() returned, whereas you should allocate more - the specification says this clearly. And as Ben outlined - all allocations should happen before the last successful GetMemoryMap() call, and yet - because you do allocations, the map is changing since you called it last time. Because you allocate yet memory for the bigger buffer. You should add a little more to the buffer size after "too small" return from GetMemoryMap(). And once you have successful return from GetMemoryMap(), you cannot allocate memory, otherwise your Key gets invalid.

Read the code again. I do allocate the exact size the first time, but the allocation routine is in a while loop. If it is too small, I free the memory and try again with the new required size. I repeat this until it is big enough. At the point where the status value from GetMemoryMap is EFI_SUCCESS, the loop is exited, and ExitBootServices is called. Nothing happens in between the last call to GetMemoryMap and ExitBootServices.

As for the rest, see JAAman's response.

[zaval]

In fact, it should not even work if the address of the buffer is NULL, or the Key variable address is NULL, because there is no buffer at all nor the place for storing the Key. My implementation would return EFI_INVALID_PARAMETER in that case. It just happens that this fw you work with keeps looking at the size even when the addresses are NULL. The point is the specification doesn't allow to get the size of the map this way, with NULLs.

look at the bottom of page 160 (my copy is version 2.6, dated January 2016... might be different page on other versions) -- it very clearly states that the function must never return EFI_INVALID_PARAMETER if the buffer is nullptr and the size is too low

it clearly and specifically states that this is allowed (using buffer == nullptr size == 0 for the initial call) and that this will never result in EFI_INVALID_PARAMETER -- if your implementation does this, it is in violation of the UEFI specification

Are you sure?

[JAAman]

thank you for posting an image that proves exactly what I said (that there is exactly the portion I had in mind when I made my post)

your post there shows under "EFI_INVALID_PARAMETER":

The MemoryMap buffer is not too small and MemoryMap is NULL

now let us examine this statement, emphasising the important parts of the sentence:

The MemoryMap buffer is not too small and MemoryMap is NULL

in other words, if MemoryMap is NULL but MemoryMap buffer is too small, this doesn't apply


therefore:
there is no error if any parameter other than MemoryMap and MemoryMapSize is NULL
if MemoryMapSize is NULL: EFI_INVALID_PARAMETER
if MemoryMapSize is too small: EFI_BUFFER_TOO_SMALL (and return correct size)
if MemoryMap is NULL:
--if MemoryMap size is too small to hold the map: EFI_BUFFER_TOO_SMALL (and return correct size)
--if MemoryMap size is large enough: EFI_INVALID_PARAMETER

the only condition which results in EFI_INVALID_PARAMETER is when both
MemoryMapSize indicates the buffer is large enough to hold the map
AND
MemoryMap is NULL

if either one of those are false (either the buffer is too small or MemoryMap is not NULL) then you will not get EFI_INVALID_PARAMETER

[zaval]

Read the code again. I do allocate the exact size the first time, but the allocation routine is in a while loop. If it is too small, I free the memory and try again with the new required size.

It's at least not the best way of doing. But since FW includes blocks in the map not only AllocatePool() and AllocatePages() allocated but "as well as blocks that the firmware is using for its own purposes." with your approach of allocating that previous minimum reported, you risk to face "too small again". Why just not rearrange the loop properly. As of the first call I already said - it's wrong.

I repeat this until it is big enough. At the point where the status value from GetMemoryMap is EFI_SUCCESS, the loop is exited, and ExitBootServices is called. Nothing happens in between the last call to GetMemoryMap and ExitBootServices.

As for the rest, see JAAman's response.

What's Print()? Does it allocate memory?
MapKey should be invalid to have been rejected by ExitBootServices(). There is nothing else that might cause it to return invalid parameter status.

[zaval]

JAAman, but if it weren't NULL and size weren't small, at the first call, function might just succeed, right? So why do this unnecessary failing? receive buffer address as NULL is invalid paramater the function signals of. that was I mean. But yes, the specification doesn't prohibit taking this way the size, you are right. It's just not exactly the best way of doing it. imho. I'll modify my implementation to allow this. I am not at the working Boot Services stage anyway.