[PATCH 0 of 3] KEXEC: Introduce new crashtables interface (v2)

[PATCH 0 of 3] KEXEC: Introduce new crashtables interface (v2)

[Andrew Cooper]

This patch series implement changes to the kexec system in Xen, both along the
lines of supporting a 32bit crash kernel with 64bit Xen, and providing more
useful information to the crash kernel.

Most of these patches have been submitted upstream in the past; The main new one
is patch 4 which introduces the crashtables mechanism of passing information to
the crashdump kernel using ELF CORE notes.  The set of patches (ported to
xen-4.1) have been tested against a wide range of hardware.

Changes since v1:
 *  Drop the top-of-mem patch.  It was convaing information very similar
    information to other memory variables, in a less sensible way.  Therefore,
    use max_page in preference.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 1 of 3] KEXEC: Allocate crash notes on boot

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]

Currently, the buffers for crash notes are allocated per CPU when a
KEXEC_CMD_kexec_get_range hypercall is made, referencing the CPU in question.
This has certain problems including not being able to allocate the crash buffers
if the host is out of memory when crashing.

In addition, my forthcoming code to support 32bit kdump kernels on 64bit Xen on
large (>64GB) boxes will require some guarentees as to where the crash note
buffers are actually allocated in physical memory.  This is far easier to sort
out at boot time, rather than after dom0 has been booted and potentially using
the physical memory required.

Therefore, allocate the crash note buffers at boot time.

Changes since v6:
 *  Tweak kexec_init() to use xzmalloc_array(), and to defer registering the
    crashdump keyhandler until the crash notes have been successfully
    allocated.

Changes since v5:
 *  Introduce sizeof_cpu_notes to move calculation of note size into a
    separate location.
 *  Tweak sizeof_note() to return size_t rather than int, as it is a
    function based upon sizeof().

Changes since v4:

 *  Replace the current cpu crash note scheme of using void pointers
    and hand calculating the size each time is needed, by a range
    structure containing a pointer and a size.  This removes duplicate
    times where the size is calculated.
 *  Tweak kexec_get_cpu().  Don't fail if a cpu is offline because it
    may already have crash notes, and may be up by the time a crash
    happens.  Split the error conditions up to return ERANGE for an
    out-of-range cpu request rather than EINVAL.  Finally, returning a
    range of zeros is acceptable, so do this in preference to failing.

Changes since v3:

 *  Alter the spinlocks to avoid calling xmalloc/xfree while holding
    the lock.
 *  Tidy up the coding style used.

Changes since v2:

 *  Allocate crash_notes dynamically using nr_cpu_ids at boot time,
    rather than statically using NR_CPUS.
 *  Fix the incorrect use of signed integers for cpu id.
 *  Fix collateral damage to do_crashdump_trigger() and
    crashdump_trigger_handler caused by reordering sizeof_note() and
    setup_note()
 *  Tweak the issue about returing -ENOMEM from kexec_init_cpu_note().
    No functional change.
 *  Change kexec_get_cpu() to attempt to allocate crash note buffers
    in case we have more free memory now than when the pcpu came up.
 *  Now that there are two codepaths possibly allocating crash notes,
    protect the allocation itself with a spinlock.

Changes since v1:

 *  Use cpu hotplug notifiers to handle allocating of the notes
    buffers rather than assuming the boot state of cpus will be the
    same as the crash state.
 *  Move crash_notes from being per_cpu.  This is because the kdump
    kernel elf binary put in the crash area is hard coded to physical
    addresses which the dom0 kernel typically obtains at boot time.
    If a cpu is offlined, its buffer should not be deallocated because
    the kdump kernel would read junk when trying to get the crash
    notes.  Similarly, the same problem would occur if the cpu was
    re-onlined later and its crash notes buffer was allocated elsewhere.
 *  Only attempt to allocate buffers if a crash area has been
    specified.  Else, allocating crash note buffers is a waste of
    space.  Along with this, change the test in kexec_get_cpu to
    return -EINVAL if no buffers have been allocated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>



