OSDev.org

Posted: **Sun Jul 15, 2007 4:30 am**

I've set my last page directory entry to the page directory itself, as that seemed the most elegant solution. I'm now trying to write a memory map function, but I can't seem to find out what is wrong with it. I'm betting it's something obvious, but after staring at code for all this time, I've probably become blind to the solution, so need a fresh set of eyes to look at it.

Forgive me if I've not provided enough information. Let me know if you need to see any more of my code:

Code: Select all

#define PAGE_KERNEL_RW 0x3
#define PAGE_USER_RW 0x7
#define PAGE_NOT_PRESENT 0x0
#define PAGE_PRESENT 0x3

#define MAKE_ADDRESS(x,y,z) (((x)<<22) | ((y)<<12) | z)

/* Map virt_addr onto phys_addr */
void pmman_map(uint32 phys_addr, uint32 virt_addr, uint32 flags)
{
	uint32 phys_page, virt_page;
	uint32 *directory;
	uint32 *table;

	/* Get page boundary below address */
	phys_page = phys_addr & 0xfffff000;
	virt_page = virt_addr & 0xfffff000;
	
	/* Get PDE and PTE */
	directory = (uint32 *)MAKE_ADDRESS(1023, 1023, (virt_page>>20));
	table = (uint32 *)MAKE_ADDRESS(1023, (virt_page>>22), 0);
	
	/* If directory entry not present, set it */
	if ((*directory & PAGE_PRESENT) == PAGE_NOT_PRESENT)
	{
		*directory = (uint32)table | PAGE_USER_RW;
	}
	
	/* Set page table entry */
	table += (virt_page>>12) & 0x3ff;
	*table = phys_page | flags;
}

The function itself executes without any faults, however, when trying to access the virtual address after mapping it, a page fault is thrown at the address which is trying to be accessed.

Let me know if you need more info.

Cheers,
Phill

Posted: **Sun Jul 15, 2007 6:18 am**

I have no idea how you decided to do shifts like that. I am assuming you did not take the time to debug the function and see what the values were. (virt_page>>12), should have easily went well over the value 1023 for a index - most likely right off into some memory and corrupted something.

Code: Select all

/* Map virt_addr onto phys_addr */
void pmman_map(uint32 dir, uint32 phys_addr, uint32 virt_addr, uint32 flags)
{
   uint32 phys_page, virt_page;
   uint32 *directory;
   uint32 *table;

   /* Get page boundary below address */
   phys_page = phys_addr & 0xfffff000;
   virt_page = virt_addr & 0xfffff000;
   
   /* Get PDE */
   directory = (uint32*)dir;
   
   if(directory[virt_addr>>22] == 0)
   {
        /* Table not even here? */
        directory[virt_addr>>22] = ALLOCATE_4096_KB_PAGE() | PAGE_USER_RW;
   }
   /* If directory table not present, set it */
   if (!(directory[virt_addr>>22] & PAGE_PRESENT))
   {
      directory[virt_addr>>22]  |= PAGE_USER_RW;
   }
   
   /* Set page table entry */
   table = (uint32*)(directory[virt_addr>>22] & ~0xFFF);
   table[virt_addr<<10>>10>>12] = phys_addr | flags;
}

(bit31)......(bit22) (bit21)......(bit12) (bit11.......bit0)
[-------PDE------] [------PTE--------] [------FLAGS-----]

If you notice bit11 to bit0 make twelve bits. Lets inspect the maximum value of twelve bits: 0xFFF = 4095. Each 0xF is a nibble which is 4bits. So we have three Fs which is three times four equals twelve. Since 0x0000 starts counting at zero we can note that once we have 0x0FFF which is 4095 we actually have 4096 counts. So using 0xFFF we can represent a memory address from 0 to 4095. Notice, how they almost make 4096.

The next bit which is bit11 will make the value 4096 like: 0x1000 = 4096. If you notice this bit with a value of one is in the PTE slot (above). Lets count them 0xF=4, 0xFF=8, 0xFFF=12, 0x1000=13. That is bit13 instead of bit12, right? Nope. My depiction above starts at bit0 so subtract one from bit13 and you get bit12 which is the first bit of the PTE.

The PTE has 10bits. The maximum value of 10bits is 0x3FF which is 1023. That makes a total of 1024 PTEs per table since we start counting at zero: 0x000 to 0x3FF. The page directory is exactly the same way which means that the directory has 1024 PDEs (starting at 0 and ending at 1023), and each table has 1024 PTEs (starting at 0 and ending at 1023).

