[patch 3/3] IA64: verify the base address of crashkernel

[patch 3/3] IA64: verify the base address of crashkernel

[Horms]

When the crashkernel command line argument is supplied, it may optionally
include the base address at which to locate the region. If this is omitted
then the kernel attempts to locate an appropriate base address and in 
the course of this checks to make sure it is placed in a region that
is writable.

If, however, the base address is supplied on the command line this check is
not made. This patch adds validation code to:

* Warn is the base address is not aligned (to 64Mb)
* Invalidate the crashkernel region if it does not lie with in
  an appropriate EFI region
* Invalidate the crash kernel region if it clashes with a reserved region
  (this was kind of handled already by virtue of the way the resource
   is inserted into /proc/iomem)

It also defined CRASHDUMP_ALIGN rather than just hardcoding it
to 64Mb as needed (previously only in kdump_find_kdump_find_rsvd_region())

The code came out to be a little longer than I had expected.
It could be made more compact, likely at the expense of some readability.
But as its in __init, its not really a big deal IMHO.

An alternative approach would to be add checks at the time the
region is inserted to /proc/iomem. I'm not sure how well this would
work, but I am happy to investigate.

Signed-off-by: Simon Horman <horms@verge.net.au>

 arch/ia64/kernel/efi.c   |   84 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/ia64/kernel/setup.c |   10 ++++-
 include/asm-ia64/kexec.h |    2 +
 3 files changed, 91 insertions(+), 5 deletions(-)

Index: linux-2.6/arch/ia64/kernel/efi.c
=================================--- linux-2.6.orig/arch/ia64/kernel/efi.c	2007-03-06 16:25:46.000000000 +0900
+++ linux-2.6/arch/ia64/kernel/efi.c	2007-03-06 16:26:41.000000000 +0900
@@ -1150,6 +1150,85 @@
 }
 
 #ifdef CONFIG_KEXEC
+
+#define CRASHDUMP_ALIGNMENT (1 << _PAGE_SIZE_64M)
+static int __init
+kdump_region_verify_efi (unsigned long base, unsigned long size)
+{
+	void *efi_map_start, *efi_map_end, *p;
+	efi_memory_desc_t *md;
+	unsigned long efi_desc_size;
+
+	efi_map_start = __va(ia64_boot_param->efi_memmap);
+	efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
+	efi_desc_size = ia64_boot_param->efi_memdesc_size;
+
+	for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+		md = p;
+		if (base < md->phys_addr || base + size > efi_md_end(md))
+			continue;
+		if (is_memory_available(md))
+			return 1;
+		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx is "
+		       "inside the unusable efi region 0x%lx-0x%lx\n", base,
+		       base + size - 1, md->phys_addr, efi_md_end(md) - 1);
+		return 0;
+	}
+
+	printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx does not "
+	       "fit in any efi region\n", base, base + size - 1);
+	return 0;
+}
+
+
+/* find a block of memory aligned to 64M exclude reserved regions
+   rsvd_regions are sorted
+ */
+static int __init
+kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
+	       			 struct rsvd_region *rsvd_regions, int n)
+{
+	int i;
+
+	for (i = 0; i < n; i++) {
+		if (__pa(rsvd_regions[i].start) < base ||
+		    __pa(rsvd_regions[i].end) >= base + size - 1)
+			continue;
+		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
+		       "clashes with reserved region 0x%lx-0x%lx\n", base,
+		       base + size - 1, __pa(rsvd_regions[i].start),
+		       __pa(rsvd_regions[i].end));
+		return 0;
+	}
+	return 0;
+}
+
+
+/* find a block of memory aligned to 64M exclude reserved regions
+   rsvd_regions are sorted
+ */
+int __init
+kdump_region_verify (unsigned long base, unsigned long size,
+		     struct rsvd_region *rsvd_regions, int n)
+{
+	/* This isn't considered to be a failure condition,
+	 * but it isn't desireable either, so log it */
+	if (ALIGN(base, CRASHDUMP_ALIGNMENT) != base)
+		printk(KERN_WARNING "Kdump: warning: crashkernel region "
+		       "0x%lx-0x%lx is not aligned to 0x%x\n",
+		       base, base + size - 1, CRASHDUMP_ALIGNMENT);
+
+	if (!kdump_region_verify_efi(base, size))
+		return 0;
+
+	if (!kdump_region_verify_rsvd_region(base, size, rsvd_regions, n))
+		return 0;
+
+	printk(KERN_INFO "Kdump: crashkernel region verified\n");
+		return 1;
+	return 1;
+}
+
 /* find a block of memory aligned to 64M exclude reserved regions
    rsvd_regions are sorted
  */
