[for-4.7 0/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[for-4.7 0/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[Julien Grall]

Hello,

This small patch series is a bug fix for Xen 4.7 and should also be backported
to Xen 4.6.

Without it, the faulting IPA reported to memaccess may be wrong.

Regards,

Cc: wei.liu2@citrix.com

Julien Grall (2):
  xen/bitops: Introduce macros to generate mask
  xen/arm: traps: Correctly interpret the content of the register
    HPFAR_EL2

 xen/arch/arm/traps.c            | 11 +++++++++--
 xen/include/asm-arm/processor.h |  7 +++++++
 xen/include/xen/bitops.h        | 11 +++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

The code has been imported from the header include/linux/bitops.h in
Linux v4.6-rc3.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/xen/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index cb56f24..e1a4d93 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -3,6 +3,17 @@
 #include <asm/types.h>
 
 /*
+ * Create a contiguous bitmask starting at bit position @l and ending at
+ * position @h. For example
+ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
+ */
+#define GENMASK(h, l) \
+	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
+
+/*
  * ffs: find first bit set. This is defined the same way as
  * the libc and compiler builtin ffs routines, therefore
  * differs in spirit from the above ffz (man ffs).
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[for-4.7 2/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[Julien Grall]

The register HPFAR_EL2 (resp. HPFAR on arm32) contains the bits [47:12]
(resp. [39:12]) of the faulting IPA. Unlike other registers that represent
an address, the upper bits of the IPA are stored in the register bits
[4:39] (resp. [4:21]).

However, Xen assumes that the register contains the faulting IPA correctly
offsetted. This will result to get a wrong IPA when the fault is happening
during a translation table walk. Note this is only affecting  memaccess.

Introduce a new helper to get the faulting IPA from HPFAR_EL2 and
replace direct read from the register by the helper.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: tamas@tklengyel.com

This is a bug fix for Xen 4.7 and should also be backported to Xen 4.6.
Without this patch, the faulting IPA reported to memaccess may be wrong.

---
 xen/arch/arm/traps.c            | 11 +++++++++--
 xen/include/asm-arm/processor.h |  7 +++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1516abd..5e865cf 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2363,6 +2363,13 @@ done:
     if (first) unmap_domain_page(first);
 }
 
+static inline paddr_t get_faulting_ipa(void)
+{
+    register_t hpfar = READ_SYSREG(HPFAR_EL2);
+
+    return ((paddr_t)(hpfar & HPFAR_MASK) << (12 - 4));
+}
+
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       const union hsr hsr)
 {
@@ -2381,7 +2388,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
         };
 
         if ( hsr.iabt.s1ptw )
-            gpa = READ_SYSREG(HPFAR_EL2);
+            gpa = get_faulting_ipa();
         else
         {
             /*
@@ -2431,7 +2438,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
 #endif
 
     if ( dabt.s1ptw )
-        info.gpa = READ_SYSREG(HPFAR_EL2);
+        info.gpa = get_faulting_ipa();
     else
     {
         rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 7e6eb66..6789cd0 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -565,6 +565,13 @@ union hsr {
 
 #define FSC_LL_MASK    (_AC(0x03,U)<<0)
 
+/* HPFAR_EL2: Hypervisor IPA Fault Address Register */
+#ifdef CONFIG_ARM_64
+#define HPFAR_MASK	GENMASK(39, 4)
+#else
+#define HPFAR_MASK	GENMASK(31, 4)
+#endif
+
 /* Time counter hypervisor control register */
 #define CNTHCTL_EL2_EL1PCTEN (1u<<0) /* Kernel/user access to physical counter */
 #define CNTHCTL_EL2_EL1PCEN  (1u<<1) /* Kernel/user access to CNTP timer regs */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Stefano Stabellini]

On Wed, 13 Apr 2016, Julien Grall wrote:
> The code has been imported from the header include/linux/bitops.h in
> Linux v4.6-rc3.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
>  xen/include/xen/bitops.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index cb56f24..e1a4d93 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -3,6 +3,17 @@
>  #include <asm/types.h>
>  
>  /*
> + * Create a contiguous bitmask starting at bit position @l and ending at
> + * position @h. For example
> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> + */
> +#define GENMASK(h, l) \
> +	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> +
> +#define GENMASK_ULL(h, l) \
> +	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
> +
> +/*
>   * ffs: find first bit set. This is defined the same way as
>   * the libc and compiler builtin ffs routines, therefore
>   * differs in spirit from the above ffz (man ffs).
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Andrew Cooper]

On 13/04/16 16:55, Julien Grall wrote:
> The code has been imported from the header include/linux/bitops.h in
> Linux v4.6-rc3.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/include/xen/bitops.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index cb56f24..e1a4d93 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -3,6 +3,17 @@
>  #include <asm/types.h>
>  
>  /*
> + * Create a contiguous bitmask starting at bit position @l and ending at
> + * position @h. For example
> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> + */
> +#define GENMASK(h, l) \
> +	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> +
> +#define GENMASK_ULL(h, l) \
> +	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))

