Virtual memory manager in C

Question

Virtual Memory manager that has functions, mappage, unmappage, remappage

If the physical address that was passed in to mappage is 0, the physical pages will be randomly allocated. I decided to do it like this since i'm using the limine bootloader and it automatically restricts 0 - 0x1000 memory addresses.

Here is a picture of the 4 level paging system

#define V2P(V) (uintptr_t)V - HHDM_OFFSET
#define P2V(P) (uintptr_t)P + HHDM_OFFSET

typedef uint64_t pde_t;
typedef uint64_t pte_t;
typedef uint64_t pml4_t;
typedef uint64_t pdpte_t;

uint64_t *bitmap;

#define PRESENT     0x1

typedef struct {
    uint64_t *table;
    uint8_t shift; // how many times to shift to get the correct bits
} Table;


// get the PTE that represents the virtual address and allocate a page for it.
// it will allocate new directories and tables if needed

uint64_t mappage(uint64_t vaddr, uint64_t paddr, uint8_t flags) {
    uint64_t *PDPTE, *PDE, *PTE;
    Table tablearr[4] = {{PML4E, 39}, {PDPTE, 30}, {PDE, 21}, {PTE, 12}};
    uint16_t index;
    uintptr_t addr;
    for (uint8_t i = 0; i < 4; ++i) {
        index = (vaddr >> (tablearr[i].shift)) & 0x1FF;
        if ((tablearr[i].table[index] & 1) == 0) {

            // if at the end of array allocate a page
            if (i == 3)
                newentry(&tablearr[i].table[index], paddr, flags);
            else
                tablearr[i + 1].table = newentry(&tablearr[i].table[index], 0, flags);
        } else {
            if (i == 3) {
                panic("panic: mappage, page already in use\n");
                break;
            }
            addr = (tablearr[i].table[index] >> 12);
            tablearr[i + 1].table = P2V(addr);
        }
    }
    return vaddr;
}

// get PTE and set present flag to 0 and free page from physical memory
void unmappage(uint64_t vaddr) {
    pte_t *pte = getpte(vaddr);
    uintptr_t addr = ((uintptr_t)*pte >> 12);
    *pte &= (0 << PRESENT);
    freepg((void*)P2V(addr), 1);
}

// 1. if PFN is already in use, error 
// 2. get current PTE and replace the address part of its bits
void remappage(uint64_t vaddr, int pfn) {
    if (!isfree(pfn))
        panic("panic: remappage, pfn: is not free\n");
    pte_t *pte = getpte(vaddr);
    uintptr_t addr = *pte >> 12;
    freepg((void*)P2V(addr), 1);
    *pte |= (pfn * PGSIZE) << 12;
}

void memset64(uint64_t* mem, int n) {
    for (int i = 0; i < n; ++i) {
        mem[i] = 0;
    }
}