@@ -1159,7 +1238,6 @@
 {
   int i;
   u64 start, end;
-  u64 alignment = 1UL << _PAGE_SIZE_64M;
   void *efi_map_start, *efi_map_end, *p;
   efi_memory_desc_t *md;
   u64 efi_desc_size;
@@ -1172,13 +1250,13 @@
 	  md = p;
 	  if (!efi_wb(md))
 		  continue;
-	  start = ALIGN(md->phys_addr, alignment);
+	  start = ALIGN(md->phys_addr, CRASHDUMP_ALIGNMENT);
 	  end = efi_md_end(md);
 	  for (i = 0; i < n; i++) {
 		if (__pa(r[i].start) >= start && __pa(r[i].end) < end) {
 			if (__pa(r[i].start) > start + size)
 				return start;
-			start = ALIGN(__pa(r[i].end), alignment);
+			start = ALIGN(__pa(r[i].end), CRASHDUMP_ALIGNMENT);
 			if (i < n-1 && __pa(r[i+1].start) < start + size)
 				continue;
 			else
Index: linux-2.6/arch/ia64/kernel/setup.c
=================================--- linux-2.6.orig/arch/ia64/kernel/setup.c	2007-03-06 16:25:40.000000000 +0900
+++ linux-2.6/arch/ia64/kernel/setup.c	2007-03-06 16:27:44.000000000 +0900
@@ -271,11 +271,17 @@
 			else
 				base = 0;
 			if (size) {
+				sort_regions(rsvd_region, n);
 				if (!base) {
-					sort_regions(rsvd_region, n);
 					base = kdump_find_rsvd_region(size,
 							      	rsvd_region, n);
-					}
+				}
+				else {
+					if (!kdump_region_verify(base, size,
+								 rsvd_region,
+								 n))
+						base = ~0UL;
+				}
 				if (base != ~0UL) {
 					rsvd_region[n].start  						(unsigned long)__va(base);
Index: linux-2.6/include/asm-ia64/kexec.h
=================================--- linux-2.6.orig/include/asm-ia64/kexec.h	2007-03-06 16:25:40.000000000 +0900
+++ linux-2.6/include/asm-ia64/kexec.h	2007-03-06 16:26:41.000000000 +0900
@@ -37,6 +37,8 @@
 extern void kexec_disable_iosapic(void);
 extern void crash_save_this_cpu(void);
 struct rsvd_region;
+extern int kdump_region_verify(unsigned long base,
+		unsigned long size, struct rsvd_region *rsvd_regions, int n);
 extern unsigned long kdump_find_rsvd_region(unsigned long size,
 		struct rsvd_region *rsvd_regions, int n);
 extern void kdump_cpu_freeze(struct unw_frame_info *info, void *arg);

--

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

RE: [patch 3/3] IA64: verify the base address of crashkernel

[Zou, Nanhai]

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Horms
> Sent: 2007Äê3ÔÂ6ÈÕ 15:29
> To: Linux-IA64; fastboot@lists.osdl.org
> Cc: Luck, Tony; Zou, Nanhai; Magnus Damm
> Subject: [patch 3/3] IA64: verify the base address of crashkernel
> 
> When the crashkernel command line argument is supplied, it may optionally
> include the base address at which to locate the region. If this is omitted
> then the kernel attempts to locate an appropriate base address and in
> the course of this checks to make sure it is placed in a region that
> is writable.
> 
> If, however, the base address is supplied on the command line this check is
> not made. This patch adds validation code to:
> 
> * Warn is the base address is not aligned (to 64Mb)
> * Invalidate the crashkernel region if it does not lie with in
>   an appropriate EFI region
> * Invalidate the crash kernel region if it clashes with a reserved region
>   (this was kind of handled already by virtue of the way the resource
>    is inserted into /proc/iomem)
> 
> It also defined CRASHDUMP_ALIGN rather than just hardcoding it
> to 64Mb as needed (previously only in kdump_find_kdump_find_rsvd_region())
> 
> The code came out to be a little longer than I had expected.
> It could be made more compact, likely at the expense of some readability.
> But as its in __init, its not really a big deal IMHO.
> 
> An alternative approach would to be add checks at the time the
> region is inserted to /proc/iomem. I'm not sure how well this would
> work, but I am happy to investigate.
> 
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
>  arch/ia64/kernel/efi.c   |   84
> ++++++++++++++++++++++++++++++++++++++++++++--
>  arch/ia64/kernel/setup.c |   10 ++++-
>  include/asm-ia64/kexec.h |    2 +
>  3 files changed, 91 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/arch/ia64/kernel/efi.c
> =================================> --- linux-2.6.orig/arch/ia64/kernel/efi.c	2007-03-06 16:25:46.000000000
> +0900
> +++ linux-2.6/arch/ia64/kernel/efi.c	2007-03-06 16:26:41.000000000 +0900
> @@ -1150,6 +1150,85 @@
>  }
> 
>  #ifdef CONFIG_KEXEC
> +
> +#define CRASHDUMP_ALIGNMENT (1 << _PAGE_SIZE_64M)
> +static int __init
> +kdump_region_verify_efi (unsigned long base, unsigned long size)
> +{
> +	void *efi_map_start, *efi_map_end, *p;
> +	efi_memory_desc_t *md;
> +	unsigned long efi_desc_size;
> +
> +	efi_map_start = __va(ia64_boot_param->efi_memmap);
> +	efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
> +	efi_desc_size = ia64_boot_param->efi_memdesc_size;
> +
> +	for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
> +		md = p;
> +		if (base < md->phys_addr || base + size > efi_md_end(md))
> +			continue;
> +		if (is_memory_available(md))
> +			return 1;
> +		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx is "
> +		       "inside the unusable efi region 0x%lx-0x%lx\n", base,
> +		       base + size - 1, md->phys_addr, efi_md_end(md) - 1);
> +		return 0;
> +	}
> +
> +	printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx does not "
> +	       "fit in any efi region\n", base, base + size - 1);
> +	return 0;
> +}
> +
> +
> +/* find a block of memory aligned to 64M exclude reserved regions
> +   rsvd_regions are sorted
> + */
> +static int __init
> +kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
> +	       			 struct rsvd_region *rsvd_regions, int n)
> +{
> +	int i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (__pa(rsvd_regions[i].start) < base ||
> +		    __pa(rsvd_regions[i].end) >= base + size - 1)
> +			continue;
> +		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
> +		       "clashes with reserved region 0x%lx-0x%lx\n", base,
> +		       base + size - 1, __pa(rsvd_regions[i].start),
> +		       __pa(rsvd_regions[i].end));
> +		return 0;
> +	}
> +	return 0;
> +}
> +
> +
> +/* find a block of memory aligned to 64M exclude reserved regions
> +   rsvd_regions are sorted
> + */
> +int __init
> +kdump_region_verify (unsigned long base, unsigned long size,
> +		     struct rsvd_region *rsvd_regions, int n)
> +{
> +	/* This isn't considered to be a failure condition,
> +	 * but it isn't desireable either, so log it */
> +	if (ALIGN(base, CRASHDUMP_ALIGNMENT) != base)
> +		printk(KERN_WARNING "Kdump: warning: crashkernel region "
> +		       "0x%lx-0x%lx is not aligned to 0x%x\n",
> +		       base, base + size - 1, CRASHDUMP_ALIGNMENT);
> +
> +	if (!kdump_region_verify_efi(base, size))
> +		return 0;
> +
> +	if (!kdump_region_verify_rsvd_region(base, size, rsvd_regions, n))
> +		return 0;
> +
> +	printk(KERN_INFO "Kdump: crashkernel region verified\n");
> +		return 1;
> +	return 1;
> +}
> +
>  /* find a block of memory aligned to 64M exclude reserved regions
>     rsvd_regions are sorted
>   */
> @@ -1159,7 +1238,6 @@
>  {
>    int i;
>    u64 start, end;
> -  u64 alignment = 1UL << _PAGE_SIZE_64M;
>    void *efi_map_start, *efi_map_end, *p;
>    efi_memory_desc_t *md;
>    u64 efi_desc_size;
> @@ -1172,13 +1250,13 @@
>  	  md = p;
>  	  if (!efi_wb(md))
>  		  continue;
> -	  start = ALIGN(md->phys_addr, alignment);
> +	  start = ALIGN(md->phys_addr, CRASHDUMP_ALIGNMENT);
>  	  end = efi_md_end(md);
>  	  for (i = 0; i < n; i++) {
>  		if (__pa(r[i].start) >= start && __pa(r[i].end) < end) {
>  			if (__pa(r[i].start) > start + size)
>  				return start;
> -			start = ALIGN(__pa(r[i].end), alignment);
> +			start = ALIGN(__pa(r[i].end), CRASHDUMP_ALIGNMENT);
>  			if (i < n-1 && __pa(r[i+1].start) < start + size)
>  				continue;
>  			else
> Index: linux-2.6/arch/ia64/kernel/setup.c
> =================================> --- linux-2.6.orig/arch/ia64/kernel/setup.c	2007-03-06
> 16:25:40.000000000 +0900
> +++ linux-2.6/arch/ia64/kernel/setup.c	2007-03-06 16:27:44.000000000
> +0900
> @@ -271,11 +271,17 @@
>  			else
>  				base = 0;
>  			if (size) {
> +				sort_regions(rsvd_region, n);
>  				if (!base) {
> -					sort_regions(rsvd_region, n);
>  					base = kdump_find_rsvd_region(size,
>  							      	rsvd_region, n);
> -					}
> +				}
> +				else {
> +					if (!kdump_region_verify(base, size,
> +								 rsvd_region,
> +								 n))
> +						base = ~0UL;
> +				}
>  				if (base != ~0UL) {
>  					rsvd_region[n].start >  						(unsigned long)__va(base);
> Index: linux-2.6/include/asm-ia64/kexec.h
> =================================> --- linux-2.6.orig/include/asm-ia64/kexec.h	2007-03-06
> 16:25:40.000000000 +0900
> +++ linux-2.6/include/asm-ia64/kexec.h	2007-03-06 16:26:41.000000000
> +0900
> @@ -37,6 +37,8 @@
>  extern void kexec_disable_iosapic(void);
>  extern void crash_save_this_cpu(void);
>  struct rsvd_region;
> +extern int kdump_region_verify(unsigned long base,
> +		unsigned long size, struct rsvd_region *rsvd_regions, int n);
>  extern unsigned long kdump_find_rsvd_region(unsigned long size,
>  		struct rsvd_region *rsvd_regions, int n);
>  extern void kdump_cpu_freeze(struct unw_frame_info *info, void *arg);
> 
> --
> 
> --
> Horms
>   H: http://www.vergenet.net/~horms/
>   W: http://www.valinux.co.jp/en/
> 
> -

Hi Horms,
	I feel this is over-designed.
     I think to specify crash kernel base address in command line is only useful for debug, on platform like SN this feature is totally unusable.At the most of time, user should let kernel to decide where to reserve crashdump region.
    If a user wants to put crash kernel in command line, he should know what he is doing.

Zou Nanhai

> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Aron Griffis]

Simon Horman wrote:  [Tue Mar 06 2007, 02:28:52AM EST]
> +/* find a block of memory aligned to 64M exclude reserved regions
> +   rsvd_regions are sorted
> + */
> +static int __init
> +kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
> +	       			 struct rsvd_region *rsvd_regions, int n)
> +{
> +	int i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (__pa(rsvd_regions[i].start) < base ||
> +		    __pa(rsvd_regions[i].end) >= base + size - 1)
> +			continue;
> +		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
> +		       "clashes with reserved region 0x%lx-0x%lx\n", base,
> +		       base + size - 1, __pa(rsvd_regions[i].start),
> +		       __pa(rsvd_regions[i].end));
> +		return 0;
> +	}
> +	return 0;
> +}