You should have just a single GENMASK() which works in terms of LL.

Masks must be signed to work correctly when implicitly extended.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 2/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[Stefano Stabellini]

On Wed, 13 Apr 2016, Julien Grall wrote:
> The register HPFAR_EL2 (resp. HPFAR on arm32) contains the bits [47:12]
> (resp. [39:12]) of the faulting IPA. Unlike other registers that represent
> an address, the upper bits of the IPA are stored in the register bits
> [4:39] (resp. [4:21]).
> 
> However, Xen assumes that the register contains the faulting IPA correctly
> offsetted. This will result to get a wrong IPA when the fault is happening
> during a translation table walk. Note this is only affecting  memaccess.
> 
> Introduce a new helper to get the faulting IPA from HPFAR_EL2 and
> replace direct read from the register by the helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: tamas@tklengyel.com
> 
> This is a bug fix for Xen 4.7 and should also be backported to Xen 4.6.
> Without this patch, the faulting IPA reported to memaccess may be wrong.
> 
> ---
>  xen/arch/arm/traps.c            | 11 +++++++++--
>  xen/include/asm-arm/processor.h |  7 +++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1516abd..5e865cf 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2363,6 +2363,13 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +static inline paddr_t get_faulting_ipa(void)
> +{
> +    register_t hpfar = READ_SYSREG(HPFAR_EL2);
> +
> +    return ((paddr_t)(hpfar & HPFAR_MASK) << (12 - 4));
> +}
> +
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        const union hsr hsr)
>  {
> @@ -2381,7 +2388,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>          };
>  
>          if ( hsr.iabt.s1ptw )
> -            gpa = READ_SYSREG(HPFAR_EL2);
> +            gpa = get_faulting_ipa();
>          else
>          {
>              /*
> @@ -2431,7 +2438,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>  #endif
>  
>      if ( dabt.s1ptw )
> -        info.gpa = READ_SYSREG(HPFAR_EL2);
> +        info.gpa = get_faulting_ipa();
>      else
>      {
>          rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 7e6eb66..6789cd0 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -565,6 +565,13 @@ union hsr {
>  
>  #define FSC_LL_MASK    (_AC(0x03,U)<<0)
>  
> +/* HPFAR_EL2: Hypervisor IPA Fault Address Register */
> +#ifdef CONFIG_ARM_64
> +#define HPFAR_MASK	GENMASK(39, 4)
> +#else
> +#define HPFAR_MASK	GENMASK(31, 4)
> +#endif
> +
>  /* Time counter hypervisor control register */
>  #define CNTHCTL_EL2_EL1PCTEN (1u<<0) /* Kernel/user access to physical counter */
>  #define CNTHCTL_EL2_EL1PCEN  (1u<<1) /* Kernel/user access to CNTP timer regs */
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 2/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 881 bytes --]

On Wed, Apr 13, 2016 at 9:55 AM, Julien Grall <julien.grall@arm.com> wrote:

