From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES Date: Mon, 09 Mar 2015 14:16:12 +0200 Message-ID: <54FD8F0C.3060308@linaro.org> References: <1425884393-28949-1-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1425884393-28949-1-git-send-email-vijay.kilari@gmail.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: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, keir@xen.org, jbeulich@suse.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, Given the introduction of the new helper, the title looks wrong to me. On 09/03/2015 08:59, vijay.kilari@gmail.com wrote: > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 7d4ba0c..e0be36b 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -827,14 +827,15 @@ static int create_xen_table(lpae_t *entry) > > enum xenmap_operation { > INSERT, > - REMOVE > + REMOVE, > + RESERVE > }; > > static int create_xen_entries(enum xenmap_operation op, > unsigned long virt, > unsigned long mfn, > unsigned long nr_mfns, > - unsigned int ai) > + unsigned int flags) > { > int rc; > unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE; > @@ -859,13 +860,17 @@ static int create_xen_entries(enum xenmap_operation op, > > switch ( op ) { > case INSERT: > + case RESERVE: > if ( third[third_table_offset(addr)].pt.valid ) > { > printk("create_xen_entries: trying to replace an existing mapping addr=%lx mfn=%lx\n", > addr, mfn); > return -EINVAL; > } > - pte = mfn_to_xen_entry(mfn, ai); > + if ( op == RESERVE || !is_pte_present(flags) ) As you have a new operation (only used by populate_pt_range), why do you need to check is_pte_present? > void *vm_alloc(unsigned int nr, unsigned int align) > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 3e7b0ae..f743003 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -61,10 +61,27 @@ > #define DEV_WC BUFFERABLE > #define DEV_CACHED WRITEBACK > > -#define PAGE_HYPERVISOR (WRITEALLOC) > -#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED) > -#define PAGE_HYPERVISOR_WC (DEV_WC) > -#define MAP_SMALL_PAGES PAGE_HYPERVISOR > +#define PAGE_PRESENT (0x1 << 16) > +#define PAGE_NOT_PRESENT (0x0) > + > +/* bit[16] in the below representation can be used to know if > + * PTE entry should be added or not. This is useful > + * when ONLY non-leaf page table entries need to allocated. > + * > + * bits[2:0] of be below represent correponds to AttrIndx[2:0] > + * i.e lpae_t.pt.ai[2:4] > + * > + * For readability purpose MAP_SMALL_PAGES is set with PAGE_NOT_PRESENT > + * though PAGE_NOT_PRESENT is 0. > + */ > + > +#define PAGE_HYPERVISOR (WRITEALLOC | PAGE_PRESENT) > +#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED | PAGE_PRESENT) > +#define PAGE_HYPERVISOR_WC (DEV_WC | PAGE_PRESENT) > +#define MAP_SMALL_PAGES (WRITEALLOC | PAGE_NOT_PRESENT) Again, using WRITEALLOC for MAP_SMALL_PAGES doesn't make any sense. Given that you introduced a new helper populate_pt_range and a new operation (RESERVE), the flags PAGE{,_NOT}_PRESENT seems useless. And, therefore, MAP_SMALL_PAGES could be dropped. Regards, -- Julien Grall