You're returning 0 in both cases here, is that what you really want?

> +/* find a block of memory aligned to 64M exclude reserved regions
> +   rsvd_regions are sorted
> + */
> +int __init
> +kdump_region_verify (unsigned long base, unsigned long size,
> +		     struct rsvd_region *rsvd_regions, int n)
> +{
> +	/* This isn't considered to be a failure condition,
> +	 * but it isn't desireable either, so log it */
> +	if (ALIGN(base, CRASHDUMP_ALIGNMENT) != base)
> +		printk(KERN_WARNING "Kdump: warning: crashkernel region "
> +		       "0x%lx-0x%lx is not aligned to 0x%x\n",
> +		       base, base + size - 1, CRASHDUMP_ALIGNMENT);
> +
> +	if (!kdump_region_verify_efi(base, size))
> +		return 0;
> +
> +	if (!kdump_region_verify_rsvd_region(base, size, rsvd_regions, n))
> +		return 0;
> +
> +	printk(KERN_INFO "Kdump: crashkernel region verified\n");
> +		return 1;
> +	return 1;

and here it appears you have an extra return.

Aron

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

Hi,

Here is a minor update to this patch, which makes the use
of log priorities more consistent.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Date: Tue, 06 Mar 2007 16:28:51 +0900
To: Linux-IA64 <linux-ia64@vger.kernel.org>,
 fastboot@lists.osdl.org
Cc: Tony Luck <tony.luck@intel.com>,
 Nanhai Zou <nanhai.zou@intel.com>,
 Magnus Damm <magnus.damm@gmail.com>
Subject: [patch 2/3] IA64: log insertion of crashkernel region
Message-Id: <20070306073756.030729468@tabatha.lab.ultramonkey.org>

This patch adds a faclilty to print out a message regarding the success or
failure of inserting the crashkernel region. On systems with a large
ammount of memory, the chances of failure tend to be quite low, especially
now that the base address of the region can be determined by the kernel.
However, on systems with less memory, such as Xen's Domain 0, it
can occur, and silently failing is confusing to say the least.

It also updates the error message generated by kdump_find_rsvd_region()
if it can't locate a base address for the requested crashkernel region,
which is really just another failure mode for the problem detailed
in the paragraph above.

* Update 
  - Consistently use KERN_WARNING for errors/warnings (which are not
    critical) and ERN_INFO for logging success

Signed-off-by: Simon Horman <horms@verge.net.au>

 linux-2.6/arch/ia64/kernel/efi.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Index: linux-2.6/arch/ia64/kernel/efi.c
=================================--- linux-2.6.orig/arch/ia64/kernel/efi.c	2007-03-07 09:39:15.000000000 +0900
+++ linux-2.6/arch/ia64/kernel/efi.c	2007-03-07 09:40:49.000000000 +0900
@@ -1076,6 +1076,9 @@
 	u64 efi_desc_size;
 	char *name;
 	unsigned long flags;
+#ifdef CONFIG_KEXEC
+	int crashk_res_inserted = 0;
+#endif
 
 	efi_map_start = __va(ia64_boot_param->efi_memmap);
 	efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
@@ -1152,11 +1155,25 @@
 #ifdef CONFIG_KEXEC
                         insert_resource(res, &efi_memmap_res);
                         insert_resource(res, &boot_param_res);
-			if (crashk_res.end > crashk_res.start)
-				insert_resource(res, &crashk_res);
+			if (!crashk_res_inserted &&
+			    crashk_res.end > crashk_res.start &&
+			    insert_resource(res, &crashk_res) >= 0)
+				crashk_res_inserted = 1;
+
 #endif
 		}
 	}