> The register HPFAR_EL2 (resp. HPFAR on arm32) contains the bits [47:12]
> (resp. [39:12]) of the faulting IPA. Unlike other registers that represent
> an address, the upper bits of the IPA are stored in the register bits
> [4:39] (resp. [4:21]).
>
> However, Xen assumes that the register contains the faulting IPA correctly
> offsetted. This will result to get a wrong IPA when the fault is happening
> during a translation table walk. Note this is only affecting  memaccess.
>
> Introduce a new helper to get the faulting IPA from HPFAR_EL2 and
> replace direct read from the register by the helper.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>

Thanks for the fix, I totally missed that. I did notice not getting any
events for translation table-walks so at least now I know why.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1372 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 2/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[André Przywara]

On 13/04/16 16:55, Julien Grall wrote:
> The register HPFAR_EL2 (resp. HPFAR on arm32) contains the bits [47:12]
> (resp. [39:12]) of the faulting IPA. Unlike other registers that represent
> an address, the upper bits of the IPA are stored in the register bits
> [4:39] (resp. [4:21]).
> 
> However, Xen assumes that the register contains the faulting IPA correctly
> offsetted. This will result to get a wrong IPA when the fault is happening
> during a translation table walk. Note this is only affecting  memaccess.
> 
> Introduce a new helper to get the faulting IPA from HPFAR_EL2 and
> replace direct read from the register by the helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

Thanks for the catch!

Cheers,
Andre.

> 
> ---
> Cc: tamas@tklengyel.com
> 
> This is a bug fix for Xen 4.7 and should also be backported to Xen 4.6.
> Without this patch, the faulting IPA reported to memaccess may be wrong.
> 
> ---
>  xen/arch/arm/traps.c            | 11 +++++++++--
>  xen/include/asm-arm/processor.h |  7 +++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1516abd..5e865cf 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2363,6 +2363,13 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +static inline paddr_t get_faulting_ipa(void)
> +{
> +    register_t hpfar = READ_SYSREG(HPFAR_EL2);
> +
> +    return ((paddr_t)(hpfar & HPFAR_MASK) << (12 - 4));
> +}
> +
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        const union hsr hsr)
>  {
> @@ -2381,7 +2388,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>          };
>  
>          if ( hsr.iabt.s1ptw )
> -            gpa = READ_SYSREG(HPFAR_EL2);
> +            gpa = get_faulting_ipa();
>          else
>          {
>              /*
> @@ -2431,7 +2438,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>  #endif
>  
>      if ( dabt.s1ptw )
> -        info.gpa = READ_SYSREG(HPFAR_EL2);
> +        info.gpa = get_faulting_ipa();
>      else
>      {
>          rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 7e6eb66..6789cd0 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -565,6 +565,13 @@ union hsr {
>  
>  #define FSC_LL_MASK    (_AC(0x03,U)<<0)
>  
> +/* HPFAR_EL2: Hypervisor IPA Fault Address Register */
> +#ifdef CONFIG_ARM_64
> +#define HPFAR_MASK	GENMASK(39, 4)
> +#else
> +#define HPFAR_MASK	GENMASK(31, 4)
> +#endif
> +
>  /* Time counter hypervisor control register */
>  #define CNTHCTL_EL2_EL1PCTEN (1u<<0) /* Kernel/user access to physical counter */
>  #define CNTHCTL_EL2_EL1PCEN  (1u<<1) /* Kernel/user access to CNTP timer regs */
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Jan Beulich]

>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>--- a/xen/include/xen/bitops.h
>+++ b/xen/include/xen/bitops.h
>@@ -3,6 +3,17 @@
 >#include <asm/types.h>
 >
 >/*
>+ * Create a contiguous bitmask starting at bit position @l and ending at
>+ * position @h. For example
>+ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>+ */
>+#define GENMASK(h, l) \
>+    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>+
>+#define GENMASK_ULL(h, l) \
>+    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))

Irrespective of Linux perhaps considering them useful, I'm not sure they
are (and ISTR these macros having got proposed before). Plus - I don't
think we even have BITS_PER_LONG_LONG anywhere.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Andrew,

