Skip to contentSkip to conversation

bootmem.c clean up bad pfn convertions

PATCH In review Franck Bui-Huu 2 participants 3 messages Apr 8, 2005 – Apr 11, 2005

lore
Share & tools
lore

Franck Bui-Huu

author
#0 lore raw

PATCH 1/1 bootmem.c clean up bad pfn convertions

Hi,

As I described in my previous email, bootmem.c does improper pfn convertions into phys addr. This simple patch fixes that.

Regards,

Franck.

-------------------------------------------------------------------------------
This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. E-mail transmission cannot be guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or contain viruses. The sender therefore does not accept liability for any errors or omissions in the contents of this message, which arise as a result of e-mail transmission. If verification is required please request a hard-copy version.
INNOVA CARD will not therefore be liable for the message if modified.
-------------------------------------------------------------------------------

bootmem.c +10 -7
@@ -58,8 +58,8 @@ static unsigned long __init init_bootmem
 	pgdat_list = pgdat;
 
 	mapsize = (mapsize + (sizeof(long) - 1UL)) & ~(sizeof(long) - 1UL);
-	bdata->node_bootmem_map = phys_to_virt(mapstart << PAGE_SHIFT);
-	bdata->node_boot_start = (start << PAGE_SHIFT);
+	bdata->node_bootmem_map = phys_to_virt(pfn_to_phys(mapstart));
+	bdata->node_boot_start = pfn_to_phys(start);
 	bdata->node_low_pfn = end;
 
 	/*

@@ -86,11 +86,11 @@ static void __init reserve_bootmem_core(
 	unsigned long sidx = (addr - bdata->node_boot_start)/PAGE_SIZE;
 	unsigned long eidx = (addr + size - bdata->node_boot_start + 
 							PAGE_SIZE-1)/PAGE_SIZE;
-	unsigned long end = (addr + size + PAGE_SIZE-1)/PAGE_SIZE;
+	unsigned long end = phys_to_pfn(PAGE_ALIGN(addr + size));
 
 	BUG_ON(!size);
 	BUG_ON(sidx >= eidx);
-	BUG_ON((addr >> PAGE_SHIFT) >= bdata->node_low_pfn);
+	BUG_ON(phys_to_pfn(addr) >= bdata->node_low_pfn);
 	BUG_ON(end > bdata->node_low_pfn);
 
 	for (i = sidx; i < eidx; i++)

@@ -111,7 +111,7 @@ static void __init free_bootmem_core(boo
 	 */
 	unsigned long sidx;
 	unsigned long eidx = (addr + size - bdata->node_boot_start)/PAGE_SIZE;
-	unsigned long end = (addr + size)/PAGE_SIZE;
+	unsigned long end = phys_to_pfn(addr + size);
 
 	BUG_ON(!size);
 	BUG_ON(end > bdata->node_low_pfn);

@@ -260,6 +260,7 @@ static unsigned long __init free_all_boo
 	unsigned long i, count, total = 0;
 	unsigned long idx;
 	unsigned long *map; 
+	unsigned long mapsize;
 	int gofast = 0;
 
 	BUG_ON(!bdata->node_bootmem_map);

@@ -267,7 +268,7 @@ static unsigned long __init free_all_boo
 	count = 0;
 	/* first extant page of the node */
 	page = virt_to_page(phys_to_virt(bdata->node_boot_start));