[-- Attachment #2: KEXEC-setup-crash-notes-during-boot.patch --]
[-- Type: text/x-patch, Size: 11653 bytes --]

Currently, the buffers for crash notes are allocated per CPU when a
KEXEC_CMD_kexec_get_range hypercall is made, referencing the CPU in question.
This has certain problems including not being able to allocate the crash buffers
if the host is out of memory when crashing.

In addition, my forthcoming code to support 32bit kdump kernels on 64bit Xen on
large (>64GB) boxes will require some guarentees as to where the crash note
buffers are actually allocated in physical memory.  This is far easier to sort
out at boot time, rather than after dom0 has been booted and potentially using
the physical memory required.

Therefore, allocate the crash note buffers at boot time.

Changes since v6:
 *  Tweak kexec_init() to use xzmalloc_array(), and to defer registering the
    crashdump keyhandler until the crash notes have been successfully
    allocated.

Changes since v5:
 *  Introduce sizeof_cpu_notes to move calculation of note size into a
    separate location.
 *  Tweak sizeof_note() to return size_t rather than int, as it is a
    function based upon sizeof().

Changes since v4:

 *  Replace the current cpu crash note scheme of using void pointers
    and hand calculating the size each time is needed, by a range
    structure containing a pointer and a size.  This removes duplicate
    times where the size is calculated.
 *  Tweak kexec_get_cpu().  Don't fail if a cpu is offline because it
    may already have crash notes, and may be up by the time a crash
    happens.  Split the error conditions up to return ERANGE for an
    out-of-range cpu request rather than EINVAL.  Finally, returning a
    range of zeros is acceptable, so do this in preference to failing.

Changes since v3:

 *  Alter the spinlocks to avoid calling xmalloc/xfree while holding
    the lock.
 *  Tidy up the coding style used.

Changes since v2:

 *  Allocate crash_notes dynamically using nr_cpu_ids at boot time,
    rather than statically using NR_CPUS.
 *  Fix the incorrect use of signed integers for cpu id.
 *  Fix collateral damage to do_crashdump_trigger() and
    crashdump_trigger_handler caused by reordering sizeof_note() and
    setup_note()
 *  Tweak the issue about returing -ENOMEM from kexec_init_cpu_note().
    No functional change.
 *  Change kexec_get_cpu() to attempt to allocate crash note buffers
    in case we have more free memory now than when the pcpu came up.
 *  Now that there are two codepaths possibly allocating crash notes,
    protect the allocation itself with a spinlock.

Changes since v1:

 *  Use cpu hotplug notifiers to handle allocating of the notes
    buffers rather than assuming the boot state of cpus will be the
    same as the crash state.
 *  Move crash_notes from being per_cpu.  This is because the kdump
    kernel elf binary put in the crash area is hard coded to physical
    addresses which the dom0 kernel typically obtains at boot time.
    If a cpu is offlined, its buffer should not be deallocated because
    the kdump kernel would read junk when trying to get the crash
    notes.  Similarly, the same problem would occur if the cpu was
    re-onlined later and its crash notes buffer was allocated elsewhere.
 *  Only attempt to allocate buffers if a crash area has been
    specified.  Else, allocating crash note buffers is a waste of
    space.  Along with this, change the test in kexec_get_cpu to
    return -EINVAL if no buffers have been allocated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r f61120046915 -r 1b938f3fbad5 xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -25,13 +25,19 @@
 #include <xen/kexec.h>
 #include <public/elfnote.h>
 #include <xsm/xsm.h>
+#include <xen/cpu.h>
 #ifdef CONFIG_COMPAT
 #include <compat/kexec.h>
 #endif
 
 bool_t kexecing = FALSE;
 
-static DEFINE_PER_CPU_READ_MOSTLY(void *, crash_notes);
+/* Memory regions to store the per cpu register state etc. on a crash. */
+typedef struct { Elf_Note * start; size_t size; } crash_note_range_t;
+static crash_note_range_t * crash_notes;
+
+/* Lock to prevent race conditions when allocating the crash note buffers. */
+static DEFINE_SPINLOCK(crash_notes_lock);
 
 static Elf_Note *xen_crash_note;
 
@@ -165,13 +171,17 @@ static void one_cpu_only(void)
 void kexec_crash_save_cpu(void)
 {
     int cpu = smp_processor_id();
-    Elf_Note *note = per_cpu(crash_notes, cpu);
+    Elf_Note *note;
     ELF_Prstatus *prstatus;
     crash_xen_core_t *xencore;
 
+    BUG_ON ( ! crash_notes );
+
     if ( cpumask_test_and_set_cpu(cpu, &crash_saved_cpus) )
         return;
 
+    note = crash_notes[cpu].start;
+
     prstatus = (ELF_Prstatus *)ELFNOTE_DESC(note);
 
     note = ELFNOTE_NEXT(note);
@@ -257,13 +267,6 @@ static struct keyhandler crashdump_trigg
     .desc = "trigger a crashdump"
 };
 
-static __init int register_crashdump_trigger(void)
-{
-    register_keyhandler('C', &crashdump_trigger_keyhandler);
-    return 0;
-}
-__initcall(register_crashdump_trigger);
-
 static void setup_note(Elf_Note *n, const char *name, int type, int descsz)
 {
     int l = strlen(name) + 1;
@@ -273,13 +276,142 @@ static void setup_note(Elf_Note *n, cons
     n->type = type;
 }
 
-static int sizeof_note(const char *name, int descsz)
+static size_t sizeof_note(const char *name, int descsz)
 {
     return (sizeof(Elf_Note) +
             ELFNOTE_ALIGN(strlen(name)+1) +
             ELFNOTE_ALIGN(descsz));
 }
 
+static size_t sizeof_cpu_notes(const unsigned long cpu)
+{
+    /* All CPUs present a PRSTATUS and crash_xen_core note. */
+    size_t bytes =
+        + sizeof_note("CORE", sizeof(ELF_Prstatus)) +
+        + sizeof_note("Xen", sizeof(crash_xen_core_t));
+
+    /* CPU0 also presents the crash_xen_info note. */
+    if ( ! cpu )
+        bytes = bytes +
+            sizeof_note("Xen", sizeof(crash_xen_info_t));
+
+    return bytes;
+}
+
+/* Allocate a crash note buffer for a newly onlined cpu. */
+static int kexec_init_cpu_notes(const unsigned long cpu)
+{
+    Elf_Note * note = NULL;
+    int ret = 0;
+    int nr_bytes = 0;
+
+    BUG_ON( cpu >= nr_cpu_ids || ! crash_notes );
+
+    /* If already allocated, nothing to do. */
+    if ( crash_notes[cpu].start )
+        return ret;
+
+    nr_bytes = sizeof_cpu_notes(cpu);
+    note = xmalloc_bytes(nr_bytes);
+
+    /* Protect the write into crash_notes[] with a spinlock, as this function
+     * is on a hotplug path and a hypercall path. */
+    spin_lock(&crash_notes_lock);
+
+    /* If we are racing with another CPU and it has beaten us, give up
+     * gracefully. */
+    if ( crash_notes[cpu].start )
+    {
+        spin_unlock(&crash_notes_lock);
+        /* Always return ok, because whether we successfully allocated or not,
+         * another CPU has successfully allocated. */
+        if ( note )
+            xfree(note);
+    }
+    else
+    {
+        crash_notes[cpu].start = note;
+        crash_notes[cpu].size = nr_bytes;
+        spin_unlock(&crash_notes_lock);
+
+        /* If the allocation failed, and another CPU did not beat us, give
+         * up with ENOMEM. */
+        if ( ! note )
+            ret = -ENOMEM;
+        /* else all is good so lets set up the notes. */
+        else
+        {
+            /* Set up CORE note. */
+            setup_note(note, "CORE", NT_PRSTATUS, sizeof(ELF_Prstatus));
+            note = ELFNOTE_NEXT(note);
+
+            /* Set up Xen CORE note. */
+            setup_note(note, "Xen", XEN_ELFNOTE_CRASH_REGS,
+                       sizeof(crash_xen_core_t));
+
+            if ( ! cpu )
+            {
+                /* Set up Xen Crash Info note. */
+                xen_crash_note = note = ELFNOTE_NEXT(note);
+                setup_note(note, "Xen", XEN_ELFNOTE_CRASH_INFO,
+                           sizeof(crash_xen_info_t));
+            }
+        }
+    }
+
+    return ret;
+}
+
+static int cpu_callback(
+    struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+    unsigned long cpu = (unsigned long)hcpu;
+
+    /* Only hook on CPU_UP_PREPARE because once a crash_note has been reported
+     * to dom0, it must keep it around in case of a crash, as the crash kernel
+     * will be hard coded to the original physical address reported. */
+    switch ( action )
+    {
+    case CPU_UP_PREPARE:
+        /* Ignore return value.  If this boot time, -ENOMEM will cause all
+         * manner of problems elsewhere very soon, and if it is during runtime,
+         * then failing to allocate crash notes is not a good enough reason to
+         * fail the CPU_UP_PREPARE */
+        kexec_init_cpu_notes(cpu);
+        break;
+    default:
+        break;
+    }
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_nfb = {
+    .notifier_call = cpu_callback
+};
+
+static int __init kexec_init(void)
+{
+    void *cpu = (void *)(unsigned long)smp_processor_id();
+
+    /* If no crash area, no need to allocate space for notes. */
+    if ( !kexec_crash_area.size )
+        return 0;
+
+    crash_notes = xzalloc_array(crash_note_range_t, nr_cpu_ids);
+    if ( ! crash_notes )
+        return -ENOMEM;
+
+    register_keyhandler('C', &crashdump_trigger_keyhandler);
+
+    cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);
+    register_cpu_notifier(&cpu_nfb);
+    return 0;
+}
+/* The reason for this to be a presmp_initcall as opposed to a regular
+ * __initcall is to allow the setup of the cpu hotplug handler before APs are
+ * brought up. */
+presmp_initcall(kexec_init);
+
 static int kexec_get_reserve(xen_kexec_range_t *range)
 {
     if ( kexec_crash_area.size > 0 && kexec_crash_area.start > 0) {
@@ -294,44 +426,29 @@ static int kexec_get_reserve(xen_kexec_r
 static int kexec_get_cpu(xen_kexec_range_t *range)
 {
     int nr = range->nr;
-    int nr_bytes = 0;
 
-    if ( nr < 0 || nr >= nr_cpu_ids || !cpu_online(nr) )
+    if ( nr < 0 || nr >= nr_cpu_ids )
+        return -ERANGE;
+
+    if ( ! crash_notes )
         return -EINVAL;
 
-    nr_bytes += sizeof_note("CORE", sizeof(ELF_Prstatus));
-    nr_bytes += sizeof_note("Xen", sizeof(crash_xen_core_t));
+    /* Try once again to allocate room for the crash notes.  It is just possible
+     * that more space has become available since we last tried.  If space has
+     * already been allocated, kexec_init_cpu_notes() will return early with 0.
+     */
+    kexec_init_cpu_notes(nr);
 
-    /* The Xen info note is included in CPU0's range. */
-    if ( nr == 0 )
-        nr_bytes += sizeof_note("Xen", sizeof(crash_xen_info_t));
+    /* In the case of still not having enough memory to allocate buffer room,
+     * returning a range of 0,0 is still valid. */
+    if ( crash_notes[nr].start )
+    {
+        range->start = __pa(crash_notes[nr].start);
+        range->size = crash_notes[nr].size;
+    }
+    else
+        range->start = range->size = 0;
 
-    if ( per_cpu(crash_notes, nr) == NULL )
-    {
-        Elf_Note *note;
-
-        note = per_cpu(crash_notes, nr) = xmalloc_bytes(nr_bytes);
-
-        if ( note == NULL )
-            return -ENOMEM;
-
-        /* Setup CORE note. */
-        setup_note(note, "CORE", NT_PRSTATUS, sizeof(ELF_Prstatus));
-
-        /* Setup Xen CORE note. */
-        note = ELFNOTE_NEXT(note);
-        setup_note(note, "Xen", XEN_ELFNOTE_CRASH_REGS, sizeof(crash_xen_core_t));
-
-        if (nr == 0)
-        {
-            /* Setup system wide Xen info note. */
-            xen_crash_note = note = ELFNOTE_NEXT(note);
-            setup_note(note, "Xen", XEN_ELFNOTE_CRASH_INFO, sizeof(crash_xen_info_t));
-        }
-    }
-
-    range->start = __pa((unsigned long)per_cpu(crash_notes, nr));
-    range->size = nr_bytes;
     return 0;
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2 of 3] KEXEC: Allocate crash structures in low memory

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

On 64bit Xen with 32bit dom0 and crashkernel, xmalloc'ing items such
as the CPU crash notes will go into the xenheap, which tends to be in
upper memory.  This causes problems on machines with more than 64GB
(or 4GB if no PAE support) of ram as the crashkernel physically cant
access the crash notes.

The solution is to force Xen to allocate certain structures in lower
memory.  This is achieved by introducing two new command line
parameters; low_crashinfo and crashinfo_maxaddr.  Because of the
potential impact on 32bit PV guests, and that this problem does not
exist for 64bit dom0 on 64bit Xen, this new functionality defaults to
the codebase's previous behavior, requiring the user to explicitly
add extra command line parameters to change the behavior.

This patch consists of 3 logically distinct but closely related
changes.
 1) Add the two new command line parameters.
 2) Change crash note allocation to use lower memory when instructed.
 3) Change the conring buffer to use lower memory when instructed.

There result is that the crash notes and console ring will be placed
in lower memory so useful information can be recovered in the case of
a crash.

Changes since v1:
 -  Patch xen-command-line.markdown to document new options

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>



[-- Attachment #2: KEXEC-allocate-crash-structures-low.patch --]
[-- Type: text/x-patch, Size: 11413 bytes --]

On 64bit Xen with 32bit dom0 and crashkernel, xmalloc'ing items such
as the CPU crash notes will go into the xenheap, which tends to be in
upper memory.  This causes problems on machines with more than 64GB
(or 4GB if no PAE support) of ram as the crashkernel physically cant
access the crash notes.

The solution is to force Xen to allocate certain structures in lower
memory.  This is achieved by introducing two new command line
parameters; low_crashinfo and crashinfo_maxaddr.  Because of the
potential impact on 32bit PV guests, and that this problem does not
exist for 64bit dom0 on 64bit Xen, this new functionality defaults to
the codebase's previous behavior, requiring the user to explicitly
add extra command line parameters to change the behavior.

This patch consists of 3 logically distinct but closely related
changes.
 1) Add the two new command line parameters.
 2) Change crash note allocation to use lower memory when instructed.
 3) Change the conring buffer to use lower memory when instructed.

