From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for-4.5 v2 2/2] xen: arm: Enable physical address space compression (PDX) on arm32 Date: Fri, 12 Sep 2014 16:00:20 -0700 Message-ID: <54137B04.6080907@linaro.org> References: <1410534866.27902.12.camel@kazak.uk.xensource.com> <1410534923-17209-2-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: <1410534923-17209-2-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: Roy Franz , tim@xen.org, Fu Wei , stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, Don't you implement PDX for both arm32 and arm64? If so, you may need to update the commit title. The patch looks good to me. I have only few comments on it. On 12/09/14 08:15, Ian Campbell wrote: > This allows us to support sparse physical address maps which we previously > could not because the frametable would end up taking up an enourmous fraction enormous [..] > - memset(&frame_table[0], 0, nr_pages * sizeof(struct page_info)); > - memset(&frame_table[nr_pages], -1, > - frametable_size - (nr_pages * sizeof(struct page_info))); > + memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info)); > + memset(&frame_table[nr_pdxs], -1, > + frametable_size - (nr_pdxs * sizeof(struct page_info))); > + > + frametable_virt_end = FRAMETABLE_VIRT_START + (nr_pdxs * sizeof(struct page_info)); > NIT: I don't think it's necessary to have a blank line before the end of the block. I would drop it. [..] > +/* Sets all bits from the most-significant 1-bit down to the LSB */ > +static u64 __init fill_mask(u64 mask) > +{ > + while (mask & (mask + 1)) > + mask |= mask + 1; > + return mask; > +} The function is the same on x86. Shall we create an helper? [..] > setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT); You forgot to drop check if ( bank != bootinfo.mem.nr_banks ) which, I think, is not relevant anymore. > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h > index 1c3abcf..59b2887 100644 > --- a/xen/include/asm-arm/config.h > +++ b/xen/include/asm-arm/config.h > @@ -126,7 +126,12 @@ > #define CONFIG_SEPARATE_XENHEAP 1 > > #define FRAMETABLE_VIRT_START _AT(vaddr_t,0x02000000) > -#define VMAP_VIRT_START _AT(vaddr_t,0x10000000) > +#define FRAMETABLE_SIZE MB(128-32) I would add a comment about why the frametable size is "MB(128-32)". Regards, -- Julien Grall