Re: [Xen-devel] [PATCH for-4.10] xen/arm: mm: Rework MAIR* definitions to handle 32-bit compilation environment

2017-10-11 Thread Stefano Stabellini 
On Wed, 11 Oct 2017, Julien Grall wrote:
> Commit a0543df403 "xen/arm: page: Clean-up the definition of MAIRVAL"
> combined the definition of MAIR0VAL and MAIR1VAL in MAIRVAL. Sadly, when
> building in 32-bit environment, the assembler is unable to compute
> 64-bit constant and will ignore the 32-bit most-significants bits. This
> will result of MAIR1 set 0.
> 
> Rather than fully reverting the offending commit, the code is reworked
> to still avoid hardcoded values but split the definition in 2.
> 
> Lastly, a comment is added to avoid trying to blindly combine the both
> definition again in the future.
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Stefano Stabellini 

> ---
>  xen/include/asm-arm/page.h | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index f558184e10..d948250a4a 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -52,18 +52,23 @@
>   *   ??   101
>   *   reserved 110
>   *   MT_NORMAL111      -- Write-back write-allocate
> + *
> + * /!\ It is not possible to combine the definition in MAIRVAL and then
> + * split because it would result to a 64-bit value that some assembler
> + * doesn't understand.
>   */
> -#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
> +#define _MAIR0(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
> +#define _MAIR1(attr, mt) (_AC(attr, ULL) << (((mt) * 8) - 32))
> +
> +#define MAIR0VAL (_MAIR0(0x00, MT_DEVICE_nGnRnE)| \
> +  _MAIR0(0x44, MT_NORMAL_NC)| \
> +  _MAIR0(0xaa, MT_NORMAL_WT)| \
> +  _MAIR0(0xee, MT_NORMAL_WB))
>  
> -#define MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \
> - MAIR(0x44, MT_NORMAL_NC)| \
> - MAIR(0xaa, MT_NORMAL_WT)| \
> - MAIR(0xee, MT_NORMAL_WB)| \
> - MAIR(0x04, MT_DEVICE_nGnRE) | \
> - MAIR(0xff, MT_NORMAL))
> +#define MAIR1VAL (_MAIR1(0x04, MT_DEVICE_nGnRE) | \
> +  _MAIR1(0xff, MT_NORMAL))
>  
> -#define MAIR0VAL (MAIRVAL & 0x)
> -#define MAIR1VAL (MAIRVAL >> 32)
> +#define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
>  
>  /*
>   * Layout of the flags used for updating the hypervisor page tables
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH for-4.10] xen/arm: mm: Rework MAIR* definitions to handle 32-bit compilation environment

2017-10-11 Thread Andre Przywara 
Hi,

On 11/10/17 15:15, Julien Grall wrote:
> Commit a0543df403 "xen/arm: page: Clean-up the definition of MAIRVAL"
> combined the definition of MAIR0VAL and MAIR1VAL in MAIRVAL. Sadly, when
> building in 32-bit environment, the assembler is unable to compute
> 64-bit constant and will ignore the 32-bit most-significants bits. This
> will result of MAIR1 set 0.
> 
> Rather than fully reverting the offending commit, the code is reworked
> to still avoid hardcoded values but split the definition in 2.

Nasty issue, but given the circumstances the workaround seems not too
bad for me.

> Lastly, a comment is added to avoid trying to blindly combine the both
> definition again in the future.
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Andre Przywara 

Cheers,
Andre.

> ---
>  xen/include/asm-arm/page.h | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index f558184e10..d948250a4a 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -52,18 +52,23 @@
>   *   ??   101
>   *   reserved 110
>   *   MT_NORMAL111      -- Write-back write-allocate
> + *
> + * /!\ It is not possible to combine the definition in MAIRVAL and then
> + * split because it would result to a 64-bit value that some assembler
> + * doesn't understand.
>   */
> -#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
> +#define _MAIR0(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
> +#define _MAIR1(attr, mt) (_AC(attr, ULL) << (((mt) * 8) - 32))
> +
> +#define MAIR0VAL (_MAIR0(0x00, MT_DEVICE_nGnRnE)| \
> +  _MAIR0(0x44, MT_NORMAL_NC)| \
> +  _MAIR0(0xaa, MT_NORMAL_WT)| \
> +  _MAIR0(0xee, MT_NORMAL_WB))
>  
> -#define MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \
> - MAIR(0x44, MT_NORMAL_NC)| \
> - MAIR(0xaa, MT_NORMAL_WT)| \
> - MAIR(0xee, MT_NORMAL_WB)| \
> - MAIR(0x04, MT_DEVICE_nGnRE) | \
> - MAIR(0xff, MT_NORMAL))
> +#define MAIR1VAL (_MAIR1(0x04, MT_DEVICE_nGnRE) | \
> +  _MAIR1(0xff, MT_NORMAL))
>  
> -#define MAIR0VAL (MAIRVAL & 0x)
> -#define MAIR1VAL (MAIRVAL >> 32)
> +#define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
>  
>  /*
>   * Layout of the flags used for updating the hypervisor page tables
> 

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

[Xen-devel] [PATCH for-4.10] xen/arm: mm: Rework MAIR* definitions to handle 32-bit compilation environment

2017-10-11 Thread Julien Grall 
Commit a0543df403 "xen/arm: page: Clean-up the definition of MAIRVAL"
combined the definition of MAIR0VAL and MAIR1VAL in MAIRVAL. Sadly, when
building in 32-bit environment, the assembler is unable to compute
64-bit constant and will ignore the 32-bit most-significants bits. This
will result of MAIR1 set 0.

Rather than fully reverting the offending commit, the code is reworked
to still avoid hardcoded values but split the definition in 2.

Lastly, a comment is added to avoid trying to blindly combine the both
definition again in the future.

Signed-off-by: Julien Grall 
---
 xen/include/asm-arm/page.h | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index f558184e10..d948250a4a 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -52,18 +52,23 @@
  *   ??   101
  *   reserved 110
  *   MT_NORMAL111      -- Write-back write-allocate
+ *
+ * /!\ It is not possible to combine the definition in MAIRVAL and then
+ * split because it would result to a 64-bit value that some assembler
+ * doesn't understand.
  */
-#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
+#define _MAIR0(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
+#define _MAIR1(attr, mt) (_AC(attr, ULL) << (((mt) * 8) - 32))
+
+#define MAIR0VAL (_MAIR0(0x00, MT_DEVICE_nGnRnE)| \
+  _MAIR0(0x44, MT_NORMAL_NC)| \
+  _MAIR0(0xaa, MT_NORMAL_WT)| \
+  _MAIR0(0xee, MT_NORMAL_WB))
 
-#define MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \
- MAIR(0x44, MT_NORMAL_NC)| \
- MAIR(0xaa, MT_NORMAL_WT)| \
- MAIR(0xee, MT_NORMAL_WB)| \
- MAIR(0x04, MT_DEVICE_nGnRE) | \
- MAIR(0xff, MT_NORMAL))
+#define MAIR1VAL (_MAIR1(0x04, MT_DEVICE_nGnRE) | \
+  _MAIR1(0xff, MT_NORMAL))
 
-#define MAIR0VAL (MAIRVAL & 0x)
-#define MAIR1VAL (MAIRVAL >> 32)
+#define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
 
 /*
  * Layout of the flags used for updating the hypervisor page tables
-- 
2.11.0


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