There result is that the crash notes and console ring will be placed
in lower memory so useful information can be recovered in the case of
a crash.

Changes since v1:
 -  Patch xen-command-line.markdown to document new options

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 1b938f3fbad5 -r 8ef04a0f6241 docs/misc/xen-command-line.markdown
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -188,6 +188,13 @@ The optional trailing `x` indicates that
 ### cpuid\_mask\_xsave\_eax
 ### cpuidle
 ### cpuinfo
+### crashinfo_maxaddr
+> `= <size>`
+
+> Default: `4G`
+
+Specify the maximum address to allocate certain strucutres, if used in combination with the `low_crashinfo` command line option.
+
 ### crashkernel
 ### credit2\_balance\_over
 ### credit2\_balance\_under
@@ -273,6 +280,13 @@ Set the logging level for Xen.  Any log 
 
 The optional `<rate-limited level>` options instructs which severities should be rate limited.
 
+### low\_crashinfo
+> `= none | min | all`
+
+> Default: `none` if not specified at all, or to `min` if `low\_crashinfo` is present without qualification.
+
+This option is only useful for hosts with a 32bit dom0 kernel, wishing to use kexec functionality in the case of a crash.  It represents which data structures should be deliberatly allocated in low memory, so the crash kernel may find find them.  Should be used in combination with `crashinfo_maxaddr`.
+
 ### max\_cstate
 ### max\_gsi\_irqs
 ### maxcpus
diff -r 1b938f3fbad5 -r 8ef04a0f6241 xen/arch/x86/setup.c
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -586,6 +586,10 @@ void __init __start_xen(unsigned long mb
     }
     cmdline_parse(cmdline);
 