+
+	if (crashk_res.end > crashk_res.start) {
+		if (crashk_res_inserted)
+			printk(KERN_INFO "Kdump: registered crashdump: "
+			       "0x%08lx-0x%08lx\n", crashk_res.start,
+			       crashk_res.end);
+		else
+			printk(KERN_WARNING "Kdump: failed to insert resource "
+			       "for crashdump: 0x%08lx-0x%08lx\n",
+			       crashk_res.start, crashk_res.end);
+	}
 }
 
 #ifdef CONFIG_KEXEC
@@ -1277,8 +1294,8 @@
 		return start;
   }
 
-  printk(KERN_WARNING "Cannot reserve 0x%lx byte of memory for crashdump\n",
-	size);
+  printk(KERN_WARNING "Kdump: failed to find a base address for 0x%lx bytes"
+	 "of memory for crashdump\n", size);
   return ~0UL;
 }
 #endif

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

On Tue, Mar 06, 2007 at 04:23:37PM +0800, Zou, Nanhai wrote:
> 
> Hi Horms,
> 	I feel this is over-designed.
>      I think to specify crash kernel base address in command line is only useful for debug, on platform like SN this feature is totally unusable.At the most of time, user should let kernel to decide where to reserve crashdump region.
>     If a user wants to put crash kernel in command line, he should know what he is doing.

Hi Nanhai,

while I do agree that perhaps these checks are a little verbose I don't
agree that they are uneccessary. Specifying the base address is entirely
sane on some platforms (e.g. Tiger 2). And more to the point, it is the
only method available on some architectures, and thus its seems
reasonable to expect that it might work sanley on ia64. It seems to me
that it is a good idea to have some checks in place, in line with the
checks performed when the base address is automatically determinted to
make the behaviour (more) consistent. 

Ideally it would be good if there were not two code-paths relating
to base address selection - auto and manual. Or more to the point, if
they could share the same checks. But at this point I can't see a way to
make the code do that.

I guess in the end it comes down to how easy you want it to be for users
mistakes to be caught. I think that currently kexec/kdump is quite
fragile and its easy to end up with a setup that doesn't work. I think
that changes like this one are one small step towards making a more
robust system. Ditto for the change regarding loging success or failure
of inserting the crashkernel region.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

Hi Aron,

thanks for picking up these silly errors. An updated version is below.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

When the crashkernel command line argument is supplied, it may optionally
include the base address at which to locate the region. If this is omitted
then the kernel attempts to locate an appropriate base address and in 
the course of this checks to make sure it is placed in a region that
is writable.

If, however, the base address is supplied on the command line this check is
not made. This patch adds validation code to:

* Warn is the base address is not aligned (to 64Mb)
* Invalidate the crashkernel region if it does not lie with in
  an appropriate EFI region
* Invalidate the crash kernel region if it clashes with a reserved region
  (this was kind of handled already by virtue of the way the resource
   is inserted into /proc/iomem)

It also defined CRASHDUMP_ALIGN rather than just hardcoding it
to 64Mb as needed (previously only in kdump_find_kdump_find_rsvd_region())

The code came out to be a little longer than I had expected.
It could be made more compact, likely at the expense of some readability.
But as its in __init, its not really a big deal IMHO.

An alternative approach would to be add checks at the time the
region is inserted to /proc/iomem. I'm not sure how well this would
work, but I am happy to investigate.

