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