[Xen-devel,24/27] xen/arm: page: Describe the layout of flags used to update page tables

Message ID 20170814142418.13267-25-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.m.
Currently, the flags used to update page tables (i.e PAGE_HYPERVISOR_*)
only contains the memory attribute index. Follow-up patches will add
more information in it.

At the same time introduce PAGE_AI_MASK to get the memory attribute
index easily.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c          | 2 +-
 xen/include/asm-arm/page.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Andre Przywara Aug. 23, 2017, 2:08 p.m. | #1
Hi,

On 14/08/17 15:24, Julien Grall wrote:
> Currently, the flags used to update page tables (i.e PAGE_HYPERVISOR_*)
> only contains the memory attribute index. Follow-up patches will add
> more information in it.
> 
> At the same time introduce PAGE_AI_MASK to get the memory attribute
> index easily.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I wonder if that should be merged with the next patch, to explain the
reason for it. As it stands now it just applies a mask to some existing
call, which looks a bit suspicious.
But that's just a nit and the patch itself is fine, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  xen/arch/arm/mm.c          | 2 +-
>  xen/include/asm-arm/page.h | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 411fe02842..cd7bcf7aca 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
>                  }
>                  if ( op == RESERVE )
>                      break;
> -                pte = mfn_to_xen_entry(mfn, flags);
> +                pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>                  pte.pt.table = 1;
>                  write_pte(entry, pte);
>                  break;
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d9dac92e73..1bf8e9d012 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -63,6 +63,13 @@
>  #define MAIR0VAL (MAIRVAL & 0xffffffff)
>  #define MAIR1VAL (MAIRVAL >> 32)
>  
> +/*
> + * Layout of the flags used for updating the hypervisor page tables
> + *
> + * [0:2] Memory Attribute Index
> + */
> +#define PAGE_AI_MASK(x) ((x) & 0x7U)
> +
>  #define PAGE_HYPERVISOR         (MT_NORMAL)
>  #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>  #define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>
Julien Grall Aug. 23, 2017, 2:31 p.m. | #2
On 08/23/2017 03:08 PM, Andre Przywara wrote:
> Hi,
> 
> On 14/08/17 15:24, Julien Grall wrote:
>> Currently, the flags used to update page tables (i.e PAGE_HYPERVISOR_*)
>> only contains the memory attribute index. Follow-up patches will add
>> more information in it.
>>
>> At the same time introduce PAGE_AI_MASK to get the memory attribute
>> index easily.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> I wonder if that should be merged with the next patch, to explain the
> reason for it. As it stands now it just applies a mask to some existing
> call, which looks a bit suspicious.
> But that's just a nit and the patch itself is fine, so:

Not really it just documents the current behavior rather than hiding 
what is going on.

Cheers,

> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> Cheers,
> Andre.
> 
>> ---
>>   xen/arch/arm/mm.c          | 2 +-
>>   xen/include/asm-arm/page.h | 7 +++++++
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 411fe02842..cd7bcf7aca 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
>>                   }
>>                   if ( op == RESERVE )
>>                       break;
>> -                pte = mfn_to_xen_entry(mfn, flags);
>> +                pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>>                   pte.pt.table = 1;
>>                   write_pte(entry, pte);
>>                   break;
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index d9dac92e73..1bf8e9d012 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -63,6 +63,13 @@
>>   #define MAIR0VAL (MAIRVAL & 0xffffffff)
>>   #define MAIR1VAL (MAIRVAL >> 32)
>>   
>> +/*
>> + * Layout of the flags used for updating the hypervisor page tables
>> + *
>> + * [0:2] Memory Attribute Index
>> + */
>> +#define PAGE_AI_MASK(x) ((x) & 0x7U)
>> +
>>   #define PAGE_HYPERVISOR         (MT_NORMAL)
>>   #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>>   #define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>>

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 411fe02842..cd7bcf7aca 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1021,7 +1021,7 @@  static int create_xen_entries(enum xenmap_operation op,
                 }
                 if ( op == RESERVE )
                     break;
-                pte = mfn_to_xen_entry(mfn, flags);
+                pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
                 pte.pt.table = 1;
                 write_pte(entry, pte);
                 break;
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d9dac92e73..1bf8e9d012 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -63,6 +63,13 @@ 
 #define MAIR0VAL (MAIRVAL & 0xffffffff)
 #define MAIR1VAL (MAIRVAL >> 32)
 
+/*
+ * Layout of the flags used for updating the hypervisor page tables
+ *
+ * [0:2] Memory Attribute Index
+ */
+#define PAGE_AI_MASK(x) ((x) & 0x7U)
+
 #define PAGE_HYPERVISOR         (MT_NORMAL)
 #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
 #define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)