+    /* Must be after command line argument parsing and before
+     * allocing any xenheap structures wanted in lower memory. */
+    kexec_early_calculations();
+
     parse_video_info();
 
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
diff -r 1b938f3fbad5 -r 8ef04a0f6241 xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -36,7 +36,9 @@ bool_t kexecing = FALSE;
 typedef struct { Elf_Note * start; size_t size; } crash_note_range_t;
 static crash_note_range_t * crash_notes;
 
-/* Lock to prevent race conditions when allocating the crash note buffers. */
+/* Lock to prevent race conditions when allocating the crash note buffers.
+ * It also serves to protect calls to alloc_from_crash_heap when allocating
+ * crash note buffers in lower memory. */
 static DEFINE_SPINLOCK(crash_notes_lock);
 
 static Elf_Note *xen_crash_note;
@@ -62,6 +64,24 @@ static struct {
     unsigned long size;
 } ranges[16] __initdata;
 
+/* Low crashinfo mode.  Start as INVALID so serveral codepaths can set up
+ * defaults without needing to know the state of the others. */
+enum low_crashinfo low_crashinfo_mode = LOW_CRASHINFO_INVALID;
+
+/* This value is only considered if low_crash_mode is set to MIN or ALL, so
+ * setting a default here is safe. Default to 4GB.  This is because the current
+ * KEXEC_CMD_get_range compat hypercall trucates 64bit pointers to 32 bits. The
+ * typical usecase for crashinfo_maxaddr will be for 64bit Xen with 32bit dom0
+ * and 32bit crash kernel. */
+static paddr_t __initdata crashinfo_maxaddr = 4ULL << 30;
+
+/* = log base 2 of crashinfo_maxaddr after checking for sanity. Default to
+ * larger than the entire physical address space. */
+paddr_t crashinfo_maxaddr_bits = 64;
+
+/* Pointers to keep track of the crash heap region. */
+static void *crash_heap_current = NULL, *crash_heap_end = NULL;
+
 /*
  * Parse command lines in the format
  *
@@ -140,6 +160,58 @@ static void __init parse_crashkernel(con
 }
 custom_param("crashkernel", parse_crashkernel);
 
+/* Parse command lines in the format:
+ *
+ *   low_crashinfo=[none,min,all]
+ *
+ * - none disables the low allocation of crash info.
+ * - min will allocate enough low information for the crash kernel to be able
+ *       to extract the hypervisor and dom0 message ring buffers.
+ * - all will allocate additional structures such as domain and vcpu structs
+ *       low so the crash kernel can perform an extended analysis of state.
+ */
+static void __init parse_low_crashinfo(const char * str)
+{
+
+    if ( !strlen(str) )
+        /* default to min if user just specifies "low_crashinfo" */
+        low_crashinfo_mode = LOW_CRASHINFO_MIN;
+    else if ( !strcmp(str, "none" ) )
+        low_crashinfo_mode = LOW_CRASHINFO_NONE;
+    else if ( !strcmp(str, "min" ) )
+        low_crashinfo_mode = LOW_CRASHINFO_MIN;
+    else if ( !strcmp(str, "all" ) )
+        low_crashinfo_mode = LOW_CRASHINFO_ALL;
+    else
+    {
+        printk("Unknown low_crashinfo parameter '%s'.  Defaulting to min.\n", str);
+        low_crashinfo_mode = LOW_CRASHINFO_MIN;
+    }
+}
+custom_param("low_crashinfo", parse_low_crashinfo);
+
+/* Parse command lines in the format:
+ *
+ *   crashinfo_maxaddr=<addr>
+ *
+ * <addr> will be rounded down to the nearest power of two.  Defaults to 64G
+ */
+static void __init parse_crashinfo_maxaddr(const char * str)
+{
+    u64 addr;
+
+    /* if low_crashinfo_mode is unset, default to min. */
+    if ( low_crashinfo_mode == LOW_CRASHINFO_INVALID )
+        low_crashinfo_mode = LOW_CRASHINFO_MIN;
+
+    if ( (addr = parse_size_and_unit(str, NULL)) )
+        crashinfo_maxaddr = addr;
+    else
+        printk("Unable to parse crashinfo_maxaddr. Defaulting to %p\n",
+               (void*)crashinfo_maxaddr);
+}
+custom_param("crashinfo_maxaddr", parse_crashinfo_maxaddr);
+
 void __init set_kexec_crash_area_size(u64 system_ram)
 {
     unsigned int idx;
@@ -298,6 +370,20 @@ static size_t sizeof_cpu_notes(const uns
     return bytes;
 }
 
+/* Allocate size_t bytes of space from the previously allocated
+ * crash heap if the user has requested that crash notes be allocated
+ * in lower memory.  There is currently no case where the crash notes
+ * should be free()'d. */
+static void * alloc_from_crash_heap(const size_t bytes)
+{
+    void * ret;
+    if ( crash_heap_current + bytes > crash_heap_end )
+        return NULL;
+    ret = (void*)crash_heap_current;
+    crash_heap_current += bytes;
+    return ret;
+}
+
 /* Allocate a crash note buffer for a newly onlined cpu. */
 static int kexec_init_cpu_notes(const unsigned long cpu)
 {
@@ -312,7 +398,10 @@ static int kexec_init_cpu_notes(const un
         return ret;
 
     nr_bytes = sizeof_cpu_notes(cpu);
-    note = xmalloc_bytes(nr_bytes);
+
+    /* If we dont care about the position of allocation, malloc. */
+    if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
+        note = xmalloc_bytes(nr_bytes);
 
     /* Protect the write into crash_notes[] with a spinlock, as this function
      * is on a hotplug path and a hypercall path. */
@@ -330,6 +419,11 @@ static int kexec_init_cpu_notes(const un
     }
     else
     {
+        /* If we care about memory possition, alloc from the crash heap,
+         * also protected by the crash_notes_lock. */
+        if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
+            note = alloc_from_crash_heap(nr_bytes);
+
         crash_notes[cpu].start = note;
         crash_notes[cpu].size = nr_bytes;
         spin_unlock(&crash_notes_lock);
@@ -389,6 +483,18 @@ static struct notifier_block cpu_nfb = {
     .notifier_call = cpu_callback
 };
 
+void __init kexec_early_calculations(void)
+{
+    /* If low_crashinfo_mode is still INVALID, neither "low_crashinfo" nor
+     * "crashinfo_maxaddr" have been specified on the command line, so
+     * explicitly set to NONE. */
+    if ( low_crashinfo_mode == LOW_CRASHINFO_INVALID )
+        low_crashinfo_mode = LOW_CRASHINFO_NONE;
+
+    if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
+        crashinfo_maxaddr_bits = fls(crashinfo_maxaddr) - 1;
+}
+
 static int __init kexec_init(void)
 {
     void *cpu = (void *)(unsigned long)smp_processor_id();
@@ -397,6 +503,29 @@ static int __init kexec_init(void)
     if ( !kexec_crash_area.size )
         return 0;
 
+    if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
+    {
+        size_t crash_heap_size;
+
+        /* This calculation is safe even if the machine is booted in
+         * uniprocessor mode. */
+        crash_heap_size = sizeof_cpu_notes(0) +
+            sizeof_cpu_notes(1) * (nr_cpu_ids - 1);
+        crash_heap_size = PAGE_ALIGN(crash_heap_size);
+
+        crash_heap_current = alloc_xenheap_pages(
+            get_order_from_bytes(crash_heap_size),
+            MEMF_bits(crashinfo_maxaddr_bits) );
+
+        if ( ! crash_heap_current )
+            return -ENOMEM;
+
+        crash_heap_end = crash_heap_current + crash_heap_size;
+    }
+
+    /* crash_notes may be allocated anywhere Xen can reach in memory.
+       Only the individual CPU crash notes themselves must be allocated
+       in lower memory if requested. */
     crash_notes = xzalloc_array(crash_note_range_t, nr_cpu_ids);
     if ( ! crash_notes )
         return -ENOMEM;
diff -r 1b938f3fbad5 -r 8ef04a0f6241 xen/drivers/char/console.c
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -596,7 +596,7 @@ void __init console_init_postirq(void)
         opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
 
     order = get_order_from_bytes(max(opt_conring_size, conring_size));
-    while ( (ring = alloc_xenheap_pages(order, 0)) == NULL )
+    while ( (ring = alloc_xenheap_pages(order, MEMF_bits(crashinfo_maxaddr_bits))) == NULL )
     {
         BUG_ON(order == 0);
         order--;
diff -r 1b938f3fbad5 -r 8ef04a0f6241 xen/include/xen/kexec.h
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -25,6 +25,19 @@ void set_kexec_crash_area_size(u64 syste
 #define KEXEC_IMAGE_CRASH_BASE   2
 #define KEXEC_IMAGE_NR           4
 
+enum low_crashinfo {
+    LOW_CRASHINFO_INVALID = 0,
+    LOW_CRASHINFO_NONE = 1,
+    LOW_CRASHINFO_MIN = 2,
+    LOW_CRASHINFO_ALL = 3
+};
+
+/* Low crashinfo mode.  Start as INVALID so serveral codepaths can set up
+ * defaults without needing to know the state of the others. */
+extern enum low_crashinfo low_crashinfo_mode;
+extern paddr_t crashinfo_maxaddr_bits;
+void kexec_early_calculations(void);
+
 int machine_kexec_load(int type, int slot, xen_kexec_image_t *image);
 void machine_kexec_unload(int type, int slot, xen_kexec_image_t *image);
 void machine_kexec_reserved(xen_kexec_reserve_t *reservation);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3 of 3] KEXEC: Introduce new crashtables interface

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

Introduce the 'crashtables' interface as an extensible method of passing data to
the crashdump kernel, providing a substantially more flexability than the
current method XenServer uses of hacking around with the symbol file.

Care is taken to suitably align values, and to expliticly disabiguate pointers
from sizes by use of a separate table, despite the raw data being compatabile
between the two.

In addition, all the details for "low_crashinfo=min" are passed into their
respective tables.

Changes since v1:
 *  Fix up naming conventions and static qualifiers.
 *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
    use. Instead, mark entries as invalid so as to reduce confusion to the
    crashdump kernel trying to parse the table.
 *  Change strtab setup so a failure at that point wont fail the entire kexec
    setup.
 *  Change the modifications to console.c to prevent marking conring and
    conring_size as global symbols.
 *  Add more details into crashtables.h clarifying certain information.
 *  Add emacs local variables at the end of crashtables.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>



[-- Attachment #2: KEXEC-add-crashtables-interface.patch --]
[-- Type: text/x-patch, Size: 13365 bytes --]

Introduce the 'crashtables' interface as an extensible method of passing data to
the crashdump kernel, providing a substantially more flexability than the
current method XenServer uses of hacking around with the symbol file.

Care is taken to suitably align values, and to expliticly disabiguate pointers
from sizes by use of a separate table, despite the raw data being compatabile
between the two.

In addition, all the details for "low_crashinfo=min" are passed into their
respective tables.

Changes since v1:
 *  Fix up naming conventions and static qualifiers.
 *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
    use. Instead, mark entries as invalid so as to reduce confusion to the
    crashdump kernel trying to parse the table.
 *  Change strtab setup so a failure at that point wont fail the entire kexec
    setup.
 *  Change the modifications to console.c to prevent marking conring and
    conring_size as global symbols.
 *  Add more details into crashtables.h clarifying certain information.
 *  Add emacs local variables at the end of crashtables.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 8ef04a0f6241 -r 90208fe69bd4 xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -16,6 +16,7 @@
 #include <xen/types.h>
 #include <xen/kexec.h>
 #include <xen/keyhandler.h>
+#include <xen/compile.h>
 #include <public/kexec.h>
 #include <xen/cpumask.h>
 #include <asm/atomic.h>
@@ -23,7 +24,9 @@
 #include <xen/version.h>
 #include <xen/console.h>
 #include <xen/kexec.h>
+#include <public/version.h>
 #include <public/elfnote.h>
+#include <public/crashtables.h>
 #include <xsm/xsm.h>
 #include <xen/cpu.h>
 #ifdef CONFIG_COMPAT
@@ -41,7 +44,7 @@ static crash_note_range_t * crash_notes;
  * crash note buffers in lower memory. */
 static DEFINE_SPINLOCK(crash_notes_lock);
 
-static Elf_Note *xen_crash_note;
+static Elf_Note *xen_crash_note, *xen_stringtab, *xen_valtab, *xen_symtab;
 
 static cpumask_t crash_saved_cpus;
 
@@ -82,6 +85,22 @@ paddr_t crashinfo_maxaddr_bits = 64;
 /* Pointers to keep track of the crash heap region. */
 static void *crash_heap_current = NULL, *crash_heap_end = NULL;
 
+/* Crash table data structures */
+static char * crash_strtab = NULL;
+static size_t crash_strtab_size = 0;
+
+static val64tab_t crash_val64tab[] =
+{
+    { XEN_VALTAB_MAX_PAGE, 0 },
+    { XEN_VALTAB_CONRING_SIZE, 0 }
+};
+
+static sym64tab_t crash_sym64tab[] =
+{
+    { XEN_SYMTAB_CONRING, 0 }
+};
+
+
 /*
  * Parse command lines in the format
  *
@@ -262,12 +281,115 @@ void kexec_crash_save_cpu(void)
     elf_core_save_regs(&prstatus->pr_reg, xencore);
 }
 
+/* Round val up to a machine word alignment */
+size_t round_up(size_t val)
+{
+    return ((val + (sizeof(unsigned long)-1)) &
+            ~((sizeof (unsigned long))-1));
+}
+
+extern xen_commandline_t saved_cmdline;
+/* Setup the stringtable.  In case of failue, crash_strtab pointer is left
+ * as NULL. */
+static int setup_strtab(void)
+{
+    char * ptr;
+    size_t sl = sizeof(unsigned long);
+
+#define STRING(x) #x
+#define INT2STR(x) STRING(x)
+
+#define XEN_STR_VERSION INT2STR(XEN_VERSION) "." INT2STR(XEN_SUBVERSION) \
+        XEN_EXTRAVERSION
+#define XEN_STR_COMPILED_BY XEN_COMPILE_BY "@" XEN_COMPILE_HOST "." \
+        XEN_COMPILE_DOMAIN
+
+    size_t size =
+        sl + round_up(sizeof(XEN_STR_VERSION)) +
+        sl + round_up(sizeof(XEN_CHANGESET)) +
+        sl + round_up(sizeof(XEN_COMPILE_DATE)) +
+        sl + round_up(sizeof(XEN_STR_COMPILED_BY)) +
+        sl + round_up(sizeof(XEN_COMPILER)) +
+        sl + round_up(strlen(saved_cmdline)+1);
+
+    crash_strtab = xzalloc_bytes(size);
+    if ( ! crash_strtab )
+        return -ENOMEM;
+
+    crash_strtab_size = size;
+    ptr = crash_strtab;
+
+#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
+    *(unsigned long*)ptr = (v); ptr += sl;                  \
+    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
+
+#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
+    *(unsigned long*)ptr = (v); ptr += sl;                  \
+    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)
+
+    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_VERSION, XEN_STR_VERSION);
+    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_CSET, XEN_CHANGESET);
+    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILE_DATE, XEN_COMPILE_DATE);
+    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILED_BY, XEN_STR_COMPILED_BY);
+    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILER, XEN_COMPILER);
+    WRITE_STRTAB_ENTRY_VAR(XEN_STRINGTAB_CMDLINE, saved_cmdline);
+
+#undef WRITE_STRTAB_ENTRY_VAR
+#undef WRITE_STRTAB_ENTRY_CONST
+#undef XEN_STR_COMPILED_BY
+#undef XEN_STR_VERSION
+#undef INT2STR
+#undef STRING
+
+    return 0;
+}
+
+static void write_val64tab(void)
+{
+    int i;
+    for ( i=0; i<ARRAY_SIZE(crash_val64tab); ++i )
+    {
+        switch( crash_val64tab[i].id )
+        {
+        case XEN_VALTAB_MAX_PAGE:
+            crash_val64tab[i].val = (uint64_t)(max_page);
+            break;
+        case XEN_VALTAB_CONRING_SIZE:
+            console_write_val64tab(&crash_val64tab[i]);
+            break;
+        default:
+            crash_val64tab[i].id = XEN_VALTAB_INVALID;
+            crash_val64tab[i].val = 0;
+            break;
+        }
+    }
+}
+
+static void write_sym64tab(void)
+{
+    int i;
+    for ( i=0; i<ARRAY_SIZE(crash_sym64tab); ++i )
+    {
+        switch( crash_sym64tab[i].id )
+        {
+        case XEN_SYMTAB_CONRING:
+            console_write_sym64tab(&crash_sym64tab[i]);
+            break;
+        default:
+            crash_sym64tab[i].id = XEN_SYMTAB_INVALID;
+            crash_sym64tab[i].addr = 0;
+            break;
+        }
+    }
+}
+
 /* Set up the single Xen-specific-info crash note. */
 crash_xen_info_t *kexec_crash_save_info(void)
 {
     int cpu = smp_processor_id();
     crash_xen_info_t info;
     crash_xen_info_t *out = (crash_xen_info_t *)ELFNOTE_DESC(xen_crash_note);
+    char * details;
 
     BUG_ON(!cpumask_test_and_set_cpu(cpu, &crash_saved_cpus));
 
@@ -284,6 +406,20 @@ crash_xen_info_t *kexec_crash_save_info(
     /* Copy from guaranteed-aligned local copy to possibly-unaligned dest. */
     memcpy(out, &info, sizeof(info));
 
+    if ( crash_strtab_size )
+    {
+        details = ELFNOTE_DESC(xen_stringtab);
+        memcpy(details, crash_strtab, crash_strtab_size);
+    }
+
+    details = ELFNOTE_DESC(xen_valtab);
+    write_val64tab();
+    memcpy(details, crash_val64tab, sizeof(crash_val64tab));
+
+    details = ELFNOTE_DESC(xen_symtab);
+    write_sym64tab();
+    memcpy(details, crash_sym64tab, sizeof(crash_sym64tab));
+
     return out;
 }
 
@@ -365,7 +501,10 @@ static size_t sizeof_cpu_notes(const uns
     /* CPU0 also presents the crash_xen_info note. */
     if ( ! cpu )
         bytes = bytes +
-            sizeof_note("Xen", sizeof(crash_xen_info_t));
+            sizeof_note("Xen", sizeof(crash_xen_info_t)) +
+            sizeof_note("Xen", crash_strtab_size) +
+            sizeof_note("Xen", sizeof(crash_val64tab)) +
+            sizeof_note("Xen", sizeof(crash_sym64tab));
 
     return bytes;
 }
@@ -449,6 +588,24 @@ static int kexec_init_cpu_notes(const un
                 xen_crash_note = note = ELFNOTE_NEXT(note);
                 setup_note(note, "Xen", XEN_ELFNOTE_CRASH_INFO,
                            sizeof(crash_xen_info_t));
+
+                /* Set up Xen crash_strtab note, if successfully created. */
+                if ( crash_strtab_size )
+                {
+                    xen_stringtab = note = ELFNOTE_NEXT(note);
+                    setup_note(note, "Xen", XEN_CRASHTABLE_STRINGTAB,
+                               crash_strtab_size);
+                }
+
+                /* Set up Xen crash_val64tab note. */
+                xen_valtab = note = ELFNOTE_NEXT(note);
+                setup_note(note, "Xen", XEN_CRASHTABLE_VAL64TAB,
+                           sizeof(crash_val64tab));
+
+                /* Set up Xen crash_sym64tab note. */
+                xen_symtab = note = ELFNOTE_NEXT(note);
+                setup_note(note, "Xen", XEN_CRASHTABLE_SYM64TAB,
+                           sizeof(crash_sym64tab));
             }
         }
     }