To extract the bits you use shifts:
(phy_addr>>22): There are 22bits if you count them that make up the PTE and FLAGS (12 + 10 = 22). We shift them to the right where they disappear and are replaced by zeros on the left.

(phy_addr>>12 & 0x3FF): This removes the FLAGS part which is 12bits. The problem is the PDE is still at the end giving us a crazy value if we tried to use it. So we AND by 0x3FF which chops the ending off since if you remember 0x3FF is really 10bits.

phy_addr........ = 1011011100 0101011111 011111010101
phy_addr>>12 = 000000000000 1011011100 0101011111
0x3FF = ............. 000000000000 0000000000 1111111111
phy_addr>>12 & 0x3FF = (below - one line)[/i]
..........................000000000000 0000000000 0101011111

Posted: **Sun Jul 15, 2007 7:06 am**

Kevin McGuire wrote:I have no idea how you decided to do shifts like that. I am assuming you did not take the time to debug the function and see what the values were. (virt_page>>12), should have easily went well over the value 1023 for a index - most likely right off into some memory and corrupted something.

Hehe, looks like I'm not the only one who's tired

I feel a lot better now that I know I'm not the only one who can get confused by all this bit shifting.
Only place I have (virt_page>>12) is where there is also a (& 0x3FF), which of course guarantees that the value cannot be greater than 1023.

After a break watching a film, I came back to it and saw the problem immediately... As I thought a stupid error, and I've no idea what I was doing, but it was just a matter of changing one line from:
*directory = (uint32)table | PAGE_USER_RW;
to:
*directory |= PAGE_USER_RW;

It now works perfectly. Thanks for your time writing your post. I did read it and I appreciate the refresher. If only I had read what you posted before I started writing this function, it would have saved me a lot of time figuring it out myself.

thanks again,
Phill

Posted: **Sun Jul 15, 2007 7:14 am**

Well, It makes it very difficult when someone writes code that is very difficult to interpret. You made things way to complicated by using crazy macros.

I have no idea how you decided to do shifts like that. I am assuming you did not take the time to debug the function and see what the values were.

If I was going to help someone and saw code like that I would immediately stop and tell them I am sorry, but I am unable to follow the code.

After a break watching a film, I came back to it and saw the problem immediately... As I thought a stupid error, and I've no idea what I was doing, but it was just a matter of changing one line from:

How about you stop posting really trivial questions, and spend that time working on it instead of watching a film while you use the forum to post time sink questions.

Just to clarify that I do not mind trivial questions; if someone is truly devoted to trying to figure out what the problem is. I really get mad when the trivial problem is so unimportant that watching a movie and coming back was the solution and justification for my time trying to explain that the code is hard to read, and I *assumed* that it was incorrect because of the complexity involved.

Posted: **Sun Jul 15, 2007 7:44 am**

I've just got back from holiday and am reading through the 103 new posts quickly, but shouldn't the page present bit be bit 0 rather than bit 1? (0x01 rather than 0x03)?

Posted: **Sun Jul 15, 2007 8:15 am**

You could be right AJ, but I think I just mis-named the PAGE_PRESENT constant.

Just to clarify that I do not mind trivial questions; if someone is truly devoted to trying to figure out what the problem is. I really get mad when the trivial problem is so unimportant that watching a movie and coming back was the solution and justification for my time trying to explain that the code is hard to read, and I *assumed* that it was incorrect because of the complexity involved.

I spent a much much longer time than I would like to admit before posting on the forum, and I ran it through a debugger time and time again. I really do agree with you about time wasting on forums, but assure you, I actually kept working on it to the point where I was loosing my mind and had to take a break. I didn't mean to give the impression that I decided to watch a film and let you solve it for me - I enjoy programming because of the problem solving, so I ask for help as a last resort.

The weird MAKE_ADDRESS macro, i got from someone else in IRC... didn't think twice about how ugly it looked, because I understood it at the time. I could use some more comments, but I didn't think the rest was that terrible?

Let's make this topic a little more valuable: Let me know how you think I could improve the readability and niceness of my code. I'm blinded by the fact that I wrote it, and I wrote it today (and yesterday).

cheers,
Phill.

OSDev.org

Physical Memory Map function not working as expected

Physical Memory Map function not working as expected