Re: [PATCH v1] xen/arm: Do not allocate pte entries for MAP_SMALL_PAGES

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-03-03 at 13:28 +0530, Vijay Kilari wrote:
> On Tue, Feb 24, 2015 at 6:29 PM, Julien Grall <julien.grall@linaro.org> 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