On 13/04/2016 19:14, Andrew Cooper wrote:
> On 13/04/16 16:55, Julien Grall wrote:
>> The code has been imported from the header include/linux/bitops.h in
>> Linux v4.6-rc3.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>> ---
>>   xen/include/xen/bitops.h | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
>> index cb56f24..e1a4d93 100644
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -3,6 +3,17 @@
>>   #include <asm/types.h>
>>
>>   /*
>> + * Create a contiguous bitmask starting at bit position @l and ending at
>> + * position @h. For example
>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>> + */
>> +#define GENMASK(h, l) \
>> +	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>> +
>> +#define GENMASK_ULL(h, l) \
>> +	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>
> You should have just a single GENMASK() which works in terms of LL.
>
> Masks must be signed to work correctly when implicitly extended.

May I ask you why? AFAICT, if the mask is signed, it will result to 
duplicate the sign bit to the new bits. This is not the expected 
behavior of this macro.

For instance, the following patch is using this macro to mask RES0 bits 
in the HPFAR_EL2 register. For ARM, RES0 means the bit is currently read 
as zero but the software should not rely on it to preserve forward 
compatibility.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Jan,

On 14/04/2016 05:01, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -3,6 +3,17 @@
>   >#include <asm/types.h>
>   >
>   >/*
>> + * Create a contiguous bitmask starting at bit position @l and ending at
>> + * position @h. For example
>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>> + */
>> +#define GENMASK(h, l) \
>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>> +
>> +#define GENMASK_ULL(h, l) \
>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>
> Irrespective of Linux perhaps considering them useful, I'm not sure they
> are (and ISTR these macros having got proposed before).

This is useful on ARM to generate mask for register. For instance, the 
following patch introduces mask for the register HPFAR_EL2. Only the 
bits [4:39] are usable, the rest are RES0.

For ARM, RES0 means the bit is currently read as zero but the software 
should not rely on it to preserve forward compatibility. So we want to 
mask those bits to avoid breakage with new version of the architecture.

 > Plus - I don't
> think we even have BITS_PER_LONG_LONG anywhere.

Hmmm right, we don't have it. I can drop GENMASK_ULL as I only need 
GENMASK for the moment.

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Jan Beulich]

>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>On 14/04/2016 05:01, Jan Beulich wrote:
>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -3,6 +3,17 @@
>>   >#include <asm/types.h>
>>   >
>>   >/*
>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>> + * position @h. For example
>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>> + */
>>> +#define GENMASK(h, l) \
>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>> +
>>> +#define GENMASK_ULL(h, l) \
>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>
>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>> are (and ISTR these macros having got proposed before).
>
>This is useful on ARM to generate mask for register. For instance, the 
>following patch introduces mask for the register HPFAR_EL2. Only the 
>bits [4:39] are usable, the rest are RES0.
>
>For ARM, RES0 means the bit is currently read as zero but the software 
>should not rely on it to preserve forward compatibility. So we want to 
>mask those bits to avoid breakage with new version of the architecture.

 All understood and needed on every kind of architecture. Yet what's wrong
with expressing this is as 0xfffffffff0, as is being done most everywhere else?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Jan,

On 14/04/16 15:56, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>> On 14/04/2016 05:01, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>>> --- a/xen/include/xen/bitops.h
>>>> +++ b/xen/include/xen/bitops.h
>>>> @@ -3,6 +3,17 @@
>>>    >#include <asm/types.h>
>>>    >
>>>    >/*
>>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>>> + * position @h. For example
>>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>>> + */
>>>> +#define GENMASK(h, l) \
>>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>>> +
>>>> +#define GENMASK_ULL(h, l) \
>>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>>
>>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>>> are (and ISTR these macros having got proposed before).
>>
>> This is useful on ARM to generate mask for register. For instance, the
>> following patch introduces mask for the register HPFAR_EL2. Only the
>> bits [4:39] are usable, the rest are RES0.
>>
>> For ARM, RES0 means the bit is currently read as zero but the software
>> should not rely on it to preserve forward compatibility. So we want to
>> mask those bits to avoid breakage with new version of the architecture.
>
>   All understood and needed on every kind of architecture. Yet what's wrong
> with expressing this is as 0xfffffffff0, as is being done most everywhere else?

