From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 3 of 3] KEXEC: Allocate crash structures in low memory Date: Thu, 22 Dec 2011 18:09:23 +0000 Message-ID: <4EF37253.9000903@citrix.com> References: <23c31d59ffb1590815bc.1324575385@andrewcoop.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <23c31d59ffb1590815bc.1324575385@andrewcoop.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Andrew Cooper Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org 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