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: Thu, 12 Jun 2014 10:00:28 +0100 Message-ID: <53996C2C.4000900@linaro.org> References: <1402504640.16332.50.camel@kazak.uk.xensource.com> <1402504804-29173-7-git-send-email-ian.campbell@citrix.com> <5398D5F6.1080707@linaro.org> <1402558252.31087.63.camel@dagon.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402558252.31087.63.camel@dagon.hellion.org.uk> 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: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 12/06/14 08:30, Ian Campbell wrote: > On Wed, 2014-06-11 at 23:19 +0100, Julien Grall wrote: >> 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? > > "directory common"? I don't get your meaning. Sorry, I meant xen/common/ >>> + return P2M_ONE_PROGRESS; >>> + } >>> + else if ( level == 3 ) >>> + return -ENOMEM; >>> + } >>> + >>> + BUG_ON(level == 3); /* L3 is always superpage aligned */ >> >> Did you mean page-aligned? > > What I meant was that an L3 entry is always "superpage aligned" because > it is the smallest possible element. Since I wrote that I renamed my > is_superpage_aligned function to is_mapping_aligned. I should perhaps > update this comment to reflect that, which would make it clearer. Oh ok. In my mind, L3 is using page alignment not superpage alignment. I think was confuse because in your cover letter you define superpage as 2M or 1G mappings. >> [..] >> >>> + 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). > > True. > >> I think this pte.p2m.table set can be handled directly in >> mfn_to_p2m_entry. This will avoid duplicating code. > > It can't because mfn_to_p2m_entry is used to create both table and > mapping style entries. There is only on call where we don't override the pte.p2m.table bit (the one at the end of p2m_create table). I would move this extra test in the mfn_to_p2m_entry and override only for this specific case. >> [..] >> >>> + } >>> + 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? > > s/redo/undo/? Undo sorry. Regards, -- Julien Grall