From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hollis Blanchard <hollisb@us.ibm.com>
Subject: Re: [PATCH] [xen,
	xencomm] various xencomm fixes (was Re: [XenPPC]
	Re: [Xen-ia64-devel] [PATCH 1/2] remove xencomm page size limit(xen
	side))
Date: Fri, 10 Aug 2007 13:54:47 -0500
Message-ID: <1186772087.7073.26.camel@basalt>
References: <20070731061019.GB6039%yamahata@valinux.co.jp>
	<1185913517.6802.74.camel@bling>
	<20070801063654.GD14448%yamahata@valinux.co.jp>
	<1185995274.15389.21.camel@basalt>
	<20070802024439.GB6395%yamahata@valinux.co.jp>
	<1186066846.27484.7.camel@basalt>
	<20070803021242.GB17231%yamahata@valinux.co.jp>
	<1186152196.17978.1.camel@diesel>
	<20070807084454.GB20189%yamahata@valinux.co.jp>
Reply-To: Hollis Blanchard <hollisb@us.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <20070807084454.GB20189%yamahata@valinux.co.jp>
List-Unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: xen-ppc-devel <xen-ppc-devel@lists.xensource.com>, xen-devel@lists.xensource.com, Alex Williamson <alex.williamson@hp.com>, xen-ia64-devel@lists.xensource.com
List-Id: xen-devel@lists.xenproject.org

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