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

[David Vrabel <david.vrabel@citrix.com>]

On 22/12/11 17:36, Andrew Cooper wrote:
> 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.

I've never been entirely convinced that this was the correct approach.
It seems that using a 64-bit crash kernel would be an easier solution.

> @@ -320,7 +405,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);

Suggest refactoring this (and the other similar places) so that the
check for the regular/crash head occurs in one wrapper alloc function.

>      /* Protect the write into crash_notes[] with a spinlock, as this function
>       * is on a hotplug path and a hypercall path. */
> @@ -338,6 +426,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);
> @@ -397,6 +490,24 @@ 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;

Why not initialize low_crash_info_mode to NONE?

> +
> +    crashinfo_maxaddr_bits = 0;
> +    if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
> +    {
> +        paddr_t tmp = crashinfo_maxaddr;
> +
> +        while ( tmp >>= 1 )
> +            crashinfo_maxaddr_bits++;
> +    }
> +}

Do these during the parsing of the command line?

These will allow you to remove this unhelpfully named function.

> +
>  static int __init kexec_init(void)
>  {
>      void *cpu = (void *)(unsigned long)smp_processor_id();
> @@ -407,6 +518,29 @@ static int __init kexec_init(void)
>  
>      register_keyhandler('C', &crashdump_trigger_keyhandler);
>  
> +    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 )
> +            crash_heap_end = crash_heap_current + crash_heap_size;
> +        else
> +            return -ENOMEM;
> +    }

Suggest moving this into a crash_heap_setup() function.

> +
> +    /* 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 = xmalloc_array(crash_note_range_t, nr_cpu_ids);
>      if ( ! crash_notes )
>          return -ENOMEM;
> diff -r 3da37c68284f -r 23c31d59ffb1 xen/drivers/char/console.c
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -584,6 +584,7 @@ void __init console_init_postirq(void)
>  {
>      char *ring;
>      unsigned int i, order;
> +    u64 memflags;
>  
>      serial_init_postirq();
>  
> @@ -591,7 +592,8 @@ 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 )
> +    memflags = low_crashinfo_mode > LOW_CRASHINFO_NONE ? MEMF_bits(crashinfo_maxaddr_bits) : 0;

If you set crashinfo_maxaddr_bits to 64 if low_crashinfo_mode isn't used
you wouldn't need this test.

David