* Updates thanks to Aron Griffis
  - kdump_region_verify_rsvd_region should return 1 (not 0) if its checks 
    succeed
  - Remove duplicate "return 1" from the end of kdump_region_verify

Signed-off-by: Simon Horman <horms@verge.net.au>

 arch/ia64/kernel/efi.c   |   84 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/ia64/kernel/setup.c |   10 ++++-
 include/asm-ia64/kexec.h |    2 +
 3 files changed, 91 insertions(+), 5 deletions(-)
Index: linux-2.6/arch/ia64/kernel/efi.c
=================================--- linux-2.6.orig/arch/ia64/kernel/efi.c	2007-03-07 09:39:10.000000000 +0900
+++ linux-2.6/arch/ia64/kernel/efi.c	2007-03-07 09:51:01.000000000 +0900
@@ -1160,6 +1160,84 @@
 }
 
 #ifdef CONFIG_KEXEC
+
+#define CRASHDUMP_ALIGNMENT (1 << _PAGE_SIZE_64M)
+static int __init
+kdump_region_verify_efi (unsigned long base, unsigned long size)
+{
+	void *efi_map_start, *efi_map_end, *p;
+	efi_memory_desc_t *md;
+	unsigned long efi_desc_size;
+
+	efi_map_start = __va(ia64_boot_param->efi_memmap);
+	efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
+	efi_desc_size = ia64_boot_param->efi_memdesc_size;
+
+	for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+		md = p;
+		if (base < md->phys_addr || base + size > efi_md_end(md))
+			continue;
+		if (is_memory_available(md))
+			return 1;
+		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx is "
+		       "inside the unusable efi region 0x%lx-0x%lx\n", base,
+		       base + size - 1, md->phys_addr, efi_md_end(md) - 1);
+		return 0;
+	}
+
+	printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx does not "
+	       "fit in any efi region\n", base, base + size - 1);
+	return 0;
+}
+
+
+/* find a block of memory aligned to 64M exclude reserved regions
+   rsvd_regions are sorted
+ */
+static int __init
+kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
+	       			 struct rsvd_region *rsvd_regions, int n)
+{
+	int i;
+
+	for (i = 0; i < n; i++) {
+		if (__pa(rsvd_regions[i].start) < base ||
+		    __pa(rsvd_regions[i].end) >= base + size - 1)
+			continue;
+		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
+		       "clashes with reserved region 0x%lx-0x%lx\n", base,
+		       base + size - 1, __pa(rsvd_regions[i].start),
+		       __pa(rsvd_regions[i].end));
+		return 0;
+	}
+	return 1;
+}
+
+
+/* find a block of memory aligned to 64M exclude reserved regions
+   rsvd_regions are sorted
+ */
+int __init
+kdump_region_verify (unsigned long base, unsigned long size,
+		     struct rsvd_region *rsvd_regions, int n)
+{
+	/* This isn't considered to be a failure condition,
+	 * but it isn't desireable either, so log it */
+	if (ALIGN(base, CRASHDUMP_ALIGNMENT) != base)
+		printk(KERN_WARNING "Kdump: warning: crashkernel region "
+		       "0x%lx-0x%lx is not aligned to 0x%x\n",
+		       base, base + size - 1, CRASHDUMP_ALIGNMENT);
+
+	if (!kdump_region_verify_efi(base, size))
+		return 0;
+
+	if (!kdump_region_verify_rsvd_region(base, size, rsvd_regions, n))
+		return 0;
+
+	printk(KERN_INFO "Kdump: crashkernel region verified\n");
+	return 1;
+}
+
 /* find a block of memory aligned to 64M exclude reserved regions
    rsvd_regions are sorted
  */
