From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v1] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES Date: Tue, 24 Feb 2015 09:38:33 +0000 Message-ID: <54EC4699.8050905@linaro.org> References: <1424264213-30614-1-git-send-email-vijay.kilari@gmail.com> <54E48DAB.5030307@linaro.org> <1424770282.27930.259.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1424770282.27930.259.camel@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 Cc: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 24/02/2015 09:31, Ian Campbell wrote: > On Wed, 2015-02-18 at 13:03 +0000, Julien Grall wrote: >>> + { >>> + pte = mfn_to_xen_entry(mfn, (ai & 0xffff)); >> >> Please introduce a new macro for the mask. > > Better would be a pte_foo accessor, similar (if not identical) to x86's > pte_get_flags. So pte_get_flags(ai) or so. I'm not able to find a such function in x86. Did you intend to mean pte_flags_to_cacheattr? > >> >>> + pte.pt.table = 1; >>> + write_pte(&third[third_table_offset(addr)], pte); >>> + } >>> break; >>> case REMOVE: >>> if ( !third[third_table_offset(addr)].pt.valid ) >>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h >>> index 3e7b0ae..80415b3 100644 >>> --- a/xen/include/asm-arm/page.h >>> +++ b/xen/include/asm-arm/page.h >>> @@ -61,10 +61,16 @@ >>> #define DEV_WC BUFFERABLE >>> #define DEV_CACHED WRITEBACK >>> >>> +/* bit 16 in the Attribute index 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 >>> + */ >>> +#define PTE_INVALID (0x1 << 16) >> >> It makes more sense to introduce a PTE_PRESENT flags compare to >> PTE_INVALID. The former has more meaning that the latter. > > Agreed that PTE_PRESENT is the way to go. Ideally this could all be done > via the lpae_pt_t type, but I suspect that might turn out to be tricky. I don't think it's a good idea to re-use lpae_pt_t type for this purpose. We might decide to introduce flags which won't be set in the final PTE. In another side, using PTE_PRESENT would require to introduce a PAGE_AVAIL0 (or smth similar). > #define PTE_PRESENT ((struct lpae_t){ .pt.present = 1 }).bits > > probably doesn't work, I'm not even sure if this sort of thing is > possible. If not then "#define PTE_PRESET (1ULL<<0)". The attribute index (write-alloc, buferrable...) is using the less significant 3 bits. So I was suggesting to use the top of the word. Regards, -- Julien Grall