Re: [Xen-ia64-devel] [patch 04/16] IA64 Kexec/kdump

[Alex Williamson <alex.williamson@xxxxxx>]

On Wed, 2007-09-12 at 17:28 +0900, Simon Horman wrote:
> plain text document attachment (ia64-kexec.patch)

> Changes and updates.
> 
> 1. Remove fake rendz path and related code according to discuss with Khalid 
> Aziz.
> 2. fc.i offset fix in relocate_kernel.S.
> 3. iospic shutdown code eoi and mask race fix from Fujitsu.
> 4. Warm boot hook in machine_kexec to SN SAL code from Jack Steiner.
> 5. Send slave to SAL slave loop patch from Jay Lan.
> 6. Kdump on non-recoverable MCA event patch from Jay Lan
> 7. Use CTL_UNNUMBERED in kdump_on_init sysctl.
> 

   Couple general issues:

      * This would be easier to consume as two patches.  One as a port
        of the upstream patch, the second with the xen-ification.
      * Tabs and spaces are mixed in this patch.  As a linux patch, this
        should use only tab indenting.  kdump_find_rsvd_region() has a
        particularly odd style.

And a specific one below...
 
> +#ifdef CONFIG_KEXEC
> +     /* crashkernel=size@offset specifies the size to reserve for a crash
> +      * kernel.(offset is ingored for keep compatibility with other archs)
> +      * By reserving this memory we guarantee that linux never set's it
> +      * up as a DMA target.Useful for holding code to do something
> +      * appropriate after a kernel panic.
> +      */
> +     {
> +             char *from = strstr(saved_command_line, "crashkernel=");
> +#ifndef CONFIG_XEN
> +             unsigned long base, size;
> +             if (from) {
> +                     size = memparse(from + 12, &from);
> +                     if (size) {
> +                             sort_regions(rsvd_region, n);
> +                             base = kdump_find_rsvd_region(size,
> +                             rsvd_region, n);
> +                             if (base != ~0UL) {
> +                                     rsvd_region[n].start =
> +                                             (unsigned long)__va(base);
> +                                     rsvd_region[n].end =
> +                                             (unsigned long)__va(base + 
> size);
> +                                     n++;
> +                                     crashk_res.start = base;
> +                                     crashk_res.end = base + size - 1;
> +                             }
> +                     }
> +             }
> +#else /* CONFIG_XEN */
> +             if (from)
> +                     printk("Ignoring crashkernel command line, "
> +                            "parameter will be supplied by xen\n");
> +             xen_machine_kexec_setup_resources();
> +#endif /* CONFIG_XEN */

   This doesn't look transparent paravirtualization safe.  I'll note
this in a few other places too, but I'm sure I've missed some.  A
CONFIG_XEN kernel should be able to boot on bare metal too.  Thanks,

        Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel