[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Wed, Sep 12, 2007 at 12:21:19PM -0600, Alex Williamson wrote:
> 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.

Thanks, changed.

>  
> >  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?

Also updated.

> 
> 
> > +#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).

No, there isn't. The basic reason the code is like it is is because
thats how the Linux code is. I did previously work on a patch for
upstream Linux to search in reverse order, but it turned out to be a
little messier than I would have liked and I abandoned it. I can try
again if you like.

> 
> > +   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.

Thanks, fixed.

> 
> > 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();

Agreed. I have removed vector. I'll send a patch to upstream Linux too.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.