It is less intuitive to read and it is easier to make a mistake in the mask.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Jan Beulich]

>>> Julien Grall <julien.grall@arm.com> 04/14/16 5:08 PM >>>
>On 14/04/16 15:56, Jan Beulich wrote:
>>>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>>> On 14/04/2016 05:01, Jan Beulich wrote:
>>>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>>>> --- a/xen/include/xen/bitops.h
>>>>> +++ b/xen/include/xen/bitops.h
>>>>> @@ -3,6 +3,17 @@
>>>>    >#include <asm/types.h>
>>>>    >
>>>>    >/*
>>>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>>>> + * position @h. For example
>>>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>>>> + */
>>>>> +#define GENMASK(h, l) \
>>>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>>>> +
>>>>> +#define GENMASK_ULL(h, l) \
>>>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>>>
>>>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>>>> are (and ISTR these macros having got proposed before).
>>>
>>> This is useful on ARM to generate mask for register. For instance, the
>>> following patch introduces mask for the register HPFAR_EL2. Only the
>>> bits [4:39] are usable, the rest are RES0.
>>>
>>> For ARM, RES0 means the bit is currently read as zero but the software
>>> should not rely on it to preserve forward compatibility. So we want to
>>> mask those bits to avoid breakage with new version of the architecture.
>>
>>   All understood and needed on every kind of architecture. Yet what's wrong
>> with expressing this is as 0xfffffffff0, as is being done most everywhere else?
>
>It is less intuitive to read and it is easier to make a mistake in the mask.

Well, I guess that depends on the person reading/writing the respective piece
of code. To me the macroized form would at least require getting used to.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 0/2] xen/arm: traps: Correctly interpret the content of the register HPFAR_EL2

[Wei Liu]

On Wed, Apr 13, 2016 at 04:55:29PM +0100, Julien Grall wrote:
> Hello,
> 
> This small patch series is a bug fix for Xen 4.7 and should also be backported
> to Xen 4.6.
> 
> Without it, the faulting IPA reported to memaccess may be wrong.
> 
> Regards,
> 
> Cc: wei.liu2@citrix.com
> 
> Julien Grall (2):
>   xen/bitops: Introduce macros to generate mask
>   xen/arm: traps: Correctly interpret the content of the register
>     HPFAR_EL2

The second patch is a genuine bug fix, so 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> 
>  xen/arch/arm/traps.c            | 11 +++++++++--
>  xen/include/asm-arm/processor.h |  7 +++++++
>  xen/include/xen/bitops.h        | 11 +++++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Jan,

On 14/04/16 16:23, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/14/16 5:08 PM >>>
>> On 14/04/16 15:56, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>>>> On 14/04/2016 05:01, Jan Beulich wrote:
>>>>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>>>>> --- a/xen/include/xen/bitops.h
>>>>>> +++ b/xen/include/xen/bitops.h
>>>>>> @@ -3,6 +3,17 @@
>>>>>     >#include <asm/types.h>
>>>>>     >
>>>>>     >/*
>>>>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>>>>> + * position @h. For example
>>>>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>>>>> + */
>>>>>> +#define GENMASK(h, l) \
>>>>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>>>>> +
>>>>>> +#define GENMASK_ULL(h, l) \
>>>>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>>>>
>>>>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>>>>> are (and ISTR these macros having got proposed before).
>>>>
>>>> This is useful on ARM to generate mask for register. For instance, the
>>>> following patch introduces mask for the register HPFAR_EL2. Only the
>>>> bits [4:39] are usable, the rest are RES0.
>>>>
>>>> For ARM, RES0 means the bit is currently read as zero but the software
>>>> should not rely on it to preserve forward compatibility. So we want to
>>>> mask those bits to avoid breakage with new version of the architecture.
>>>
>>>    All understood and needed on every kind of architecture. Yet what's wrong
>>> with expressing this is as 0xfffffffff0, as is being done most everywhere else?
>>
>> It is less intuitive to read and it is easier to make a mistake in the mask.
>
> Well, I guess that depends on the person reading/writing the respective piece
> of code. To me the macroized form would at least require getting used to.