@@ -503,6 +660,9 @@ static int __init kexec_init(void)
     if ( !kexec_crash_area.size )
         return 0;
 
+    /* If unsuccessful, crash_strtab remains NULL and later code copes. */
+    setup_strtab();
+
     if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
     {
         size_t crash_heap_size;
diff -r 8ef04a0f6241 -r 90208fe69bd4 xen/drivers/char/console.c
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -996,6 +996,34 @@ void __warn(char *file, int line)
 }
 
 
+/* Write static values into crashtables.  This saves making these symbols
+ * non static. */
+void console_write_val64tab(val64tab_t * valtab)
+{
+    switch(valtab->id)
+    {
+    case XEN_VALTAB_CONRING_SIZE:
+        valtab->val = (uint64_t)conring_size;
+        break;
+    default:
+        break;
+    }
+}
+
+void console_write_sym64tab(sym64tab_t * symtab)
+{
+    switch(symtab->id)
+    {
+    case XEN_SYMTAB_CONRING:
+        symtab->addr = (uint64_t)__pa(conring);
+        break;
+    default:
+        break;
+    }
+}
+
+
+
 /*
  * **************************************************************
  * ****************** Console suspend/resume ********************
diff -r 8ef04a0f6241 -r 90208fe69bd4 xen/include/public/crashtables.h
--- /dev/null
+++ b/xen/include/public/crashtables.h
@@ -0,0 +1,97 @@
+/******************************************************************************
+ * crashtables.h
+ *
+ * Definitions used for the Xen ELF crash notes.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2011,2012 Andrew Cooper, Citrix Ltd.
+ */
+
+#ifndef __XEN_PUBLIC_CRASHTABLES_H__
+#define __XEN_PUBLIC_CRASHTABLES_H__
+
+#include "xen.h"
+
+/*
+ * Xen crash tables.
+ *
+ * Base number chosen as the logical progression following
+ * XEN_ELFNOTE_CRASH and XEN_ELFNOTE_DUMPCORE.
+ *
+ * Crashdump envronments can work out how long an 'unsigned long' is by the type
+ * of core file Xen produces.  This simplifies the Xen code as it will never
+ * have to write tables with mixed word sizes for the id parameter.
+ */
+
+/*
+ * Xen string crash table.
+ * Note is a list in the form of:
+ *     Machine word (32/64bit) String ID.
+ *     Null terminating string, 0-extended to word alignment.
+ */
+#define XEN_CRASHTABLE_STRINGTAB            0x3000000
+
+#define XEN_STRINGTAB_INVALID 0
+#define XEN_STRINGTAB_VERSION 1
+#define XEN_STRINGTAB_CSET 2
+#define XEN_STRINGTAB_COMPILE_DATE 3
+#define XEN_STRINGTAB_COMPILED_BY 4
+#define XEN_STRINGTAB_COMPILER 5
+#define XEN_STRINGTAB_CMDLINE 6
+
+/*
+ * Xen value crash table.
+ * Note is a list in the form of:
+ *     Machine word (32/64bit) Value ID.
+ *     64bit value.
+ */
+#define XEN_CRASHTABLE_VAL64TAB             0x3000001
+
+typedef struct { unsigned long id; uint64_t val; } val64tab_t;
+
+#define XEN_VALTAB_INVALID 0
+#define XEN_VALTAB_MAX_PAGE 1
+#define XEN_VALTAB_CONRING_SIZE 2
+
+/*
+ * Xen symbol crash table
+ * Note is a list in the form of:
+ *     Machine word (32/64bit) Symbol ID.
+ *     Symbol Address (64bit pointers).
+ */
+#define XEN_CRASHTABLE_SYM64TAB             0x3000002
+
+typedef struct { unsigned long id; uint64_t addr; } sym64tab_t;
+
+#define XEN_SYMTAB_INVALID 0
+#define XEN_SYMTAB_CONRING 1
+
+
+#endif /* __XEN_PUBLIC_CRASHTABLES_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-set-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff -r 8ef04a0f6241 -r 90208fe69bd4 xen/include/xen/console.h
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -9,6 +9,7 @@
 
 #include <xen/inttypes.h>
 #include <public/xen.h>
