From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v3] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES Date: Tue, 10 Mar 2015 11:52:34 +0000 Message-ID: <1425988354.21353.99.camel@citrix.com> References: <1425884393-28949-1-git-send-email-vijay.kilari@gmail.com> <54FD8F0C.3060308@linaro.org> <54FED93E.5010806@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54FED93E.5010806@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: "Keir (Xen.org)" , Vijay Kilari , Stefano Stabellini , Prasun Kapoor , andrew.cooper3@citrix.com, Vijaya Kumar K , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Jan Beulich , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On Tue, 2015-03-10 at 11:45 +0000, Julien Grall wrote: > On 09/03/15 16:08, Vijay Kilari wrote: > > On Mon, Mar 9, 2015 at 5:46 PM, Julien Grall wrote: > >> 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? > > > > map_pages_to_xen() can still take MAP_SMALL_PAGES as flags. > > In future if any common code requires MAP_SMALL_PAGES then, > > this can be used. > > The only usage was in vmap that you removed in this patch... > > Furthermore, we decided to use to introduce populate_pt_range in order > to avoid using MAP_SMALL_PAGES on ARM... > > It's pointless to keep to different way to population page table... > > [..] > > >> > >> And, therefore, MAP_SMALL_PAGES could be dropped. > > > > MAP_SMALL_PAGES is still used in common code esp. EFI code. > > We can remove this provided if we clean up this. But I still think > > MAP_SMALL_PAGES is required to keep equivalent functionality of x86. > > If you looked at the code you would have notice that the code is only > compiled for x86 and would never work for ARM (_PAGE_PAT, _PAGE_PWT... > doesn't exist). Right, I think we should just remove MAP_SMALL_PAGES on ARM and if/when it turns out we need it we can add it back and implement it in the PT creation code. In any case the fix to make vmap_init use the new function should certainly be in a separate patch to anything which is fixing MAP_SMALL_PAGES. Ian.