Hi, Ingo Glad to see your comments.
On Fri, Jan 27, 2017 at 09:47:23AM +0100, Ingo Molnar wrote: > >* Wei Yang <[email protected]> wrote: > >> e820_all_mapped() iterates the e820 table to check whether a region is >> mapped or not. Since the e820 table is sorted, when the region is less than >> the current region, no need to continue the iteration. >> >> The patch breaks the loop accordingly. >> >> Signed-off-by: Wei Yang <[email protected]> >> --- >> arch/x86/kernel/e820.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c >> index 90e8dde..f4fb197 100644 >> --- a/arch/x86/kernel/e820.c >> +++ b/arch/x86/kernel/e820.c >> @@ -86,10 +86,16 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned >> type) >> for (i = 0; i < e820->nr_map; i++) { >> struct e820entry *ei = &e820->map[i]; >> >> + /* Since the e820 table is sorted, when the region is less >> + * than the current region, break it. >> + */ >> + if (ei->addr >= end) >> + break; > >Please have a look at the relevant sections in Documentation/CodingStyle. (And >yes, this file violates it in a number of ways, but that's no reason to add to >the >mess.) > I took a look in the Documentation/CodingStyle, looks the comment style is not the preferred one. While I have checked this by scripts/checkpatch.pl, which prompts 0 error and 0 warning, so I thought it is fine. I would change this if you like. >But, more importantly, the reason I have not applied the patch before is that >while it's true that the e820 map is _eventually_ sorted, have you made >certain >that all calls to e820_all_mapped() are done when the map is already sorted? > I think so, if not those caller really need to make sure not to do this at that moment. The e820_all_mapped() can't work well if it is not sorted, even without this change. For example, we have two ranges [0x1000, 0x1FFF] and [0x2000, 0x2FFF] but they are not sorted in e820. Then a range [0x1500, 0x2500] would be judge not all mapped, which actually is in range [0x1000, 0x2FFF]. BTW, I went through all the callers, and most of them are called from subsys_init() or arch_initcall() which is after setup_memory_map() called in setup_arch(). And two of them are called by e820_add_kernel_range() and efi_reserve_boot_services(), and both are after setup_memory_map(). >Thanks, > > Ingo -- Wei Yang Help you, Help me
signature.asc
Description: PGP signature
