Re: [Xen-devel] [RFC PATCH v3 05/10] arm/p2m: Make PTE helpers publicly available

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

On 06/15/2017 09:53 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 06/15/2017 12:05 PM, Sergej Proskurin wrote:
>> In this commit we make the p2m_* helpers, which access PTE properties in
>> a simplified way, publicly available. This is due to the fact that the
>> helpers will be used in guest_walk.c in one of the following commits.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc:
>> ---
>>   xen/arch/arm/p2m.c        | 23 -----------------------
>>   xen/include/asm-arm/p2m.h | 27 +++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index b7bbea1d81..eecbcdf870 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -46,29 +46,6 @@ static const paddr_t level_masks[] =
>>   static const uint8_t level_orders[] =
>>       { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>>   -static inline bool_t p2m_valid(lpae_t pte)
>> -{
>> -    return pte.p2m.valid;
>> -}
>> -/*
>> - * These two can only be used on L0..L2 ptes because L3 mappings set
>> - * the table bit and therefore these would return the opposite to what
>> - * you would expect.
>> - */
>> -static inline bool_t p2m_table(lpae_t pte)
>> -{
>> -    return p2m_valid(pte) && pte.p2m.table;
>> -}
>> -static inline bool_t p2m_mapping(lpae_t pte)
>> -{
>> -    return p2m_valid(pte) && !pte.p2m.table;
>> -}
>> -
>> -static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>> -{
>> -    return (level < 3) && p2m_mapping(pte);
>> -}
>> -
>>   static void p2m_flush_tlb(struct p2m_domain *p2m);
>>     /* Unlock the flush and do a P2M TLB flush if necessary */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 18c57f936e..8053f2a0cf 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -198,6 +198,33 @@ static inline int p2m_is_write_locked(struct
>> p2m_domain *p2m)
>>       return rw_is_write_locked(&p2m->lock);
>>   }
>>   +/*
>> + * Helpers to lookup properties of ptes.
>> + */
>> +
>> +static inline bool_t p2m_valid(lpae_t pte)
> 
> The name implies they should only be used for stage-2 page table. But
> you are using for other place such as for stage-1 page table.
> 
> This means that they are in the wrong header (they should go in page.h).
> 
> I would actually start to split page.h with lpae.h and
> short-descriptor.h to avoid mixing two type of page table in the same
> header. Note that I would be happy if you keep lpae in page.h for the
> time being. But short-descriptor should definitely go in a separate file.
> 
> I would also rename the helpers you move to lpae_*.
> 
>> +{
>> +    return pte.p2m.valid;
> 
> As you plan to re-use for other things than stage-2 page-table, you use
> pte.walk rather than pte.p2m.
> 
> All those comments applies for the rest of the helpers.
> 
>> +}
>> +/*
>> + * These two can only be used on L0..L2 ptes because L3 mappings set
>> + * the table bit and therefore these would return the opposite to what
>> + * you would expect.
>> + */
>> +static inline bool_t p2m_table(lpae_t pte)
>> +{
>> +    return p2m_valid(pte) && pte.p2m.table;
>> +}
>> +static inline bool_t p2m_mapping(lpae_t pte)
>> +{
>> +    return p2m_valid(pte) && !pte.p2m.table;
>> +}
>> +
>> +static inline bool p2m_is_superpage(lpae_t pte, unsigned int level)
>> +{
>> +    return (level < 3) && p2m_mapping(pte);
>> +}
>> +
>>   /* Look up the MFN corresponding to a domain's GFN. */
>>   mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>>  
> 

Thanks. I have moved the upper helpers to page.h for now and renamed
them to lpae_* helpers as part of my most recent patch version. The
submission will follow soon.

Cheers,
~Sergej

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel