[Xen-devel,19/27] xen/arm: page: Clean-up the definition of MAIRVAL

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

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.m.
Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
both hardcoded value. This makes quite difficult to understand the value
written in both registers.

Rework the definition by using value of each attribute shifted by their
associated index.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/page.h | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

Comments

Andre Przywara Aug. 23, 2017, 11:42 a.m. | #1
Hi,

On 14/08/17 15:24, Julien Grall wrote:
> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
> both hardcoded value. This makes quite difficult to understand the value
> written in both registers.
> 
> Rework the definition by using value of each attribute shifted by their
> associated index.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I checked all the bits and encoding against the ARMv8 ARM, they look
correct to me.
However I feel that the attribute renaming patch (20/27) should come
before this one.
However:

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

Cheers,
Andre.

> ---
>  xen/include/asm-arm/page.h | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d7a048b64d..86b227c291 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -22,6 +22,21 @@
>  #define LPAE_SH_INNER         0x3
>  
>  /*
> + * Attribute Indexes.
> + *
> + * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
> + * table entry. They are indexes into the bytes of the MAIR*
> + * registers, as defined above.
> + *
> + */
> +#define MT_UNCACHED      0x0
> +#define MT_BUFFERABLE    0x1
> +#define MT_WRITETHROUGH  0x2
> +#define MT_WRITEBACK     0x3
> +#define MT_DEV_SHARED    0x4
> +#define MT_WRITEALLOC    0x7
> +
> +/*
>   * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
>   * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>   *
> @@ -35,26 +50,18 @@
>   *   reserved         110
>   *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>   *
> - *   MT_DEV_WC        001   (== BUFFERABLE)
>   */
> -#define MAIR0VAL 0xeeaa4400
> -#define MAIR1VAL 0xff000004
> -#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
> +#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
>  
> -/*
> - * Attribute Indexes.
> - *
> - * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
> - * table entry. They are indexes into the bytes of the MAIR*
> - * registers, as defined above.
> - *
> - */
> -#define MT_UNCACHED      0x0
> -#define MT_BUFFERABLE    0x1
> -#define MT_WRITETHROUGH  0x2
> -#define MT_WRITEBACK     0x3
> -#define MT_DEV_SHARED    0x4
> -#define MT_WRITEALLOC    0x7
> +#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
> +                 MAIR(0x44, MT_BUFFERABLE)   | \
> +                 MAIR(0xaa, MT_WRITETHROUGH) | \
> +                 MAIR(0xee, MT_WRITEBACK)    | \
> +                 MAIR(0x04, MT_DEV_SHARED)   | \
> +                 MAIR(0xff, MT_WRITEALLOC))
> +
> +#define MAIR0VAL (MAIRVAL & 0xffffffff)
> +#define MAIR1VAL (MAIRVAL >> 32)
>  
>  #define PAGE_HYPERVISOR         (MT_WRITEALLOC)
>  #define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
>
Julien Grall Aug. 23, 2017, 6:53 p.m. | #2
On 08/23/2017 12:42 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, Julien Grall wrote:
>> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
>> both hardcoded value. This makes quite difficult to understand the value
>> written in both registers.
>>
>> Rework the definition by using value of each attribute shifted by their
>> associated index.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> I checked all the bits and encoding against the ARMv8 ARM, they look
> correct to me.
> However I feel that the attribute renaming patch (20/27) should come
> before this one.

I can invert the two patches. I don't plan to keep your tags in both 
patches as this would sensibly change some bits.

Cheers,

Patch

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d7a048b64d..86b227c291 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -22,6 +22,21 @@ 
 #define LPAE_SH_INNER         0x3
 
 /*
+ * Attribute Indexes.
+ *
+ * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
+ * table entry. They are indexes into the bytes of the MAIR*
+ * registers, as defined above.
+ *
+ */
+#define MT_UNCACHED      0x0
+#define MT_BUFFERABLE    0x1
+#define MT_WRITETHROUGH  0x2
+#define MT_WRITEBACK     0x3
+#define MT_DEV_SHARED    0x4
+#define MT_WRITEALLOC    0x7
+
+/*
  * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
  * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
  *
@@ -35,26 +50,18 @@ 
  *   reserved         110
  *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
  *
- *   MT_DEV_WC        001   (== BUFFERABLE)
  */
-#define MAIR0VAL 0xeeaa4400
-#define MAIR1VAL 0xff000004
-#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
+#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
 
-/*
- * Attribute Indexes.
- *
- * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
- * table entry. They are indexes into the bytes of the MAIR*
- * registers, as defined above.
- *
- */
-#define MT_UNCACHED      0x0
-#define MT_BUFFERABLE    0x1
-#define MT_WRITETHROUGH  0x2
-#define MT_WRITEBACK     0x3
-#define MT_DEV_SHARED    0x4
-#define MT_WRITEALLOC    0x7
+#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
+                 MAIR(0x44, MT_BUFFERABLE)   | \
+                 MAIR(0xaa, MT_WRITETHROUGH) | \
+                 MAIR(0xee, MT_WRITEBACK)    | \
+                 MAIR(0x04, MT_DEV_SHARED)   | \
+                 MAIR(0xff, MT_WRITEALLOC))
+
+#define MAIR0VAL (MAIRVAL & 0xffffffff)
+#define MAIR1VAL (MAIRVAL >> 32)
 
 #define PAGE_HYPERVISOR         (MT_WRITEALLOC)
 #define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)