For Your Review, My memory manager.

[astrocrep]

So this is my low level memory manager...
It works in Qemu... fails on real hw (only tested on 1 machine)
I think the reason its failing is because I am assuming the top of ram is ok...

Anyway I am pasting the whole file here.

I would really like to get some feedback on it... both good and bad... but always constructive ...

The more feedback the better...

Thanks,
Rich

Edit: I know its missing the default allocated pages (where the kernel is etc etc)... I will add for the that in a day or two...

Code: Select all

#include <memman.h>
#include <xprintf.h>
#include <multiboot.h>

/* 
 * The follow group of unsigned longs are used
 * in finding and aligned the positions of the 
 * page tables and memory bitmaps
 * */
unsigned long size_of_bitmap;
unsigned long memory_roof;
unsigned long memory_bitmap_head;
unsigned long page_table_count;
unsigned long page_table_space;
unsigned long page_dir_head;
unsigned long page_tables_head;

/* 
 * This below is the actual memory bitmap
 * */
unsigned char *memory_bitmap;

/*
 * Below is the actual memory manager goodies
 * These guys keep track of page tallies...
 * */

unsigned long total_pages;
unsigned long free_pages;

/*
 * Paging goodies...
 * */
unsigned long *page_directory;
extern unsigned long read_cr0(void); 
extern void write_cr0(unsigned long cr0); 
extern unsigned long read_cr3(void); 
extern void write_cr3(unsigned long *cr3);

/*
 * Two function below from osdev.org forums...
 * */
char check_page(unsigned long Page)
{
	unsigned long offset, bit;
 	unsigned char Byte;
 	offset = Page/8;
 	bit = Page&7;
 	bit = (1<<bit); 
 	Byte = memory_bitmap[offset]; 
 	if ((bit&Byte) == bit)
  		return 1; 
 	return 0; 
}

void change_page(unsigned long Page, char Value) 
{
 	unsigned long offset, bit, mask;
 	unsigned char Byte;
 	offset = Page/8;
 	bit = Page&7;
 	mask = ~(1<<bit); 
 	bit = (Value<<bit); 
 	Byte = memory_bitmap[offset]; 
 	Byte&=mask; 
 	Byte|=bit; 
 	memory_bitmap[offset] = Byte; 
} 

/* 
 * Creates a valid PT filled with 1:1 PTEs
 * */
void create_pt(unsigned long* buffer, char options, long starting_mb)
{
	long address = starting_mb * (1024 * 1024);
	int i;

 	for(i=0; i<1024; i++) 
  	{ 
   		buffer[i] = address | options; 
   		address += 4096; 
  	} 
}

/*
 * This is the actual "install"
 **/
void mm_init(multiboot_info_t *mbd)
{
	int i;
	char *temp;
	
	kprintf("Ram Detected: %d MB\n", (mbd->mem_upper + 1024) / 1024);
        
    memory_map_t *mmap; 
    
   	kprintf("Memory Table found at: 0x%x\n",mbd->mmap_addr); 
   
    kprintf("BASE\t\t\tLENGTH\t\t\tTYPE\n"); 

	mmap = (memory_map_t *) (mbd->mmap_addr); 
    while((unsigned long) mmap < mbd->mmap_addr + mbd->mmap_length) 
    { 
    	kprintf("%dkb",mmap->base_addr_low/1024);
    	kprintf("\t\t%dkb",mmap->length_low/1024);
    	kprintf("\t\t%d\n",mmap->type);
    	
    	if (mmap->type == 1)
    	{
    		if ((mmap->base_addr_low+mmap->length_low) > memory_roof)
    		{
    			memory_roof = mmap->base_addr_low+mmap->length_low;
    		}
    	}
      	mmap = (memory_map_t *) ((unsigned long) mmap + mmap->size + sizeof (mmap->size)); 
	}
	
	total_pages = memory_roof / 4096;
	free_pages = total_pages;
	
	size_of_bitmap = (memory_roof / 4096) / 8;
		
	memory_bitmap_head = memory_roof - size_of_bitmap;
		
	page_table_count = ((memory_roof / 1024) / 1024) / 4;
		
	page_table_space = page_table_count * 4096;
		
	page_dir_head = memory_bitmap_head - page_table_space - 4096;
		
	page_directory = (unsigned long *) page_dir_head;
	
	page_tables_head = memory_bitmap_head - page_table_space;
		
	for (;;)
	{
		if ((page_dir_head % 4096) != 0)
			page_dir_head--;
		else
			break;

	}
		
	memory_bitmap = (char *)(memory_bitmap_head);
	
	//Do a test to ensure we didn't screw anything up...
	for(i=0;i<size_of_bitmap;i++)
		memory_bitmap[i] = 1;
	
	for(i=0;i<size_of_bitmap;i++)
		if (memory_bitmap[i] != 1)
		{
			kprintf("FATAL ERROR: Memory Manager Calculations Failed!\nHalting...");
			for(;;);
		}

	//Push 0's back in there...
	for(i=0;i<size_of_bitmap;i++)
		memory_bitmap[i] = 0;

	for(i=0;i<1024;i++)
	{
		if (i<page_table_count)
		{
			page_directory[i] = (page_tables_head + (i * 4096));
			page_directory[i] = page_directory[i] | 3;
			create_pt((page_tables_head + (i * 4096)),3,(i * 4));	
		}
		else
		{
			page_directory[i] = 0 | 2;
		}
	}
	
	write_cr3((long)page_directory); // put that page directory address into CR3 
 	write_cr0(read_cr0() | 0x80000000); // set the paging bit in CR0 to 1 
 	kprintf("Success!\n");
}