+#include <public/crashtables.h>
 
 struct xen_sysctl_readconsole;
 long read_console_ring(struct xen_sysctl_readconsole *op);
@@ -37,6 +38,9 @@ int console_steal(int handle, void (*fn)
 /* Give back stolen console. Takes the identifier returned by console_steal. */
 void console_giveback(int id);
 
+void console_write_val64tab(val64tab_t * valtab);
+void console_write_sym64tab(sym64tab_t * symtab);
+
 int console_suspend(void);
 int console_resume(void);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3 of 3] KEXEC: Introduce new crashtables interface

[Jan Beulich]

>>> On 12.03.12 at 15:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Changes since v1:
>  *  Fix up naming conventions and static qualifiers.
>  *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
>     use. Instead, mark entries as invalid so as to reduce confusion to the
>     crashdump kernel trying to parse the table.
>  *  Change strtab setup so a failure at that point wont fail the entire kexec
>     setup.
>  *  Change the modifications to console.c to prevent marking conring and
>     conring_size as global symbols.
>  *  Add more details into crashtables.h clarifying certain information.

While you indeed added a few things, I don't see any of my remarks
against v1 having got addressed.

>  *  Add emacs local variables at the end of crashtables.h

I'd rather like to see them go away in the other public headers too.

>+#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
>+    *(unsigned long*)ptr = (v); ptr += sl;                  \
>+    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
>+
>+#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
>+    *(unsigned long*)ptr = (v); ptr += sl;                  \
>+    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)

