Re: [PATCH 1/3] xen/arm: p2m: extend create_p2m_entries to support read-only mapping

[Julien Grall <julien.grall@linaro.org>]

On 11/11/2013 01:23 PM, Ian Campbell wrote:
> On Thu, 2013-10-24 at 10:02 +0100, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/p2m.c         | 19 +++++++++++--------
>>   xen/include/asm-arm/page.h |  5 +++--
>>   2 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 2d09fef..fdbb07b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -111,7 +111,7 @@ static int p2m_create_table(struct domain *d,
>>       clear_page(p);
>>       unmap_domain_page(p);
>>
>> -    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM);
>> +    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, 1);
>
> If this is a bool_t then true/false might be more appropriate? (and all
> the other such too)
>
> I have a weak preference for avoiding such boolean parameters, since it
> is never obvious what a given 1/0/true/false is actually is without
> having to look at the prototype. At least there is only 1 such param
> here (now).
>
> We don't need full p2m typing yet but perhaps we can make a start by
> defining "typdef enum { p2m_ram_rw = N; p2m_ram_ro = M } p2m_type_t" for
> this?

Sounds good. I will update the patch series with that.

>> @@ -213,14 +213,15 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
>>       return e;
>>   }
>>
>> -static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>> +static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> +                                      bool_t rw)
>>   {
>>       paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>>       lpae_t e = (lpae_t) {
>>           .p2m.xn = 0,
>>           .p2m.af = 1,
>>           .p2m.sh = LPAE_SH_OUTER,
>> -        .p2m.write = 1,
>> +        .p2m.write = !!rw,
>
> I don't think the !! is strictly necessary for either a bool_t nor a
> single bit bitfield.

I will use p2m_type_t here and check if the type is equal to p2m_ram_rw.

-- 
Julien Grall