Re: [Xen-ia64-devel] [patch 01/14] kexec for xen

[Alex Williamson <alex.williamson@xxxxxx>]

On Wed, 2007-09-12 at 17:08 +0900, Simon Horman wrote:

> @@ -401,6 +406,14 @@ efi_get_pal_addr (void)
>  
>  #ifdef XEN
>  void *pal_vaddr = 0;
> +
> +void *
> +efi_get_pal_addr(void)
> +{
> +     if (pal_vaddr)
> +             return pal_vaddr;
> +     return pal_vaddr = __efi_get_pal_addr();
> +}
>  #endif

Coding style nit

        if (!pal_vaddr)
                pal_vaddr = __efi_get_pal_addr();
        return pal_vaddr;

Does the same thing in the same number of lines and is cleaner IMHO.
 
>  void
> @@ -408,9 +421,7 @@ efi_map_pal_code (void)
>  {
>  #ifdef XEN
>       u64 psr;
> -     if (!pal_vaddr) {
> -             pal_vaddr = efi_get_pal_addr ();
> -     }
> +     efi_get_pal_addr();

(void)efi_get_pal_addr();  to make it obvious the return is
intentionally ignored?


> +#ifdef XEN
> +/* find a block of memory aligned to 64M exclude reserved regions
> + * rsvd_regions are sorted
> + */
> +unsigned long
> +kdump_find_rsvd_region(unsigned long size, struct rsvd_region *r, int n)
> +{

   This all looks fine, but is there any reason this chunk of memory
needs to be at a low address?  If not, maybe the memmap should be
searched in reverse order to avoid using up 32bit memory (should it
exist).

> +     int i;
> +     u64 start, end;
> +     u64 alignment = 1UL << _PAGE_SIZE_64M;
> +     void *efi_map_start, *efi_map_end, *p;
> +     efi_memory_desc_t *md;
> +     u64 efi_desc_size;
> +
> +     efi_map_start = __va(ia64_boot_param->efi_memmap);
> +     efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
> +     efi_desc_size = ia64_boot_param->efi_memdesc_size;
> +
> +     for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
> +             md = p;
> +             if (!efi_wb(md))
> +                     continue;
> +             start = ALIGN(md->phys_addr, alignment);
> +             end = efi_md_end(md);
> +             for (i = 0; i < n; i++) {
> +                     if (__pa(r[i].start) >= start && __pa(r[i].end) < end) {
> +                             if (__pa(r[i].start) > start + size)
> +                                     return start;
> +                             start = ALIGN(__pa(r[i].end), alignment);
> +                             if (i < n - 1
> +                                 && __pa(r[i + 1].start) < start + size)
> +                                     continue;
> +                             else
> +                                     break;
> +                     }
> +             }
> +             if (end > start + size)
> +                     return start;
> +     }
> +
> +     printk(KERN_WARNING
> +            "Cannot reserve 0x%lx byte of memory for crashdump\n", size);
> +     return ~0UL;
> +}
> +#endif

   There seems to be a mix of tab and space indenting in the
machine_kexec.c changes.  Please pick one.

> Index: xen-unstable.hg/xen/arch/ia64/xen/machine_kexec.c
> ===================================================================
> --- xen-unstable.hg.orig/xen/arch/ia64/xen/machine_kexec.c    2007-07-11 
> 13:20:14.000000000 +0900
> +++ xen-unstable.hg/xen/arch/ia64/xen/machine_kexec.c 2007-07-11 
> 13:20:16.000000000 +0900

> +static void ia64_machine_kexec(struct unw_frame_info *info, void *arg)
> +{
> +    xen_kexec_image_t *image = arg;
> +    relocate_new_kernel_t rnk;
> +    unsigned long code_addr = (unsigned long)__va(image->reboot_code_buffer);
> +    unsigned long cpu_data_pa = (unsigned long)
> +                             __pa(cpu_data(smp_processor_id()));
> +    unsigned long vector;
> +    int ii;
> +
> +    /* Interrupts aren't acceptable while we reboot */
> +    local_irq_disable();
> +
> +    /* Mask CMC and Performance Monitor interrupts */
> +    ia64_setreg(_IA64_REG_CR_PMV, 1 << 16);
> +    ia64_setreg(_IA64_REG_CR_CMCV, 1 << 16);
> +
> +    /* Mask ITV and Local Redirect Registers */
> +    ia64_set_itv(1 << 16);
> +    ia64_set_lrr0(1 << 16);
> +    ia64_set_lrr1(1 << 16);
> +
> +    /* terminate possible nested in-service interrupts */
> +    for (ii = 0; ii < 16; ii++)
> +        ia64_eoi();
> +
> +    /* unmask TPR and clear any pending interrupts */
> +    ia64_setreg(_IA64_REG_CR_TPR, 0);
> +    ia64_srlz_d();
> +    vector = ia64_get_ivr();
> +    while (vector != IA64_SPURIOUS_INT_VECTOR) {
> +        ia64_eoi();
> +        vector = ia64_get_ivr();
> +    }

   Is there a need for vector?  It seems extraneous

    while (ia64_get_ivr() != IA64_SPURIOUS_INT_VECTOR)
        ia64_eoi();

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