@@ -1169,7 +1247,6 @@
 {
   int i;
   u64 start, end;
-  u64 alignment = 1UL << _PAGE_SIZE_64M;
   void *efi_map_start, *efi_map_end, *p;
   efi_memory_desc_t *md;
   u64 efi_desc_size;
@@ -1182,13 +1259,13 @@
 	  md = p;
 	  if (!is_memory_available(md))
 		  continue;
-	  start = ALIGN(md->phys_addr, alignment);
+	  start = ALIGN(md->phys_addr, CRASHDUMP_ALIGNMENT);
 	  end = efi_md_end(md);
 	  for (i = 0; i < n; i++) {
 		if (__pa(r[i].start) >= start && __pa(r[i].end) < end) {
 			if (__pa(r[i].start) > start + size)
 				return start;
-			start = ALIGN(__pa(r[i].end), alignment);
+			start = ALIGN(__pa(r[i].end), CRASHDUMP_ALIGNMENT);
 			if (i < n-1 && __pa(r[i+1].start) < start + size)
 				continue;
 			else
Index: linux-2.6/arch/ia64/kernel/setup.c
=================================--- linux-2.6.orig/arch/ia64/kernel/setup.c	2007-03-07 09:39:10.000000000 +0900
+++ linux-2.6/arch/ia64/kernel/setup.c	2007-03-07 09:39:10.000000000 +0900
@@ -275,11 +275,17 @@
 			else
 				base = 0;
 			if (size) {
+				sort_regions(rsvd_region, n);
 				if (!base) {
-					sort_regions(rsvd_region, n);
 					base = kdump_find_rsvd_region(size,
 							      	rsvd_region, n);
-					}
+				}
+				else {
+					if (!kdump_region_verify(base, size,
+								 rsvd_region,
+								 n))
+						base = ~0UL;
+				}
 				if (base != ~0UL) {
 					rsvd_region[n].start  						(unsigned long)__va(base);
Index: linux-2.6/include/asm-ia64/kexec.h
=================================--- linux-2.6.orig/include/asm-ia64/kexec.h	2007-03-07 09:38:50.000000000 +0900
+++ linux-2.6/include/asm-ia64/kexec.h	2007-03-07 09:39:10.000000000 +0900
@@ -37,6 +37,8 @@
 extern void kexec_disable_iosapic(void);
 extern void crash_save_this_cpu(void);
 struct rsvd_region;
+extern int kdump_region_verify(unsigned long base,
+		unsigned long size, struct rsvd_region *rsvd_regions, int n);
 extern unsigned long kdump_find_rsvd_region(unsigned long size,
 		struct rsvd_region *rsvd_regions, int n);
 extern void kdump_cpu_freeze(struct unw_frame_info *info, void *arg);

RE: [patch 3/3] IA64: verify the base address of crashkernel

[Zou, Nanhai]

> -----Original Message-----
> From: Horms [mailto:horms@verge.net.au]
> Sent: 2007Äê3ÔÂ7ÈÕ 8:50
> To: Zou, Nanhai
> Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> 
> On Tue, Mar 06, 2007 at 04:23:37PM +0800, Zou, Nanhai wrote:
> >
> > Hi Horms,
> > 	I feel this is over-designed.
> >      I think to specify crash kernel base address in command line is only
> useful for debug, on platform like SN this feature is totally unusable.At the
> most of time, user should let kernel to decide where to reserve crashdump
> region.
> >     If a user wants to put crash kernel in command line, he should know what
> he is doing.
> 
> Hi Nanhai,
> 
> while I do agree that perhaps these checks are a little verbose I don't
> agree that they are uneccessary. Specifying the base address is entirely
> sane on some platforms (e.g. Tiger 2). And more to the point, it is the
> only method available on some architectures, and thus its seems
> reasonable to expect that it might work sanley on ia64. It seems to me
> that it is a good idea to have some checks in place, in line with the
> checks performed when the base address is automatically determinted to
> make the behaviour (more) consistent.
> 
> Ideally it would be good if there were not two code-paths relating
> to base address selection - auto and manual. Or more to the point, if
> they could share the same checks. But at this point I can't see a way to
> make the code do that.
> 
> I guess in the end it comes down to how easy you want it to be for users
> mistakes to be caught. I think that currently kexec/kdump is quite
> fragile and its easy to end up with a setup that doesn't work. I think
> that changes like this one are one small step towards making a more
> robust system. Ditto for the change regarding loging success or failure
> of inserting the crashkernel region.
> 
Hi,
	I think even in the situation of manually pick address, user can check /proc/iomem to found the address, it is easy.
     I don't see in which situation like kernel automatically determined method does not work but manually pick address would work. So I think manually pick address could only be help for debug. But I think it is good to have this feature since it only cost 2 lines of code.
    I believe it is good to keep the kernel code simple and clean. 
We do not need to add more than 70 lines of code to kernel only for debug print.
You can add those prints to kernel whenever you want to debug something, but put those in vanilla kernel is not a good idea.
  BTW: the code logic in your updated patch is still not correct, in kdump_region_verify_rsvd_region you do not check the partly overlap situation.

Thanks
Zou Nan hai
> --
> Horms
>   H: http://www.vergenet.net/~horms/
>   W: http://www.valinux.co.jp/en/

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

On Wed, Mar 07, 2007 at 10:15:20AM +0800, Zou, Nanhai wrote:
> > -----Original Message-----
> > From: Horms [mailto:horms@verge.net.au]
> > Sent: 2007年3月7日 8:50
> > To: Zou, Nanhai
> > Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> > Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> > 
> > On Tue, Mar 06, 2007 at 04:23:37PM +0800, Zou, Nanhai wrote:
> > >
> > > Hi Horms,
> > > 	I feel this is over-designed.
> > >      I think to specify crash kernel base address in command line is only
> > useful for debug, on platform like SN this feature is totally unusable.At the
> > most of time, user should let kernel to decide where to reserve crashdump
> > region.
> > >     If a user wants to put crash kernel in command line, he should know what
> > he is doing.
> > 
> > Hi Nanhai,
> > 
> > while I do agree that perhaps these checks are a little verbose I don't
> > agree that they are uneccessary. Specifying the base address is entirely
> > sane on some platforms (e.g. Tiger 2). And more to the point, it is the
> > only method available on some architectures, and thus its seems
> > reasonable to expect that it might work sanley on ia64. It seems to me
> > that it is a good idea to have some checks in place, in line with the
> > checks performed when the base address is automatically determinted to
> > make the behaviour (more) consistent.
> > 
> > Ideally it would be good if there were not two code-paths relating
> > to base address selection - auto and manual. Or more to the point, if
> > they could share the same checks. But at this point I can't see a way to
> > make the code do that.
> > 
> > I guess in the end it comes down to how easy you want it to be for users
> > mistakes to be caught. I think that currently kexec/kdump is quite
> > fragile and its easy to end up with a setup that doesn't work. I think
> > that changes like this one are one small step towards making a more
> > robust system. Ditto for the change regarding loging success or failure
> > of inserting the crashkernel region.
> > 
> Hi,
> 	I think even in the situation of manually pick address, user can check /proc/iomem to found the address, it is easy.
>      I don't see in which situation like kernel automatically determined method does not work but manually pick address would work. So I think manually pick address could only be help for debug. But I think it is good to have this feature since it only cost 2 lines of code.
>     I believe it is good to keep the kernel code simple and clean. 
> We do not need to add more than 70 lines of code to kernel only for debug print.
> You can add those prints to kernel whenever you want to debug something, but put those in vanilla kernel is not a good idea.
>   BTW: the code logic in your updated patch is still not correct, in kdump_region_verify_rsvd_region you do not check the partly overlap situation.

I think that the manual option is also important because it maintains
feature-compatibility with other architectures. I don't consider it
a hack that might work purely for the purposes of debugging.

With regards to 70 lines of extra code, it can probably be consolidated
a bit - for insance I think that the ckeck contained in
kdump_region_verify_rsvd_region() is more important than the other
checks which I guess could be disposed of if the length of the code
really is a problem. But in any case the new code is almost entirely in
__init. So I don't really see that it is a big concern.

As for the partly-overlaped case in kdump_region_verify_rsvd_region(). 
I'm not entirely sure what you are getting at there. Are you talking
about an optimisation to the check, or a correctness problem?

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

RE: [patch 3/3] IA64: verify the base address of crashkernel

[Zou, Nanhai]

> -----Original Message-----
> From: Horms [mailto:horms@verge.net.au]
> Sent: 2007年3月7日 11:46
> To: Zou, Nanhai
> Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> 
> On Wed, Mar 07, 2007 at 10:15:20AM +0800, Zou, Nanhai wrote:
> > > -----Original Message-----
> > > From: Horms [mailto:horms@verge.net.au]
> > > Sent: 2007年3月7日 8:50
> > > To: Zou, Nanhai
> > > Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> > > Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> > >
> > > On Tue, Mar 06, 2007 at 04:23:37PM +0800, Zou, Nanhai wrote:
> > > >
> > > > Hi Horms,
> > > > 	I feel this is over-designed.
> > > >      I think to specify crash kernel base address in command line is only
> > > useful for debug, on platform like SN this feature is totally unusable.At
> the
> > > most of time, user should let kernel to decide where to reserve crashdump
> > > region.
> > > >     If a user wants to put crash kernel in command line, he should know
> what
> > > he is doing.
> > >
> > > Hi Nanhai,
> > >
> > > while I do agree that perhaps these checks are a little verbose I don't
> > > agree that they are uneccessary. Specifying the base address is entirely
> > > sane on some platforms (e.g. Tiger 2). And more to the point, it is the
> > > only method available on some architectures, and thus its seems
> > > reasonable to expect that it might work sanley on ia64. It seems to me
> > > that it is a good idea to have some checks in place, in line with the
> > > checks performed when the base address is automatically determinted to
> > > make the behaviour (more) consistent.
> > >
> > > Ideally it would be good if there were not two code-paths relating
> > > to base address selection - auto and manual. Or more to the point, if
> > > they could share the same checks. But at this point I can't see a way to
> > > make the code do that.
> > >
> > > I guess in the end it comes down to how easy you want it to be for users
> > > mistakes to be caught. I think that currently kexec/kdump is quite
> > > fragile and its easy to end up with a setup that doesn't work. I think
> > > that changes like this one are one small step towards making a more
> > > robust system. Ditto for the change regarding loging success or failure
> > > of inserting the crashkernel region.
> > >
> > Hi,
> > 	I think even in the situation of manually pick address, user can check
> /proc/iomem to found the address, it is easy.
> >      I don't see in which situation like kernel automatically determined
> method does not work but manually pick address would work. So I think manually
> pick address could only be help for debug. But I think it is good to have this
> feature since it only cost 2 lines of code.
> >     I believe it is good to keep the kernel code simple and clean.
> > We do not need to add more than 70 lines of code to kernel only for debug
> print.
> > You can add those prints to kernel whenever you want to debug something, but
> put those in vanilla kernel is not a good idea.
> >   BTW: the code logic in your updated patch is still not correct, in
> kdump_region_verify_rsvd_region you do not check the partly overlap situation.
> 
> I think that the manual option is also important because it maintains
> feature-compatibility with other architectures. I don't consider it
> a hack that might work purely for the purposes of debugging.
> 
  I don't understand why we need to maintain compatibility with other architectures here. Manfully choose may confuse user, XXX@16M may work on one arch,but not on another arch. Other architectures need manually choose crash kernel region simply because they do not support kernel automatically choose feature. 

  I keep the XXX@YYY format to just make kdump script compatible, do that distributions does not need to maintain different kdump scripts for different arches. 

> With regards to 70 lines of extra code, it can probably be consolidated
> a bit - for insance I think that the ckeck contained in
> kdump_region_verify_rsvd_region() is more important than the other
> checks which I guess could be disposed of if the length of the code
> really is a problem. But in any case the new code is almost entirely in
> __init. So I don't really see that it is a big concern.
> 
> As for the partly-overlaped case in kdump_region_verify_rsvd_region().
> I'm not entirely sure what you are getting at there. Are you talking
> about an optimisation to the check, or a correctness problem?
> 
 (reserve_region.start < base && reserve_region.end < base + size)
  or
 (reserve_region.end > base && reserve_region.end > base + size) will pass the check

  Zou Nan hai

> --
> Horms
>   H: http://www.vergenet.net/~horms/
>   W: http://www.valinux.co.jp/en/
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

On Wed, Mar 07, 2007 at 12:50:12PM +0800, Zou, Nanhai wrote:
> On Wed, Mar 07, 2007 at 11:46, Horms wrote:
> > 
> > I think that the manual option is also important because it
> > maintains feature-compatibility with other architectures. I don't
> > consider it a hack that might work purely for the purposes of
> > debugging.
> 
> I don't understand why we need to maintain compatibility with other
> architectures here. Manfully choose may confuse user, XXX@16M may work
> on one arch,but not on another arch. Other architectures need manually
> choose crash kernel region simply because they do not support kernel
> automatically choose feature. 
>
> I keep the XXX@YYY format to just make kdump script compatible, do
> that distributions does not need to maintain different kdump scripts
> for different arches. 

From my point of view, what you say in the paragraph immediately above
is the crux of the point. There is a case for compatibility.  And
furthermore, that compatibility really ought to be correct rather than a
bit of a hack. (Or in this case a bit more of a hack than the auto
select code path.)

> > With regards to 70 lines of extra code, it can probably be consolidated
> > a bit - for insance I think that the ckeck contained in
> > kdump_region_verify_rsvd_region() is more important than the other
> > checks which I guess could be disposed of if the length of the code
> > really is a problem. But in any case the new code is almost entirely in
> > __init. So I don't really see that it is a big concern.
> > 
> > As for the partly-overlaped case in kdump_region_verify_rsvd_region().
> > I'm not entirely sure what you are getting at there. Are you talking
> > about an optimisation to the check, or a correctness problem?
> > 
>  (reserve_region.start < base && reserve_region.end < base + size)
>   or
>  (reserve_region.end > base && reserve_region.end > base + size) will pass the check

Thanks, is this logic better?

kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
                                struct rsvd_region *rsvd_regions, int n)
{
	int i;

	for (i = 0; i < n; i++) {
		/* Assume that start < end && size > 0 */
		if (__pa(rsvd_regions[i].start) >= base + size &&
		    __pa(rsvd_regions[i].end) < base)
			continue;
		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
		       "clashes with reserved region 0x%lx-0x%lx\n", base,
		       base + size - 1, __pa(rsvd_regions[i].start),
		       __pa(rsvd_regions[i].end));
		return 0;
	}
	return 1;
}

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

RE: [patch 3/3] IA64: verify the base address of crashkernel

[Zou, Nanhai]

> -----Original Message-----
> From: Horms [mailto:horms@verge.net.au]
> Sent: 2007Äê3ÔÂ7ÈÕ 15:55
> To: Zou, Nanhai
> Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> 
> On Wed, Mar 07, 2007 at 12:50:12PM +0800, Zou, Nanhai wrote:
> > On Wed, Mar 07, 2007 at 11:46, Horms wrote:
> > >
> > > I think that the manual option is also important because it
> > > maintains feature-compatibility with other architectures. I don't
> > > consider it a hack that might work purely for the purposes of
> > > debugging.
> >
> > I don't understand why we need to maintain compatibility with other
> > architectures here. Manfully choose may confuse user, XXX@16M may work
> > on one arch,but not on another arch. Other architectures need manually
> > choose crash kernel region simply because they do not support kernel
> > automatically choose feature.
> >
> > I keep the XXX@YYY format to just make kdump script compatible, do
> > that distributions does not need to maintain different kdump scripts
> > for different arches.
> 

> From my point of view, what you say in the paragraph immediately above
> Thanks, is this logic better?
> 
  Don't write code to improve a "no user will use" feature. 
  Let's keep kernel code clean.

> kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
>                                 struct rsvd_region *rsvd_regions, int n)
> {
> 	int i;
> 
> 	for (i = 0; i < n; i++) {
> 		/* Assume that start < end && size > 0 */
> 		if (__pa(rsvd_regions[i].start) >= base + size &&
> 		    __pa(rsvd_regions[i].end) < base)
> 			continue;
    This is much worse. Have you ever tested it?

> 		printk(KERN_WARNING "Kdump: crashkernel region 0x%lx-0x%lx "
> 		       "clashes with reserved region 0x%lx-0x%lx\n", base,
> 		       base + size - 1, __pa(rsvd_regions[i].start),
> 		       __pa(rsvd_regions[i].end));
> 		return 0;
> 	}
> 	return 1;
> }


> 
Zou Nan hai
> --
> Horms
>   H: http://www.vergenet.net/~horms/
>   W: http://www.valinux.co.jp/en/

Re: [patch 3/3] IA64: verify the base address of crashkernel

[Horms]

On Wed, Mar 07, 2007 at 05:06:39PM +0800, Zou, Nanhai wrote:
> > -----Original Message-----
> > From: Horms [mailto:horms@verge.net.au]
> > Sent: 2007年3月7日 15:55
> > To: Zou, Nanhai
> > Cc: Linux-IA64; fastboot@lists.osdl.org; Luck, Tony; Magnus Damm
> > Subject: Re: [patch 3/3] IA64: verify the base address of crashkernel
> > 
> > On Wed, Mar 07, 2007 at 12:50:12PM +0800, Zou, Nanhai wrote:
> > > On Wed, Mar 07, 2007 at 11:46, Horms wrote:
> > > >
> > > > I think that the manual option is also important because it
> > > > maintains feature-compatibility with other architectures. I don't
> > > > consider it a hack that might work purely for the purposes of
> > > > debugging.
> > >
> > > I don't understand why we need to maintain compatibility with other
> > > architectures here. Manfully choose may confuse user, XXX@16M may work
> > > on one arch,but not on another arch. Other architectures need manually
> > > choose crash kernel region simply because they do not support kernel
> > > automatically choose feature.
> > >
> > > I keep the XXX@YYY format to just make kdump script compatible, do
> > > that distributions does not need to maintain different kdump scripts
> > > for different arches.
> > 
> 
> > From my point of view, what you say in the paragraph immediately above
> > Thanks, is this logic better?
> > 
>   Don't write code to improve a "no user will use" feature. 
>   Let's keep kernel code clean.

I think we could argue about this forever :-)

> > kdump_region_verify_rsvd_region (unsigned long base, unsigned long size,
> >                                 struct rsvd_region *rsvd_regions, int n)
> > {
> > 	int i;
> > 
> > 	for (i = 0; i < n; i++) {
> > 		/* Assume that start < end && size > 0 */
> > 		if (__pa(rsvd_regions[i].start) >= base + size &&
> > 		    __pa(rsvd_regions[i].end) < base)
> > 			continue;
>     This is much worse. Have you ever tested it?

Sorry, I wrote it down wrong :(
That should have been:

	if (__pa(rsvd_regions[i].start) >= base + size ||
	    __pa(rsvd_regions[i].end) <= base)
		continue;

I'll do some testing tomorrow. But this was what I was thinking about:

s=__pa(rsvd_regions[i].start)
e=__pa(rsvd_regions[i].end)

                  base             base+size
OK:    s:bad  e:ok |                  |
BAD:   s:bad       |            e:bad |
BAD:   s:bad       |                  |         e:bad
BAD:   s:bad       |            e:bad |
BAD:               |  s:bad     e:bad | 
BAD:               |  s:bad           |         e:bad
OK:                |                  |  s:ok   e:bad

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/