pte_t *getpte(uint64_t vaddr) {
    uintptr_t addr;
    uint16_t index = vaddr >> 39;
    if ((PML4E[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PML4E not in use\n");
    addr = PML4E[index] >> 12;
    pdpte_t *PDPTE = P2V(addr);
    index = (vaddr >> 30) & 0x1FF;
    if ((PDPTE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PDPTE not in use\n");
    addr = PDPTE[index] >> 12;
    pde_t *PDE = P2V(addr);
    index = (vaddr >> 21) & 0x1FF;
    if ((PDE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PDE not in use\n");
    addr = PDE[index] >> 12;
    pte_t *PTE = P2V(addr);
    index = (vaddr >> 12) & 0x1FF;
    if ((PTE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PTE not in use\n");
    return &PTE[index];
}

// set new entry in the table_entry table
// returns the new entry
uint64_t* newentry(uint64_t *table_entry, uint64_t paddr, uint8_t flags) {
    uint64_t *page;
    if (paddr != 0)
        page = pallocaddr(1, paddr);
    else
        page = palloc(1);
    paddr = V2P(page);
    *table_entry = ((uintptr_t)paddr << 12);
    *table_entry |= flags;
    memset64(page, PGSIZE);
    return page;
}

G. Sliepen · Accepted Answer · 2022-08-08 22:36:39Z

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code mutiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

There is too much repetition in this function:

pte_t *getpte(uint64_t vaddr) {
    uintptr_t addr;
    uint16_t index = vaddr >> 39;
    if ((PML4E[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PML4E not in use\n");
    addr = PML4E[index] >> 12;
    pdpte_t *PDPTE = P2V(addr);
    index = (vaddr >> 30) & 0x1FF;
    if ((PDPTE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PDPTE not in use\n");
    addr = PDPTE[index] >> 12;
    pde_t *PDE = P2V(addr);
    index = (vaddr >> 21) & 0x1FF;
    if ((PDE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PDE not in use\n");
    addr = PDE[index] >> 12;
    pte_t *PTE = P2V(addr);
    index = (vaddr >> 12) & 0x1FF;
    if ((PTE[index] & PRESENT) == 0)
        panic("ERROR: getpte(), PTE not in use\n");
    return &PTE[index];
}

It might be better to creat a function that performs the test and calls the panic function:

static void test_and_panic(uint64_t test_value, const char* name)
{
    if (!(test_value & PRESENT))
    {
        char panic_buffer[256];
        sprintf(panic_buffer, "ERROR: getpte(), %s not in use\n", name);
        panic(panic_buffer);
    }
}

pte_t *getpte(uint64_t vaddr)
{
    uint16_t index = vaddr >> 39;
    test_and_panic(PML4E[index], "PML4E");
    uintptr_t addr = PML4E[index] >> 12;

    pdpte_t *PDPTE = P2V(addr);
    index = (vaddr >> 30) & 0x1FF;
    test_and_panic(PDPTE[index], "PDPTE");
    addr = PDPTE[index] >> 12;

    pde_t *PDE = P2V(addr);
    index = (vaddr >> 21) & 0x1FF;
    test_and_panic(PDE[index], "PDE");
    addr = PDE[index] >> 12;

    pte_t *PTE = P2V(addr);
    index = (vaddr >> 12) & 0x1FF;
    test_and_panic(PTE[index], "PTE");

    return &PTE[index];
}

Notes about the above example:

!(expression) is equivalent to (expression) == 0.
A best practice is to always initialize a variable when it declared.
Another best practice is to only declare a variable when it is needed.
For ease of maintenance, a best practice is for if statements and loops to always have a complex statement (brackets).
It isn't clear from the variable names what the variables represent, what are PDPTE, PML4E, PDE and PTE?
It isn't clear that the 0x1FF mask is the proper value for each of these values.

Magic Numbers

There are Magic Numbers in the getpte() function (39, 12, 30, 0x1FF, 21 and 12), it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintainence easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Don't Reinvent the Wheel

The function memset64() is unnecessary, the C library provides the function memset(). The C library version is generally optimized and will perform better than the current implementation.

It could be even DRYer by expanding test_and_panic() to also do the shifting and producing an address. If someone is writing a page table walker, then perhaps they don't have a C library they can rely on, so that might be why there's a memset64(), but otherwise it's of course better to use whatever is already available. — G. Sliepen
– G. Sliepen, Commented Aug 8, 2022 at 22:39
@G.Sliepen if they need to write their own memset then pointers rather than indexing is probably the way to do it. They might not be able to trust the optimization a compiler provides in code this low level. — pacmaninbw
– pacmaninbw ♦, Commented Aug 8, 2022 at 23:12

Stack Exchange Network

Virtual memory manager in C

1 Answer 1

DRY Code

Magic Numbers

Don't Reinvent the Wheel

You must log in to answer this question.

Hot Network Questions

Virtual memory manager in C

1 Answer 1

DRY Code

Magic Numbers

Don't Reinvent the Wheel

You must log in to answer this question.

Related

Hot Network Questions