From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v1] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES Date: Tue, 3 Mar 2015 10:27:58 +0000 Message-ID: <1425378478.24959.71.camel@citrix.com> References: <1424264213-30614-1-git-send-email-vijay.kilari@gmail.com> <54E48DAB.5030307@linaro.org> <1424770282.27930.259.camel@citrix.com> <54EC4699.8050905@linaro.org> <1424773562.27930.301.camel@citrix.com> <54EC75C7.7040209@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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 Cc: Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , Julien Grall , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On Tue, 2015-03-03 at 13:28 +0530, Vijay Kilari wrote: > On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall wrote: > > On 24/02/15 10:26, Ian Campbell wrote: > >> On Tue, 2015-02-24 at 09:38 +0000, Julien Grall wrote: > >>> 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? > >> > >> It's actually get_pte_flags. > >> > >>> In another side, using PTE_PRESENT would require to introduce a > >>> PAGE_AVAIL0 (or smth similar). > >> > >> Why? > > > > If we have only a bit PTE_PRESENT, how do you define MAP_SMALL_PAGES? > > MAP_SMALL_PAGES is already defined as WRITE_ALLOC which occupies lower 3 bits > what is need for PAGE_AVAIL0? > > Below definitions should suffice? I think so. You don't need the ()s around DEV_* and WRITE_ALLOC etc in the PAGE_* definitions. A comment on the format of the bit packing done here would also be useful e.g. "16: present\n2:0: pte attributes field" in a little block comment above this stuff, below the DEV_* stuff. PAGE_PRESENT might be more in keepign that PTE_PRESENT, since this isn't a PTE bit pattern. > -#define PTE_INVALID (0x1 << 16) > +#define PTE_PRESENT (0x1 << 16) > > -#define PAGE_HYPERVISOR (WRITEALLOC) > -#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED) > -#define PAGE_HYPERVISOR_WC (DEV_WC) > -#define MAP_SMALL_PAGES (PAGE_HYPERVISOR | (PTE_INVALID)) > +#define PAGE_HYPERVISOR ((WRITEALLOC) | PTE_PRESENT ) > +#define PAGE_HYPERVISOR_NOCACHE ((DEV_SHARED) | PTE_PRESENT ) > +#define PAGE_HYPERVISOR_WC ((DEV_WC) | PTE_PRESENT ) > +#define MAP_SMALL_PAGES (WRITEALLOC) > + > +#define is_pte_present(x) ((x) & PTE_PRESENT) > +#define get_pte_flags(x) ((x) & 0x7) > > > > > > Anyway, I guess introduce a separate helper would help here. > > > >>>> #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. > >> > >> I was suggesting to use bits 2..4 as in the real PTE, to be more similar > >> to the x86 interpretation of this argument. > > > > I don't think we have to follow how x86 interpret this argument. This is > > just a series of flags and may or may not match a bit in the PTE. > > > > For instance, in the case of MAP_SMALL_PAGES, we don't have to write the > > final PTE. > > > > Regards, > > > > -- > > Julien Grall