Re: [Xen-ia64-devel] [PATCH][RFC] embed memory map in domain pseudo physical address space

[Alex Williamson <alex.williamson@xxxxxx>]

Hi Isaku,

   A few comments inline.  Thanks,

        Alex

On Thu, 2007-02-01 at 22:15 +0900, Isaku Yamahata wrote:
> +static void
> +setup_dom0_memmap_info(struct domain *d, struct fw_tables *tables,
> +                      int *num_mds)
> +{
> +       int i;
> +       efi_memory_desc_t *md;
> +       efi_memory_desc_t *last_mem_md = NULL;
> +       xen_ia64_memmap_info_t* memmap_info;
> +       unsigned long paddr_start;
> +       unsigned long paddr_end;
> +
> +       for (i = 0; i < *num_mds; i++) {
> +               md = &tables->efi_memmap[i];
> +               if (md->attribute == EFI_MEMORY_WB &&
> +                   md->type == EFI_CONVENTIONAL_MEMORY &&
> +                   md->num_pages > (1UL << (PAGE_SHIFT -
> EFI_PAGE_SHIFT)))
> +                       last_mem_md = md;
> +       }
> +       if (last_mem_md == NULL) {
> +               printk("%s: warning: "
> +                      "no dom0 contiguous memory to hold memory map
> \n",
> +                      __func__);
> +               return;
> +       }

   Looks like you could search the array starting from the end as a
minor optimization.


> +       paddr_end = last_mem_md->phys_addr +
> +               (last_mem_md->num_pages << EFI_PAGE_SHIFT);
> +       paddr_start = paddr_end - (1UL << (PAGE_SHIFT -
> EFI_PAGE_SHIFT));

   This doesn't look right.  We're not multiplying by the EFI page size.

> +       paddr_start &= PAGE_MASK;

   Seems like it's possible this mask could make (paddr_start <
last_mem_md->phys_addr)


-- 
Alex Williamson                             HP Open Source & Linux Org.


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