Initialize phys_start during early Xen setup

With early Xen setup, we can move the initialization of xen_phys_start to the setup routine. This is cleaner, as we can get rid of ifdef'ed arch-specific code in makedumpfile.c (which should IMO stay generic).

Signed-off-by Petr Tesarik <[email protected]>

---
 arch/x86.c     |    9 +++++++++
 arch/x86_64.c  |    9 +++++++++
 makedumpfile.c |   16 ----------------
 3 files changed, 18 insertions(+), 16 deletions(-)

arch/x86.c +0 -0

@@ -296,6 +296,15 @@ int get_xen_basic_info_x86(void)
 	unsigned long frame_table_vaddr;
 	unsigned long xen_end;

arch/x86.c +0 -0

 	if (SYMBOL(pgd_l2) == NOT_FOUND_SYMBOL &&
 	    SYMBOL(pgd_l3) == NOT_FOUND_SYMBOL) {
 		ERRMSG("Can't get pgd.\n");

arch/x86_64.c +9 -0

@@ -361,6 +361,15 @@ int get_xen_basic_info_x86_64(void)
 	unsigned long frame_table_vaddr;
 	unsigned long xen_end;
 
+ 	if (!info->xen_phys_start) {
+		if (info->xen_crash_info_v < 2) {
+			ERRMSG("Can't get Xen physical start address.\n"
+			       "Please use the --xen_phys_start option.");
+			return FALSE;
+		}
+		info->xen_phys_start = info->xen_crash_info.v2->xen_phys_start;
+	}
+
 	if (SYMBOL(pgd_l4) == NOT_FOUND_SYMBOL) {
 		ERRMSG("Can't get pml4.\n");
 		return FALSE;

makedumpfile.c +0 -16

@@ -5365,20 +5365,6 @@ init_xen_crash_info(void)
 }
 
 int
-get_xen_phys_start(void)
-{
-	if (info->xen_phys_start)
-		return TRUE;
-
-#if defined(__x86__) || defined(__x86_64__)
-	if (info->xen_crash_info_v >= 2)
-		info->xen_phys_start = info->xen_crash_info.v2->xen_phys_start;
-#endif
-
-	return TRUE;
-}
-
-int
 get_xen_info(void)
 {
 	unsigned long domain;

@@ -5877,8 +5863,6 @@ initial_xen(void)
 		if (!read_vmcoreinfo_from_vmcore(offset, size, TRUE))
 			return FALSE;
 	}
-	if (!get_xen_phys_start())
-		return FALSE;
 	if (!get_xen_info())
 		return FALSE;
 

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Hello Petr,

Quoted 17 lines · On Fri, 24 Aug 2012 17:42:24 +0200 Petr Tesarik <[email protected]> wrote:

With early Xen setup, we can move the initialization of xen_phys_start to the setup routine. This is cleaner, as we can get rid of ifdef'ed arch-specific code in makedumpfile.c (which should IMO stay generic).

Signed-off-by Petr Tesarik <[email protected]>

---
 arch/x86.c     |    9 +++++++++
 arch/x86_64.c  |    9 +++++++++
 makedumpfile.c |   16 ----------------
 3 files changed, 18 insertions(+), 16 deletions(-)

arch/x86.c +0 -0

@@ -296,6 +296,15 @@ int get_xen_basic_info_x86(void)
 	unsigned long frame_table_vaddr;
 	unsigned long xen_end;

arch/x86.c int get_xen_basic_info_x86(void) · line 307

Quoted 10 lines

arch/x86.c +9 -0

 
+	if (!info->xen_phys_start) {
+		if (info->xen_crash_info_v < 2) {
+			ERRMSG("Can't get Xen physical start address.\n"
+			       "Please use the --xen_phys_start option.");
+			return FALSE;
+		}
+		info->xen_phys_start = info->xen_crash_info.v2->xen_phys_start;
+	}
+

I tested for the vmcore which doesn't include xen_phys_start, then the error message below showed:

  $ makedumpfile_v1.5.1 -E -X --xen-syms xen-syms-2.6.18-92.el5.debug vmcore dumpfile.EX
  get_xen_basic_info_x86: Can't get Xen physical start address.
  Please use the --xen_phys_start option.
  makedumpfile Failed.
  $

So, should I understand this result as reasonable error handling ?

The reason why I have this question is because the same vmcore can be filtered out with makedumpfile-1.5.0 or before like below:

  $ makedumpfile_v1.5.0 -E -X --xen-syms xen-syms-2.6.18-92.el5.debug vmcore dumpfile.EX
  Switched running mode from cyclic to non-cyclic,
  because the cyclic mode doesn't support Xen.
  Copying data                       : [100 %]

The dumpfile is saved to dumpfile.EX.

makedumpfile Completed.
$

I have checked with grep, it seems that xen_phys_start is used for x86_64 only. So, is the valid check really necessary for x86 ?

Thanks
Atsushi Kumagai

Quoted 56 lines

arch/x86.c +0 -0

 	if (SYMBOL(pgd_l2) == NOT_FOUND_SYMBOL &&
 	    SYMBOL(pgd_l3) == NOT_FOUND_SYMBOL) {
 		ERRMSG("Can't get pgd.\n");

arch/x86_64.c +9 -0

@@ -361,6 +361,15 @@ int get_xen_basic_info_x86_64(void)
 	unsigned long frame_table_vaddr;
 	unsigned long xen_end;
 
+ 	if (!info->xen_phys_start) {
+		if (info->xen_crash_info_v < 2) {
+			ERRMSG("Can't get Xen physical start address.\n"
+			       "Please use the --xen_phys_start option.");
+			return FALSE;
+		}
+		info->xen_phys_start = info->xen_crash_info.v2->xen_phys_start;
+	}
+
 	if (SYMBOL(pgd_l4) == NOT_FOUND_SYMBOL) {
 		ERRMSG("Can't get pml4.\n");
 		return FALSE;

makedumpfile.c +0 -16

@@ -5365,20 +5365,6 @@ init_xen_crash_info(void)
 }
 
 int
-get_xen_phys_start(void)
-{
-	if (info->xen_phys_start)
-		return TRUE;
-
-#if defined(__x86__) || defined(__x86_64__)
-	if (info->xen_crash_info_v >= 2)
-		info->xen_phys_start = info->xen_crash_info.v2->xen_phys_start;
-#endif
-
-	return TRUE;
-}
-
-int
 get_xen_info(void)
 {
 	unsigned long domain;

@@ -5877,8 +5863,6 @@ initial_xen(void)
 		if (!read_vmcoreinfo_from_vmcore(offset, size, TRUE))
 			return FALSE;
 	}
-	if (!get_xen_phys_start())
-		return FALSE;
 	if (!get_xen_info())
 		return FALSE;
 

Quoted 4 lines

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Hello Petr,

Quoted 56 lines · On Wed, 28 Nov 2012 14:56:58 +0900 Atsushi Kumagai…

Hello Petr,

Quoted 29 lines · On Fri, 24 Aug 2012 17:42:24 +0200 Petr Tesarik <[email protected]> wrote:

With early Xen setup, we can move the initialization of xen_phys_start to the setup routine. This is cleaner, as we can get rid of ifdef'ed arch-specific code in makedumpfile.c (which should IMO stay generic).

Signed-off-by Petr Tesarik <[email protected]>

---
 arch/x86.c     |    9 +++++++++
 arch/x86_64.c  |    9 +++++++++
 makedumpfile.c |   16 ----------------
 3 files changed, 18 insertions(+), 16 deletions(-)

arch/x86.c +3 -0

@@ -296,6 +296,15 @@ int get_xen_basic_info_x86(void)
 	unsigned long frame_table_vaddr;
 	unsigned long xen_end;
 
+	if (!info->xen_phys_start) {
+		if (info->xen_crash_info_v < 2) {
+			ERRMSG("Can't get Xen physical start

address.\n"

arch/x86.c +1 -0

+			       "Please use the --xen_phys_start

option.");

arch/x86.c +3 -0

+			return FALSE;
+		}
+		info->xen_phys_start =

info->xen_crash_info.v2->xen_phys_start;

arch/x86.c

Quoted 2 lines

arch/x86.c +2 -0

+	}
+

I tested for the vmcore which doesn't include xen_phys_start, then the error message below showed:

  $ makedumpfile_v1.5.1 -E -X --xen-syms
xen-syms-2.6.18-92.el5.debug vmcore dumpfile.EX
get_xen_basic_info_x86: Can't get Xen physical start address.
Please use the --xen_phys_start option. makedumpfile Failed.
  $

So, should I understand this result as reasonable error handling ?

The reason why I have this question is because the same vmcore can be filtered out with makedumpfile-1.5.0 or before like below:

  $ makedumpfile_v1.5.0 -E -X --xen-syms
xen-syms-2.6.18-92.el5.debug vmcore dumpfile.EX Switched running
mode from cyclic to non-cyclic, because the cyclic mode doesn't
support Xen. Copying data                       : [100 %]

Quoted 7 lines

The dumpfile is saved to dumpfile.EX.

makedumpfile Completed.
$

I have checked with grep, it seems that xen_phys_start is used for x86_64 only. So, is the valid check really necessary for x86 ?

I don't know much about Xen, but I think this is just regression. And I want to release v1.5.1 in this week, so I will fix this regression with the patch below:

arch/x86.c +0 -3

index 7de0495..ef29e3c 100644
@@ -294,15 +294,6 @@ kvtop_xen_x86(unsigned long kvaddr)
 
 int get_xen_basic_info_x86(void)
 {
-       if (!info->xen_phys_start) {
-               if (info->xen_crash_info_v < 2) {
-                       ERRMSG("Can't get Xen physical start

address.\n"
- "Please use the --xen_phys_start
option.");

arch/x86.c +0 -3

-                       return FALSE;
-               }
-               info->xen_phys_start =

info->xen_crash_info.v2->xen_phys_start;

arch/x86.c +0 -2

-       }
-
        if (SYMBOL(pgd_l2) == NOT_FOUND_SYMBOL &&
            SYMBOL(pgd_l3) == NOT_FOUND_SYMBOL) {
                ERRMSG("Can't get pgd.\n");

Please let me know if you have any objections.

Quoted 29 lines

I don't know much about Xen, but I think this is just regression. And I want to release v1.5.1 in this week, so I will fix this regression with the patch below:

arch/x86.c +0 -3

index 7de0495..ef29e3c 100644
@@ -294,15 +294,6 @@ kvtop_xen_x86(unsigned long kvaddr)
 
 int get_xen_basic_info_x86(void)
 {
-       if (!info->xen_phys_start) {
-               if (info->xen_crash_info_v < 2) {
-                       ERRMSG("Can't get Xen physical start

address.\n"
- "Please use the --xen_phys_start
option.");

arch/x86.c +0 -3

-                       return FALSE;
-               }
-               info->xen_phys_start =

info->xen_crash_info.v2->xen_phys_start;

arch/x86.c +0 -2

-       }
-
        if (SYMBOL(pgd_l2) == NOT_FOUND_SYMBOL &&
            SYMBOL(pgd_l3) == NOT_FOUND_SYMBOL) {
                ERRMSG("Can't get pgd.\n");

Quoted 1 line

Please let me know if you have any objections.

Hello Atsushi-san,

I'm sorry for my late reaction. Yes, this piece should be removed. While there is a xen_phys_start variable in 32-bit Xen, it is always zero, so we shouldn't complain if we can't get it.

Petr Tesarik