Didn't even notice in v1 that here you're using "unsigned long" too.

>+        switch( crash_val64tab[i].id )
>+        {
>+        case XEN_VALTAB_MAX_PAGE:
>+            crash_val64tab[i].val = (uint64_t)(max_page);

Pointless cast and parentheses.

>+            break;
>+        case XEN_VALTAB_CONRING_SIZE:
>+            console_write_val64tab(&crash_val64tab[i]);

            console_write_val64tab(crash_val64tab[i].id, &crash_val64tab[i].val);

would eliminate the need to expose the crashval types to console.h.
I'd further suggest folding the two console related functions.

And my reservations remain: Why is this any better than the consumer
looking at the symbol table (as it would continue to need to do for all
other information it might be after).

Jan

Re: [PATCH 3 of 3] KEXEC: Introduce new crashtables interface

[Andrew Cooper]

On 12/03/12 16:35, Jan Beulich wrote:
>>>> On 12.03.12 at 15:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Changes since v1:
>>  *  Fix up naming conventions and static qualifiers.
>>  *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
>>     use. Instead, mark entries as invalid so as to reduce confusion to the
>>     crashdump kernel trying to parse the table.
>>  *  Change strtab setup so a failure at that point wont fail the entire kexec
>>     setup.
>>  *  Change the modifications to console.c to prevent marking conring and
>>     conring_size as global symbols.
>>  *  Add more details into crashtables.h clarifying certain information.
> While you indeed added a few things, I don't see any of my remarks
> against v1 having got addressed.

There is a new comment section at the top, explaining the basis for
numbering, and why unsigned longs are not a problem.

To summarize:

Numbering follows on from XEN_ELFNOTE_CRASH_* (0x100000x) and
XEN_ELFNOTE_DUMPCORE_* (0x200000x)

unsigned longs are more complicated.  The justification was that Xen
would only ever use the size it was compiled for, and this would be
calculable from the core file class (ELFCLASS{32,64})  Any crashdump
environment designed for the same bitness as Xen would be fine, and any
general crashdump environment would have to implement all possible
sizes, based on the class.

I will see if there is a neat way to expose explicit structures for both
possible bitnesses of Xen.

>>  *  Add emacs local variables at the end of crashtables.h
> I'd rather like to see them go away in the other public headers too.

I am ambivalent, but will maintain consistency with the others.

>> +#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
>> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
>> +    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
>> +
>> +#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
>> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
>> +    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)
> Didn't even notice in v1 that here you're using "unsigned long" too.