void mm_install()
{
	
}

char *AllocPages(int size) 
{ 
	 int pages = size; 
	 int i=0; 
	 int count=0; 
	 int t=0; 

	 if (pages > free_pages) return 0;           /* Chunk to Big to Allocate */ 
	 for (i=0;i < (size_of_bitmap * 8);i++)                    /* Loop through the Page Bit Map */ 
	 { 
		if (check_page(i) == 0)                         /* If Bit map index is 0 */ 
		{ 
			for (count = 0; count < pages; count++)      /* Enter an Inner loop to find Consecutive Pages */ 
			if (check_page(i+count) != 0) break;         /* if this page isn't free stop checking */ 
			if (count == pages)                          /* After the loop, are the pages free equal to what we are looking for? */ 
			{ 
				for (t=0; t<count; t++)                    /* Lets Declare this chain of Pages Allocated */ 
					change_page(i+t,1);                        /* Set it to Non-Zero*/ 
				return (char *)(i*4096);                                   /* Return the Location of the Starting Page */ 
		   } 
			i += count; 
		} 
	 } 
	 return 0; 
}

[jnc100]

I haven't looked through all your code, but

Code: Select all

for (;;)
   {
      if ((page_dir_head % 4096) != 0)
         page_dir_head--;
      else
         break;

   } 

How slow is that?! Why not just

Code: Select all

page_dir_head &= 0xfffff000

Also, you use a lot of divides by 8 and 4096. I'd imagine the gcc optimiser does this for you, but if you want to be sure, its a lot quicker to replace divisons like this with right shifts, e.g.

Code: Select all

i / 8     = i >> 3

Regards,
John.

[astrocrep]

Thanks for the comments.

-Rich

[Candy]

Also, you use a lot of divides by 8 and 4096. I'd imagine the gcc optimiser does this for you, but if you want to be sure, its a lot quicker to replace divisons like this with right shifts, e.g.

Code: Select all

i / 8     = i >> 3

Regards,
John.

For the love of all that's holy - or whatever your religion/nonreligion prescribes - leave optimizing to the optimizer. Use a divide when you intend a divide, use a shift when you intend a shift. The optimizer will make the code fast.

If you really really want to make your code fast, go work on the optimizer instead.

[jnc100]

Surely there's no way that an optimiser would optimise i / 8 to anything quicker than it would optimise i >> 3 to?

[frank]

I have lots of code that uses divides and I have looked at the source that the compiler generates and very rarely do I actually see a div operation. i mostly see shifts and masks.

[Tyler]

Surely there's no way that an optimiser would optimise i / 8 to anything quicker than it would optimise i >> 3 to?

It will always do that, hence the reason you should not do it in code, as it reduces the readability and makes it even harder to understand.

[astrocrep]

Well, I decided to pretty much chalk up the mm as a learning experience...

I am now writing a seperate low-level page allocator, and then the paging mechanisim on top of that...

So far so good, I hope to be enabling paging with my newer mm tonight.

EDIT:The code above is sooo toast... got myself an excellent dynamic paging mm... (created by myself too!)

Thanks,
Rich