-	idx = bdata->node_low_pfn - (bdata->node_boot_start >> PAGE_SHIFT);
+	idx = bdata->node_low_pfn - phys_to_pfn(bdata->node_boot_start);
 	map = bdata->node_bootmem_map;
 	/* Check physaddr is O(LOG2(BITS_PER_LONG)) page aligned */
 	if (bdata->node_boot_start == 0 ||

@@ -313,7 +314,9 @@ static unsigned long __init free_all_boo
 	 */
 	page = virt_to_page(bdata->node_bootmem_map);
 	count = 0;
-	for (i = 0; i < ((bdata->node_low_pfn-(bdata->node_boot_start >> PAGE_SHIFT))/8 + PAGE_SIZE-1)/PAGE_SIZE; i++,page++) {
+	mapsize = (bdata->node_low_pfn - phys_to_pfn(bdata->node_boot_start)+7)/8;
+	mapsize = (mapsize + PAGE_SIZE-1)/PAGE_SIZE;
+	for (i = 0; i < mapsize; i++, page++) {
 		count++;
 		__ClearPageReserved(page);
 		set_page_count(page, 1);
#1 lore raw
Quoted 2 lines · Franck Bui-Huu

As I described in my previous email, bootmem.c does improper pfn convertions into phys addr. This simple patch fixes that.

...

bootmem.c static unsigned long __init init_bootmem · line 62

Quoted 4 lines
bootmem.c +2 -2
-       bdata->node_bootmem_map = phys_to_virt(mapstart << PAGE_SHIFT);
-       bdata->node_boot_start = (start << PAGE_SHIFT);
+       bdata->node_bootmem_map = phys_to_virt(pfn_to_phys(mapstart));
+       bdata->node_boot_start = pfn_to_phys(start);

The only arch with phys_to_pfn() defined is UML, so the patch simply won't compile anything but UML on current kernels (unless I'm missing something).

Could you try to give us a more complete description of your problem? I know your memory doesn't start at 0x0, but what problems does that cause? Does the mem_map[] allocation blow up, etc...

If it's just mem_map[], That calculation could be fixed pretty easily. Something like

+#ifdef CONFIG_CRAZY_MIPS_FOO_MEM_MAP_START...
+extern unsigned long mem_map_start_pfn
+#else
+#define mem_map_start_pfn 0UL
+#endif
-#define pfn_to_page(pfn) (mem_map + (pfn))
+#define pfn_to_page(pfn) (mem_map + (pfn) - mem_map_start_pfn)

(those names are horrid, please improve them, if you plan to do this)

All of the zone (and allocator) calculations should be just fine, because it already has a zone_start_pfn.

-- Dave

author
#2 lore raw
Quoted 5 lines · Dave Hansen

The only arch with phys_to_pfn() defined is UML, so the patch simply won't compile anything but UML on current kernels (unless I'm missing something).

oops, I forgot to send the part of the patch that defines these macros, sorry.

Quoted 5 lines

Could you try to give us a more complete description of your problem? I know your memory doesn't start at 0x0, but what problems does that cause? Does the mem_map[] allocation blow up, etc...

yes, it actually intends to solve that.

Quoted 7 lines

If it's just mem_map[], That calculation could be fixed pretty easily. Something like

Review

Quoted 7 lines
(quoted diff) +6 -1
+#ifdef CONFIG_CRAZY_MIPS_FOO_MEM_MAP_START... 
+extern unsigned long mem_map_start_pfn
+#else
+#define mem_map_start_pfn 0UL
+#endif
-#define pfn_to_page(pfn)        (mem_map + (pfn))
+#define pfn_to_page(pfn)        (mem_map + (pfn) - mem_map_start_pfn)

This is another solution indeed...But you will have to define another constant which will be used for virtual to physical address convertion. In my case I have

#define phys_to_pfn(x) (((x) - PHYS_START) >> PAGE_SHIFT)
#define pa(x) ((x) - PAGE_OFFSET + PHYS_START)

and "pfn_to_page" stays untouch.
But I expect your solution easier to implement since as you said previously no arch defines "phys_to_pfn".

Thanks

Franck.

-------------------------------------------------------------------------------
Come to visit Innova Card at CTST (Las Vegas, April 11-14, 2005) on booth 559 and Cards Asia (Singapore, April 27-29, 2005) on booth 4A04 !

links:
- www.ctst.com
- www.worldofcards.biz/2005/cardsa_SG/

-------------------------------------------------------------------------------
This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system.
Innova Card will not therefore be liable for the message if modified.