Same justification as above.

>> +        switch( crash_val64tab[i].id )
>> +        {
>> +        case XEN_VALTAB_MAX_PAGE:
>> +            crash_val64tab[i].val = (uint64_t)(max_page);
> Pointless cast and parentheses.

True

>> +            break;
>> +        case XEN_VALTAB_CONRING_SIZE:
>> +            console_write_val64tab(&crash_val64tab[i]);
>             console_write_val64tab(crash_val64tab[i].id, &crash_val64tab[i].val);
>
> would eliminate the need to expose the crashval types to console.h.
> I'd further suggest folding the two console related functions.

But the crashval #DEFINES would still need to be exposed and they are
all in the same header file.

The symtab and valtab functions are separate because they refer to
separate tables.  If I were to fold the functions, I would either need
an extra parameter and nested switch statements, or settle with a
dependence between ID values.  The first is an ugly hack, and the second
works against the design principles of separating the tables in the
first place.

> And my reservations remain: Why is this any better than the consumer
> looking at the symbol table (as it would continue to need to do for all
> other information it might be after).

There are several different reasons.  In the 'min' case, it is true that
all relevant information is in the symbol table, but there is
information required for 'all' which wont be in the symbol table.  We
have had cases in the past where users have updated Xen without updating
the symbol file, resulting in confusing or wrong crash dump analyses. 
Whilst in an ideal world, we wouldn't support this, that argument
doesn't fly politically.

Finally, and most importantly along the line of 32bit crashdump support
on 64bit Xen; It is problematic to find out (through the symbol file and
page table walks) that an value you need is located in memory outside
the first 64GB.  By entering the value into the value table, it is
explicitly being made accessible to a 32bit crash kernel.

> Jan
>
-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 3 of 3] KEXEC: Introduce new crashtables interface

[Jan Beulich]

>>> On 12.03.12 at 19:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 12/03/12 16:35, Jan Beulich wrote:
>>>>> On 12.03.12 at 15:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> Changes since v1:
>>>  *  Fix up naming conventions and static qualifiers.
>>>  *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
>>>     use. Instead, mark entries as invalid so as to reduce confusion to the
>>>     crashdump kernel trying to parse the table.
>>>  *  Change strtab setup so a failure at that point wont fail the entire kexec
>>>     setup.
>>>  *  Change the modifications to console.c to prevent marking conring and
>>>     conring_size as global symbols.
>>>  *  Add more details into crashtables.h clarifying certain information.
>> While you indeed added a few things, I don't see any of my remarks
>> against v1 having got addressed.
> 
> There is a new comment section at the top, explaining the basis for
> numbering, and why unsigned longs are not a problem.

Didn't expect it there, and thus overlooked it - I'm sorry.

> To summarize:
> 
> Numbering follows on from XEN_ELFNOTE_CRASH_* (0x100000x) and
> XEN_ELFNOTE_DUMPCORE_* (0x200000x)

Then please call the new ones XEN_ELFNOTE_* too, fold the whole
stuff into public/elfnote.h, and generate the data as normal notes.
Or is there a reason not to? Even more so that part of what you do
here is really overlapping with XEN_ELFNOTE_DUMPCORE_* (which
you could simply re-use).

> unsigned longs are more complicated.  The justification was that Xen
> would only ever use the size it was compiled for, and this would be
> calculable from the core file class (ELFCLASS{32,64})  Any crashdump
> environment designed for the same bitness as Xen would be fine, and any
> general crashdump environment would have to implement all possible
> sizes, based on the class.

If going with "proper" notes, this will become mute anyway.

>>> +#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
>>> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
>>> +    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
>>> +
>>> +#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
>>> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
>>> +    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)
>> Didn't even notice in v1 that here you're using "unsigned long" too.
> 
> Same justification as above.

Yet here it doesn't even make sense - you're not going to have a
string table of 4Gb or more in size, and hence you can as well use
uint32_t uniformly.

>>> +            break;
>>> +        case XEN_VALTAB_CONRING_SIZE:
>>> +            console_write_val64tab(&crash_val64tab[i]);
>>             console_write_val64tab(crash_val64tab[i].id, 
> &crash_val64tab[i].val);
>>
>> would eliminate the need to expose the crashval types to console.h.
>> I'd further suggest folding the two console related functions.
> 
> But the crashval #DEFINES would still need to be exposed and they are
> all in the same header file.

Then pass a bool_t instead of the ID. You're wanting two values from
the console code only after all.

> The symtab and valtab functions are separate because they refer to
> separate tables.  If I were to fold the functions, I would either need
> an extra parameter and nested switch statements, or settle with a
> dependence between ID values.  The first is an ugly hack, and the second
> works against the design principles of separating the tables in the
> first place.

No, the separation can be completely transparent to the console code,
just leveraging that the data type is always uint64_t.

>> And my reservations remain: Why is this any better than the consumer
>> looking at the symbol table (as it would continue to need to do for all
>> other information it might be after).
> 
> There are several different reasons.  In the 'min' case, it is true that
> all relevant information is in the symbol table, but there is
> information required for 'all' which wont be in the symbol table.  We
> have had cases in the past where users have updated Xen without updating
> the symbol file, resulting in confusing or wrong crash dump analyses. 

When installing a package (or even manually via "make install-xen")
this can't happen, so someone must have broken this by copying
stuff manually. Which I don't think the code ought to be prepared
to deal with. You just can't have provisions for all sort of admin
mistakes - it won't scale.

> Whilst in an ideal world, we wouldn't support this, that argument
> doesn't fly politically.
> 
> Finally, and most importantly along the line of 32bit crashdump support
> on 64bit Xen; It is problematic to find out (through the symbol file and
> page table walks) that an value you need is located in memory outside
> the first 64GB.  By entering the value into the value table, it is
> explicitly being made accessible to a 32bit crash kernel.

I'm not following - Xen always resides in memory below 4G, so no
symbol in the symbol table can possibly be inaccessible (with the
pages tables for Xen's code and data being static and hence below
4G too, eventual page table walks wouldn't represent a problem
either).

Jan