It is a matter of taste. Is there any reason to not allow different way 
to create a mask?

In the case of ARM, the macro version helps to find quickly if a mask 
matches the ARM specification. It is also less error-prone with 64-bit mask.

If you are concerned to have this macro in the common, I would be fine 
to carry it in asm-arm/.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Jan Beulich]

>>> Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
>It is a matter of taste.

Indeed.

> Is there any reason to not allow different way to create a mask?

I dislike it, but not so much to stand in the way to get it in. I.e. I'm not going
to NAK it, but I'm also not currently planning to ACK it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Jan,

On 20/04/16 17:43, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
>> It is a matter of taste.
>
> Indeed.
>
>> Is there any reason to not allow different way to create a mask?
>
> I dislike it, but not so much to stand in the way to get it in. I.e. I'm not going
> to NAK it, but I'm also not currently planning to ACK it.

Stefano, who is now "REST maintainers", acked this patch few days. I 
guess this could be considered as valid ack.

As this series has been release-acked by Wei and acked by Stefano, can a 
committer apply this series to master?

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Stefano Stabellini]

On Fri, 22 Apr 2016, Julien Grall wrote:
> Hi Jan,
> 
> On 20/04/16 17:43, Jan Beulich wrote:
> > > > > Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
> > > It is a matter of taste.
> > 
> > Indeed.
> > 
> > > Is there any reason to not allow different way to create a mask?
> > 
> > I dislike it, but not so much to stand in the way to get it in. I.e. I'm not
> > going
> > to NAK it, but I'm also not currently planning to ACK it.
> 
> Stefano, who is now "REST maintainers", acked this patch few days. I guess
> this could be considered as valid ack.
> 
> As this series has been release-acked by Wei and acked by Stefano, can a
> committer apply this series to master?
 
Please resubmit, dropping or fixing GENMASK_ULL 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Julien Grall]

Hi Stefano,

On 22/04/16 12:49, Stefano Stabellini wrote:
> On Fri, 22 Apr 2016, Julien Grall wrote:
>> Hi Jan,
>>
>> On 20/04/16 17:43, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
>>>> It is a matter of taste.
>>>
>>> Indeed.
>>>
>>>> Is there any reason to not allow different way to create a mask?
>>>
>>> I dislike it, but not so much to stand in the way to get it in. I.e. I'm not
>>> going
>>> to NAK it, but I'm also not currently planning to ACK it.
>>
>> Stefano, who is now "REST maintainers", acked this patch few days. I guess
>> this could be considered as valid ack.
>>
>> As this series has been release-acked by Wei and acked by Stefano, can a
>> committer apply this series to master?
>
> Please resubmit, dropping or fixing GENMASK_ULL

Oh right, I forgot about this macro. Can I keep your ack here?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [for-4.7 1/2] xen/bitops: Introduce macros to generate mask

[Stefano Stabellini]

On Fri, 22 Apr 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/04/16 12:49, Stefano Stabellini wrote:
> > On Fri, 22 Apr 2016, Julien Grall wrote:
> > > Hi Jan,
> > > 
> > > On 20/04/16 17:43, Jan Beulich wrote:
> > > > > > > Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
> > > > > It is a matter of taste.
> > > > 
> > > > Indeed.
> > > > 
> > > > > Is there any reason to not allow different way to create a mask?
> > > > 
> > > > I dislike it, but not so much to stand in the way to get it in. I.e. I'm
> > > > not
> > > > going
> > > > to NAK it, but I'm also not currently planning to ACK it.
> > > 
> > > Stefano, who is now "REST maintainers", acked this patch few days. I guess
> > > this could be considered as valid ack.
> > > 
> > > As this series has been release-acked by Wei and acked by Stefano, can a
> > > committer apply this series to master?
> > 
> > Please resubmit, dropping or fixing GENMASK_ULL
> 
> Oh right, I forgot about this macro. Can I keep your ack here?

Sure

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel