Re: [PATCH v2 04/10] xen/arm: Store p2m type in each page of the guest

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

On 12/09/2013 04:03 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Use the field 'avail' to store the type of the page. Rename it to 'p2mt' for
>> convenience.
> 
> The p2m is redundant in most uses, and the t isn't very descriptive. I
> think "type" would be fine.
> 
>> The information stored in this field will be retrieved in a future patch to
>> hange the behaviour when the page is removed.
> 
> "change"
> 
>> Also introduce guest_physmap_add_entry to map and set a specific p2m type for
>> a page.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Rename 'avail' field to 'p2mt' in the p2m structure
>>         - Add BUILD_BUG_ON to check if the enum value will fit in the field
>>         - Implement grant mapping type
>> ---
>>  xen/arch/arm/p2m.c         |   58 ++++++++++++++++++++++++++++++++------------
>>  xen/include/asm-arm/p2m.h  |   18 +++++++++++---
>>  xen/include/asm-arm/page.h |    2 +-
>>  3 files changed, 58 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 8f8b47e..e43804c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -124,7 +124,8 @@ int p2m_pod_decrease_reservation(struct domain *d,
>>      return -ENOSYS;
>>  }
>>  
>> -static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>> +static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> +                               p2m_type_t t)
>>  {
>>      paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>>      lpae_t e = (lpae_t) {
>> @@ -132,12 +133,34 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>>          .p2m.af = 1,
>>          .p2m.sh = LPAE_SH_OUTER,
>>          .p2m.read = 1,
>> -        .p2m.write = 1,
>>          .p2m.mattr = mattr,
>>          .p2m.table = 1,
>>          .p2m.valid = 1,
>> +        .p2m.p2mt = t,
>>      };
>>  
>> +    BUILD_BUG_ON(p2m_max_real_type > (2 << 4));
>> +
>> +    switch (t)
>> +    {
>> +    case p2m_grant_map_rw:
>> +        e.p2m.xn = 1;
>> +        /* Fallthrough */
>> +    case p2m_ram_rw:
>> +    case p2m_mmio_direct:
>> +    case p2m_map_foreign:
>> +        e.p2m.write = 1;
>> +        break;
>> +
>> +    case p2m_grant_map_ro:
>> +        e.p2m.xn = 1;
>> +        /* Fallthrough */
>> +    case p2m_invalid:
>> +    case p2m_ram_ro:
>> +    default:
> 
> I think you've enumerated all the options, so it is better to leave out
> the default -- then at least some compilers will complain if we add a
> new type and forget to add it here.
> 

Ok. I will remove it.


-- 
Julien Grall