Re: [Xen-ia64-devel] [PATCH 1/3] p2m table expsosure xen side

[Alex Williamson <alex.williamson@xxxxxx>]

Hi Isaku,

   Some comments...

On Tue, 2006-10-03 at 18:41 +0900, Isaku Yamahata wrote:
> +static int
> +expose_p2m_page(struct domain* d, unsigned long mpaddr, struct
> page_info* page)
> +{
> +    // we can't get_page(page) here.
> +    // pte page is allocated form xen heap.(see
> pte_alloc_one_kernel().)
> +    // so that the page has NULL page owner and it's reference count
> +    // is useless.
> +    // see also relinquish_pte()'s page_get_owner() == NULL check.
> +    BUG_ON(page_get_owner(page) != NULL);
> +
> +    if (__assign_domain_page(d, mpaddr, page_to_maddr(page),
> +                             ASSIGN_readonly) < 0) {
> +        // There was a race.
> +        return -EAGAIN;
> +    }
> +    return 0;
> +}

   Couldn't this be simplified as:

    return __assign_domain_page(d, mpaddr, page_to_maddr(page), ASSIGN_readonly)


> +    // allocate pgd, pmd.
> +    for (i = conv_start_gpfn; i < expose_num_pfn + 1; i++) {
> +        conv_pte = lookup_noalloc_domain_pte(d, (conv_start_gpfn + i)
> << PAGE_SHIFT);
> +        if (conv_pte == NULL) {
> +            continue;
> +        }
> +        
> +        assign_pte = lookup_alloc_domain_pte(d, (assign_start_gpfn <<
> PAGE_SHIFT) + i * sizeof(pte_t));
> +        if (assign_pte == NULL) {
> +            DPRINTK("%s failed to allocate pte page\n", __func__);
> +            return -ENOMEM;
> +        }
> +
> +        // skip to next pte page
> +        i += PTRS_PER_PTE;
> +        i &= ~(PTRS_PER_PTE - 1);
> +        i--;// compensate i++
> +    }

  The usage of i here is a little weird (pre-decrementing to account for
the increment in the for loop).  I think it'd be better to update it
more explicitly.  How about something like:

i = conv_start_gpfn;
while (i < expose_num_pfn + 1) {
    ...
    if (conv_pte == NULL) {
        i++;
        continue;
    }
    ...
    i += PTRS_PER_PTE;
    i &= ~(PTRS_PER_PTE - 1);
}

Same for the next for loop w/ the same structure.  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