Re: [PATCH] [xen, xencomm] various xencomm fixes (was Re: [XenPPC] Re: [Xen-ia64-devel] [PATCH 1/2] remove xencomm page size limit(xen side))

[Hollis Blanchard <hollisb@us.ibm.com>]

On Tue, 2007-08-07 at 17:44 +0900, Isaku Yamahata wrote:
> 
> +/* get_page() to prevent from another vcpu freeing the page */
> +static int
> +xencomm_paddr_to_maddr(unsigned long paddr, unsigned long *maddr,
> +        struct page_info **page)
> +{
> +    *maddr = paddr_to_maddr(paddr);
> +    if (*maddr == 0)
> +        return -EFAULT;
> +
> +    *page = virt_to_page(*maddr);
> +    if (get_page(*page, current->domain) == 0) {
> +        if (page_get_owner(*page) != current->domain)
> +            /* This page might be a page granted by another domain  */
> +            panic_domain(NULL, "copy_from_guest from foreign domain\n");
> +
> +        /* Try again. */
> +        return -EAGAIN;
> +    }
> +
> +    return 0;
> +} 

PPC doesn't implement panic_domain(), so this doesn't build for me.

I don't see how we'd ever hit this case though, nor why we'd panic the
domain. How could paddr_to_maddr() ever return an maddr that doesn't
belong to the current domain? If paddr is invalid, an error should be
returned from there and propagated up, which is better than killing the
whole domain IMHO...

> +    *page = virt_to_page(*maddr);
This line doesn't make sense. Is it an maddr or a vaddr? If it's an
maddr, we shouldn't be passing it as "virt" to virt_to_page().

-- 
Hollis Blanchard
IBM Linux Technology Center