From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 7/8] xen: arm: use superpages in p2m when pages are suitably aligned Date: Wed, 11 Jun 2014 23:19:34 +0100 Message-ID: <5398D5F6.1080707@linaro.org> References: <1402504640.16332.50.camel@kazak.uk.xensource.com> <1402504804-29173-7-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402504804-29173-7-git-send-email-ian.campbell@citrix.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 , xen-devel@lists.xen.org Cc: tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, While I was looking closer to this patch I found something strange. Why all the callers of guest_physmap_add_page in the directory common don't check that the function success to create the mapping? On 11/06/14 17:40, Ian Campbell wrote: > + page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0); > + if ( page ) > + { > + pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t); > + if ( level != 3 ) > + pte.p2m.table = 0; > + p2m_write_pte(entry, pte, flush_cache); > + p2m->stats.mappings[level]++; It looks like you forgot to update the *addr here. Did you try this code path? With the 1:1 workaround this part is never used. > + return P2M_ONE_PROGRESS; > + } > + else if ( level == 3 ) > + return -ENOMEM; > + } > + > + BUG_ON(level == 3); /* L3 is always superpage aligned */ Did you mean page-aligned? [..] > + case INSERT: > + if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) && > + /* We do not handle replacing an existing table with a superpage */ > + (level == 3 || !p2m_table(orig_pte)) ) > + { > + /* New mapping is superpage aligned, make it */ > + pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t); > + if ( level < 3 ) It's funny, sometimes you use level < 3 and some level != 3 (see in ALLOCATE). I think this pte.p2m.table set can be handled directly in mfn_to_p2m_entry. This will avoid duplicating code. [..] > + } > + else > + { > + /* New mapping is not superpage aligned, create a new table entry */ > + BUG_ON(level == 3); /* L3 is always superpage aligned */ Did you mean page-aligned? [..] > - /* Got the next page */ > - addr += PAGE_SIZE; > + rc = apply_one_level(d, &third[third_table_offset(addr)], > + 3, flush_pt, op, > + start_gpaddr, end_gpaddr, > + &addr, &maddr, &flush, > + mattr, t); > + if ( rc < 0 ) goto out; Shall we redo the whole range if the mapping has failed here? [..] > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 911d32d..821d9ef 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h [..] > +/* */ Did you intend to add a comment here? > +void p2m_dump_info(struct domain *d); > + > /* Look up the MFN corresponding to a domain's PFN. */ > paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t); > > -- Julien Grall