From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign Date: Tue, 10 Dec 2013 14:44:30 +0000 Message-ID: <52A728CE.2090908@linaro.org> References: <1386560047-17500-1-git-send-email-julien.grall@linaro.org> <1386560047-17500-10-git-send-email-julien.grall@linaro.org> <1386607240.7812.60.camel@kazak.uk.xensource.com> <52A673BB.4090902@linaro.org> <1386668222.11925.11.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VqOY6-0003ht-Ts for xen-devel@lists.xenproject.org; Tue, 10 Dec 2013 14:44:35 +0000 Received: by mail-wi0-f176.google.com with SMTP id hq4so5517514wib.15 for ; Tue, 10 Dec 2013 06:44:33 -0800 (PST) In-Reply-To: <1386668222.11925.11.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com, patches@linaro.org List-Id: xen-devel@lists.xenproject.org On 12/10/2013 09:37 AM, Ian Campbell wrote: > On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote: >> >> On 12/09/2013 04:40 PM, Ian Campbell wrote: >>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: >>>> Xen needs to know that the current page belongs to another domain. Also take >>>> the refcount to this page. >>>> >>>> Signed-off-by: Julien Grall >>>> >>>> --- >>>> Changes in v2: >>>> - Even if gcc is buggy (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501) >>>> define p2m type per mapspace to let the compiler warns about unitialized >>>> values. >>>> --- >>>> xen/arch/arm/mm.c | 24 ++++++++++++++++-------- >>>> 1 file changed, 16 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>>> index ba51f6e..b08ffa0 100644 >>>> --- a/xen/arch/arm/mm.c >>>> +++ b/xen/arch/arm/mm.c >>>> @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one( >>>> { >>>> unsigned long mfn = 0; >>>> int rc; >>>> + p2m_type_t t; >>>> >>>> switch ( space ) >>>> { >>>> @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one( >>>> >>>> d->arch.grant_table_gpfn[idx] = gpfn; >>>> >>>> + t = p2m_ram_rw; >>>> + >>>> spin_unlock(&d->grant_table->lock); >>>> break; >>>> case XENMAPSPACE_shared_info: >>>> - if ( idx == 0 ) >>>> - mfn = virt_to_mfn(d->shared_info); >>>> - else >>>> + if ( idx != 0 ) >>>> return -EINVAL; >>>> + >>>> + mfn = virt_to_mfn(d->shared_info); >>>> + t = p2m_ram_rw; >>>> + >>>> break; >>>> case XENMAPSPACE_gmfn_foreign: >>>> { >>>> - paddr_t maddr; >>>> struct domain *od; >>>> + struct page_info *page; >>>> od = rcu_lock_domain_by_any_id(foreign_domid); >>>> if ( od == NULL ) >>>> return -ESRCH; >>>> @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one( >>>> return rc; >>>> } >>>> >>>> - maddr = p2m_lookup(od, pfn_to_paddr(idx)); >>>> - if ( maddr == INVALID_PADDR ) >>>> + /* Take refcount to the foreign domain page. >>> >>> "refcount on...". Or "Take a reference to..." >>> >>>> + * Refcount will be release in XENMEM_remove_from_physmap */ >>> >>> and a few other places... >>> >>> Like with the unmap it might be best to push this reference manipulation >>> down into the eventual function which makes the mapping. That would keep >>> this stuff much more localised. >> >> Forget to answer to this part: >> >> We need to take the reference with foreign domain in parameter, >> otherwise we don't really know if the foreign domain has unlikely >> removed the mapping between the hypercall was called and now. > > Are we not holding the p2m lock at this point? p2m lock of which domain? For now, neither of foreign and current domain lock are taken in this function. > We probably do need to do the final check and unmap with the lock held. > Either by moving the unmap within the same lock as the lookup or perhaps > with some sort of cmpxchg mechanism where we pass in the pte value we > think we are tearing down. Just checking the type may not be sufficient > if the guest tears down one foreign mapping and replaces it with > another. I'm not sure to follow you, are you talking about remove the foreign mapping? If so, I have reworking all the removing code to include the put_page (see V3). -- Julien Grall