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

[Julien Grall <julien.grall@linaro.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