[Xen-devel] Re: [PATCH] e820: fix clip_to_limit()

[Xiao Guangrong <xiaoguangrong@xxxxxxxxxxxxxx>]

Keir Fraser wrote:
> On 10/11/2009 08:19, "Xiao Guangrong" <xiaoguangrong@xxxxxxxxxxxxxx> wrote:
> 
>>> Firstly, your 'break' was not inside that if-else block; it was right at the
>>> end of the for loop. Secondly, just because we found one RAM region entirely
>>> beyond the end of the clip boundary, does not mean there isn't another. We
>>> can't just bail -- we have to iterate all the way to the end of the e820
>>> map.
>>>
>> I think that sanitize_e820_map() can sort e820 items from low address
>> to high address, so, if we meet one e820 item beyond the end of the clip
>> boundary, subsequent items also beyond it.
>>
>> Maybe I misunderstand sanitize_e820_map()? I'll reread it :-)
> 
> No, you understand it. And if we meet one e820 item beyond the end of the
> clip boundary, all subsequent items are also beyond it. But that doesn't
> mean we shouldn't handle them -- in fact we must handle them, as one of them
> could be E820_RAM. Right?
> 

Yeah, It's my mistake, Thanks very much, Keir!

And I think find_max_pfn() can be optimized. like this:

--- ../a/xen/arch/x86/e820.c    2009-08-06 21:57:27.000000000 +0800
+++ ../b/xen/arch/x86/e820.c    2009-10-25 17:31:42.762997342 +0800
@@ -312,8 +312,9 @@ static unsigned long __init find_max_pfn
     }
 #endif

-    for (i = 0; i < e820.nr_map; i++) {
+    for (i = e820.nr_map -1; i >= 0; i--) {
         unsigned long start, end;
+
         /* RAM? */
         if (e820.map[i].type != E820_RAM)
             continue;
@@ -321,8 +322,8 @@ static unsigned long __init find_max_pfn
         end = PFN_DOWN(e820.map[i].addr + e820.map[i].size);
         if (start >= end)
             continue;
-        if (end > max_pfn)
-            max_pfn = end;
+       max_pfn = end;
+       break;
     }

     return max_pfn;


Thanks,
Xiao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel