[PATCH] xen/arm: Use REG_RANK_INDEX macro

[PATCH] xen/arm: Use REG_RANK_INDEX macro

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Use REG_RANK_INDEX macro to compute index to access
vgic ipriority[] and itargets[] for a given irq.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/vgic-v2.c |    7 +++++--
 xen/arch/arm/vgic-v3.c |    3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index ad7e661..1369f78 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -510,7 +510,9 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
     struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
     ASSERT(spin_is_locked(&rank->lock));
 
-    target = vgic_byte_read(rank->v2.itargets[(irq%32)/4], 0, irq % 4);
+    target = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
+                                              irq, DABT_WORD)], 0, irq & 0x3);
+
     /* 1-N SPI should be delivered as pending to all the vcpus in the
      * mask, but here we just return the first vcpu for simplicity and
      * because it would be too slow to do otherwise. */
@@ -526,7 +528,8 @@ static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
     struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
 
     ASSERT(spin_is_locked(&rank->lock));
-    priority = vgic_byte_read(rank->ipriority[(irq%32)/4], 0, irq % 4);
+    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
+                                              irq, DABT_WORD)], 0, irq & 0x3);
 
     return priority;
 }
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c65a56a..ac8cf07 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -940,7 +940,8 @@ static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
     struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
 
     ASSERT(spin_is_locked(&rank->lock));
-    priority = vgic_byte_read(rank->ipriority[(irq%32)/4], 0, irq % 4);
+    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
+                                              irq, DABT_WORD)], 0, irq & 0x3);
 
     return priority;
 }
-- 
1.7.9.5

[RFC PATCH] xen/arm: check on domain type against hardware support

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Some arm64 platforms implement only aarch64 mode. So allow
domains that are only 64-bit

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/arm64/domctl.c |    6 +++++-
 xen/arch/arm/domain_build.c |    7 +++++++
 xen/arch/arm/setup.c        |    7 +++++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 41e2562..b2f64ae 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -11,6 +11,7 @@
 #include <xen/sched.h>
 #include <xen/hypercall.h>
 #include <public/domctl.h>
+#include <asm/cpufeature.h>
 
 static long switch_mode(struct domain *d, enum domain_type type)
 {
@@ -35,7 +36,10 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         switch ( domctl->u.address_size.size )
         {
         case 32:
-            return switch_mode(d, DOMAIN_32BIT);
+            if ( cpu_has_el1_32 )
+                return switch_mode(d, DOMAIN_32BIT);
+            else
+                return -EINVAL;
         case 64:
             return switch_mode(d, DOMAIN_64BIT);
         default:
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 90abc3a..8782185 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -17,6 +17,7 @@
 #include <asm/platform.h>
 #include <asm/psci.h>
 #include <asm/setup.h>
+#include <asm/cpufeature.h>
 
 #include <asm/gic.h>
 #include <xen/irq.h>
@@ -1274,6 +1275,12 @@ int construct_dom0(struct domain *d)
         return rc;
 
 #ifdef CONFIG_ARM_64
+    /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain */
+    if ( (!(cpu_has_el1_32)) && kinfo.type == DOMAIN_32BIT )
+    {
+        printk("Platform does not support 32-bit domain\n");
+        return -EINVAL;
+    }
     d->arch.type = kinfo.type;
 #endif
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2e80b5a..9fbd315 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
     snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
     safe_strcat(*info, s);
 #endif
-    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
-    safe_strcat(*info, s);
+    if ( cpu_has_aarch32 )
+    {
+        snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
+        safe_strcat(*info, s);
+    }
 }
 
 /*
-- 
1.7.9.5

[RFC PATCH] xen/arm: Restricted access to IFSR32_EL2 and FPEXC32_EL2

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

IFSR32_EL1 and FPEXC32_EL1 registers are accessible in
aarch64 mode only if aarch32 mode is support in EL1.
So allow access ti these registers only for 32-bit domains.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/arch/arm/arm64/vfp.c |    6 ++++--
 xen/arch/arm/traps.c     |    4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm64/vfp.c b/xen/arch/arm/arm64/vfp.c
index 3cd2b1b..999a0d5 100644
--- a/xen/arch/arm/arm64/vfp.c
+++ b/xen/arch/arm/arm64/vfp.c
@@ -28,7 +28,8 @@ void vfp_save_state(struct vcpu *v)
 
     v->arch.vfp.fpsr = READ_SYSREG32(FPSR);
     v->arch.vfp.fpcr = READ_SYSREG32(FPCR);
-    v->arch.vfp.fpexc32_el2 = READ_SYSREG32(FPEXC32_EL2);
+    if ( is_32bit_domain(v->domain) )
+        v->arch.vfp.fpexc32_el2 = READ_SYSREG32(FPEXC32_EL2);
 }
 
 void vfp_restore_state(struct vcpu *v)
@@ -56,5 +57,6 @@ void vfp_restore_state(struct vcpu *v)
 
     WRITE_SYSREG32(v->arch.vfp.fpsr, FPSR);
     WRITE_SYSREG32(v->arch.vfp.fpcr, FPCR);
-    WRITE_SYSREG32(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
+    if ( is_32bit_domain(v->domain) )
+        WRITE_SYSREG32(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
 }
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 25fa8a0..cda0523 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -37,6 +37,7 @@
 #include <asm/cpregs.h>
 #include <asm/psci.h>
 #include <asm/mmio.h>
+#include <asm/cpufeature.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -789,7 +790,8 @@ void show_registers(struct cpu_user_regs *regs)
 #else
     ctxt.far = READ_SYSREG(FAR_EL1);
     ctxt.esr_el1 = READ_SYSREG(ESR_EL1);
-    ctxt.ifsr32_el2 = READ_SYSREG(IFSR32_EL2);
+    if ( is_32bit_domain(current->domain) )
+        ctxt.ifsr32_el2 = READ_SYSREG(IFSR32_EL2);
 #endif
     ctxt.vttbr_el2 = READ_SYSREG64(VTTBR_EL2);
 
-- 
1.7.9.5

[RFC PATCH] xen/arm: remove check for generic timer support for arm64

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Information about support for generic support is available in
IDR_PFR1 register in ARMv7. Where as this information is not
available in ARMv8 that supports only aarch64 bit mode.
ARMv8 being always supports generic timer, this check is not
required.

For platforms that support only aarch64 mode, IDR_PFR1 is
not implemented

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/include/asm-arm/cpufeature.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index aec9173..7a6d3de 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -27,7 +27,11 @@
 #define cpu_has_jazelle   (boot_cpu_feature32(jazelle) >= 0)
 #define cpu_has_thumbee   (boot_cpu_feature32(thumbee) == 1)
 
+#ifdef CONFIG_ARM_32
 #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
+#else
+#define cpu_has_gentimer  (1)
+#endif
 #define cpu_has_security  (boot_cpu_feature32(security) > 0)
 
 #endif
-- 
1.7.9.5

Re: [PATCH] xen/arm: Use REG_RANK_INDEX macro

[Ian Campbell]

On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Use REG_RANK_INDEX macro to compute index to access
> vgic ipriority[] and itargets[] for a given irq.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Ian Campbell]

On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> @@ -35,7 +36,10 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          switch ( domctl->u.address_size.size )
>          {
>          case 32:
> -            return switch_mode(d, DOMAIN_32BIT);
> +            if ( cpu_has_el1_32 )
> +                return switch_mode(d, DOMAIN_32BIT);
> +            else
> +                return -EINVAL;

Please can you separates the error checking from the actual work, e.g.:
                        if ( !cpu_has_el1_32 )
                                return -EINVAL;
                        return switch_mode(...)


> @@ -1274,6 +1275,12 @@ int construct_dom0(struct domain *d)
>          return rc;
>  
>  #ifdef CONFIG_ARM_64
> +    /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain */
> +    if ( (!(cpu_has_el1_32)) && kinfo.type == DOMAIN_32BIT )

You can drop the outer brackets in "(!(cpu_has_el1_32))"

> +    {
> +        printk("Platform does not support 32-bit domain\n");
> +        return -EINVAL;
> +    }
>      d->arch.type = kinfo.type;
>  #endif
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2e80b5a..9fbd315 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>      snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
>      safe_strcat(*info, s);
>  #endif
> -    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
> -    safe_strcat(*info, s);
> +    if ( cpu_has_aarch32 )

This is a bit subtle.

On a v8 processor do we need to check that the cpu has 32-bit support
before we look at this register (i.e. is it guaranteed to be
available/sane on a v8 core which has no 32-bit stuff?)

Secondly, the way cpu_has_aarch32 is currently coded it is only actually
checking for the A32 instruction set, not the T32 one.

arm32 Xen runs in A32 so we could possibly ignore that distinction. But
what about on v8, is T32 without A32 allowed? I suspect not, but the way
the docs are worded makes it a bit unclear: they say "Not support in
ARMv8", but I know you are allowed to build a v8 with no AArch32
support, so I suspect they mean "if you do AArch32 you must do both T32
and A32" (which takes me back to the first question...).

> +    {
> +        snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
> +        safe_strcat(*info, s);
> +    }
>  }
>  
>  /*

Re: [RFC PATCH] xen/arm: Restricted access to IFSR32_EL2 and FPEXC32_EL2

[Ian Campbell]

On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> IFSR32_EL1 and FPEXC32_EL1 registers are accessible in
> aarch64 mode only if aarch32 mode is support in EL1.
> So allow access ti these registers only for 32-bit domains.

s/ti/to/

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(I'll fix the typo on commit)

Ian.

Re: [RFC PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Information about support for generic support is available in
> IDR_PFR1 register in ARMv7. Where as this information is not
> available in ARMv8 that supports only aarch64 bit mode.
> ARMv8 being always supports generic timer, this check is not
> required.
> 
> For platforms that support only aarch64 mode, IDR_PFR1 is
> not implemented

I think this answers my question on "check on domain type against
hardware support"...


> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

But this patch is itself OK:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/include/asm-arm/cpufeature.h |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index aec9173..7a6d3de 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -27,7 +27,11 @@
>  #define cpu_has_jazelle   (boot_cpu_feature32(jazelle) >= 0)
>  #define cpu_has_thumbee   (boot_cpu_feature32(thumbee) == 1)
>  
> +#ifdef CONFIG_ARM_32
>  #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> +#else
> +#define cpu_has_gentimer  (1)
> +#endif
>  #define cpu_has_security  (boot_cpu_feature32(security) > 0)
>  
>  #endif

Re: [PATCH] xen/arm: Use REG_RANK_INDEX macro

[Stefano Stabellini]

On Thu, 18 Sep 2014, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Use REG_RANK_INDEX macro to compute index to access
> vgic ipriority[] and itargets[] for a given irq.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/vgic-v2.c |    7 +++++--
>  xen/arch/arm/vgic-v3.c |    3 ++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index ad7e661..1369f78 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -510,7 +510,9 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>      ASSERT(spin_is_locked(&rank->lock));
>  
> -    target = vgic_byte_read(rank->v2.itargets[(irq%32)/4], 0, irq % 4);
> +    target = vgic_byte_read(rank->v2.itargets[REG_RANK_INDEX(8,
> +                                              irq, DABT_WORD)], 0, irq & 0x3);
> +
>      /* 1-N SPI should be delivered as pending to all the vcpus in the
>       * mask, but here we just return the first vcpu for simplicity and
>       * because it would be too slow to do otherwise. */
> @@ -526,7 +528,8 @@ static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>  
>      ASSERT(spin_is_locked(&rank->lock));
> -    priority = vgic_byte_read(rank->ipriority[(irq%32)/4], 0, irq % 4);
> +    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
> +                                              irq, DABT_WORD)], 0, irq & 0x3);
>  
>      return priority;
>  }
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index c65a56a..ac8cf07 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -940,7 +940,8 @@ static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
>      struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
>  
>      ASSERT(spin_is_locked(&rank->lock));
> -    priority = vgic_byte_read(rank->ipriority[(irq%32)/4], 0, irq % 4);
> +    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
> +                                              irq, DABT_WORD)], 0, irq & 0x3);
>  
>      return priority;
>  }
> -- 
> 1.7.9.5
> 

Re: [PATCH] xen/arm: Use REG_RANK_INDEX macro

[Ian Campbell]

On Tue, 2014-09-23 at 17:45 +0100, Stefano Stabellini wrote:
> On Thu, 18 Sep 2014, vijay.kilari@gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > 
> > Use REG_RANK_INDEX macro to compute index to access
> > vgic ipriority[] and itargets[] for a given irq.
> > 
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Applied, thanks.

Re: [RFC PATCH] xen/arm: Restricted access to IFSR32_EL2 and FPEXC32_EL2

[Ian Campbell]

On Tue, 2014-09-23 at 17:32 +0100, Ian Campbell wrote:
> On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > 
> > IFSR32_EL1 and FPEXC32_EL1 registers are accessible in
> > aarch64 mode only if aarch32 mode is support in EL1.
> > So allow access ti these registers only for 32-bit domains.
> 
> s/ti/to/
> 
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied.

> (I'll fix the typo on commit)

done.

Re: [RFC PATCH] xen/arm: remove check for generic timer support for arm64

[Ian Campbell]

On Tue, 2014-09-23 at 17:33 +0100, Ian Campbell wrote:
> On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > 
> > Information about support for generic support is available in
> > IDR_PFR1 register in ARMv7. Where as this information is not
> > available in ARMv8 that supports only aarch64 bit mode.
> > ARMv8 being always supports generic timer, this check is not
> > required.
> > 
> > For platforms that support only aarch64 mode, IDR_PFR1 is
> > not implemented
> 
> I think this answers my question on "check on domain type against
> hardware support"...
> 
> 
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> But this patch is itself OK:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied. I think that's everything from this series except for " check
on domain type against hardware support".

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Vijay Kilari]

On Tue, Sep 23, 2014 at 10:01 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@gmail.com wrote:
>> @@ -35,7 +36,10 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>          switch ( domctl->u.address_size.size )
>>          {
>>          case 32:
>> -            return switch_mode(d, DOMAIN_32BIT);
>> +            if ( cpu_has_el1_32 )
>> +                return switch_mode(d, DOMAIN_32BIT);
>> +            else
>> +                return -EINVAL;
>
> Please can you separates the error checking from the actual work, e.g.:
>                         if ( !cpu_has_el1_32 )
>                                 return -EINVAL;
>                         return switch_mode(...)
>
>
>> @@ -1274,6 +1275,12 @@ int construct_dom0(struct domain *d)
>>          return rc;
>>
>>  #ifdef CONFIG_ARM_64
>> +    /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain */
>> +    if ( (!(cpu_has_el1_32)) && kinfo.type == DOMAIN_32BIT )
>
> You can drop the outer brackets in "(!(cpu_has_el1_32))"
>
>> +    {
>> +        printk("Platform does not support 32-bit domain\n");
>> +        return -EINVAL;
>> +    }
>>      d->arch.type = kinfo.type;
>>  #endif
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 2e80b5a..9fbd315 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>>      snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
>>      safe_strcat(*info, s);
>>  #endif
>> -    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
>> -    safe_strcat(*info, s);
>> +    if ( cpu_has_aarch32 )
>
> This is a bit subtle.
>
> On a v8 processor do we need to check that the cpu has 32-bit support
> before we look at this register (i.e. is it guaranteed to be
> available/sane on a v8 core which has no 32-bit stuff?)

Here ID_PFR0_EL1 which is Aarch64 register is
mapped ID_PFR0 if Aarch32 is implemented. If Aarch32 is not implemented
this register read is RES0. So check if Aarch32 is not supported at all
then ID_PFR0_EL1 should be 0.

>
> Secondly, the way cpu_has_aarch32 is currently coded it is only actually
> checking for the A32 instruction set, not the T32 one.
>
> arm32 Xen runs in A32 so we could possibly ignore that distinction. But
> what about on v8, is T32 without A32 allowed? I suspect not, but the way
> the docs are worded makes it a bit unclear: they say "Not support in
> ARMv8", but I know you are allowed to build a v8 with no AArch32
> support, so I suspect they mean "if you do AArch32 you must do both T32
> and A32" (which takes me back to the first question...).

From Spec

AArch32:
     Is the 32-bit Execution state, meaning addresses are held in
32-bit registers, and instructions in the
     base instruction sets use 32-bit registers for their processing.
AArch32 state supports the T32 and
     A32 instruction sets.

AArch32:
      AArch32 state supports the following instruction sets:
      A32
           This is a fixed-length instruction set that uses 32-bit
instruction encodings. It is
           compatible with the ARMv7 ARM instruction set.
      T32
          This is a variable-length instruction set that uses both
16-bit and 32-bit instruction
          encodings. It is compatible with the ARMv7 Thumb® instruction set

ID_PFR0 register does not show any dependency with A32 and T32 implementation.
But I think T32 instruction set being subset of A32 instruction set,
A32 is required for T32 support. But I could not find any statement
explicitly mentioning
this in any doc.

I propose cpu_has_aarch32 to check for both A32 and T32. If any one
(A32/T32) is set we
can assume that aarch32 is supported. For ARM64 that supports only
Aarch64 ID_PFR0_EL1 is RES0. so this check for T32 | A32 should work.
Something like this

diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 7a6d3de..379e366 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -22,7 +22,9 @@
 #define boot_cpu_feature32(feat)       (boot_cpu_data.pfr32.feat)

-#define cpu_has_aarch32   (boot_cpu_feature32(arm) == 1)
+#define cpu_has_a32       ((boot_cpu_feature32(arm) == 1)
 #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
+#define cpu_has_aarch32   (cpu_has_a32 || cpu_has_thumb)

Regards
Vijay

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

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Ian Campbell]

On Thu, 2014-09-25 at 14:01 +0530, Vijay Kilari wrote:
> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> >> index 2e80b5a..9fbd315 100644
> >> --- a/xen/arch/arm/setup.c
> >> +++ b/xen/arch/arm/setup.c
> >> @@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
> >>      snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
> >>      safe_strcat(*info, s);
> >>  #endif
> >> -    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
> >> -    safe_strcat(*info, s);
> >> +    if ( cpu_has_aarch32 )
> >
> > This is a bit subtle.
> >
> > On a v8 processor do we need to check that the cpu has 32-bit support
> > before we look at this register (i.e. is it guaranteed to be
> > available/sane on a v8 core which has no 32-bit stuff?)
> 
> Here ID_PFR0_EL1 which is Aarch64 register is
> mapped ID_PFR0 if Aarch32 is implemented. If Aarch32 is not implemented
> this register read is RES0. So check if Aarch32 is not supported at all
> then ID_PFR0_EL1 should be 0.

My concern was that the read would be UNDEFINED or UNPREFICTABLE or
something else unhelpful, but I was looking at the ID_PFR0 section in
the AArch32 part of the ARMv8 ARM, whereas the bit about it being RES0
is under the ID_PFR0_EL1 section in the AArch64 section.

> 
> >
> > Secondly, the way cpu_has_aarch32 is currently coded it is only actually
> > checking for the A32 instruction set, not the T32 one.
> >
> > arm32 Xen runs in A32 so we could possibly ignore that distinction. But
> > what about on v8, is T32 without A32 allowed? I suspect not, but the way
> > the docs are worded makes it a bit unclear: they say "Not support in
> > ARMv8", but I know you are allowed to build a v8 with no AArch32
> > support, so I suspect they mean "if you do AArch32 you must do both T32
> > and A32" (which takes me back to the first question...).
> 
> From Spec
> 
[...]
Thanks, I understood this to be the case already.

> ID_PFR0 register does not show any dependency with A32 and T32 implementation.
> But I think T32 instruction set being subset of A32 instruction set,
> A32 is required for T32 support.

AFAIK T32 is not a subset of A32, it's essentially a completely separate
encoding. For v7 (and earlier) I believe it is possible (and not
unheardof) to implement only one and not the other (although they aren't
called T32 and A32 in v7 I believe they are essentially the same thing
modulo some minor v8 updates as the v7 Thumb and ARM instructions).

>  But I could not find any statement
> explicitly mentioning this in any doc.

WRT v8 AArch32, I couldn't either. Best to assume they are independent I
think.

> I propose cpu_has_aarch32 to check for both A32 and T32. If any one
> (A32/T32) is set we
> can assume that aarch32 is supported. For ARM64 that supports only
> Aarch64 ID_PFR0_EL1 is RES0. so this check for T32 | A32 should work.
> Something like this
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 7a6d3de..379e366 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -22,7 +22,9 @@
>  #define boot_cpu_feature32(feat)       (boot_cpu_data.pfr32.feat)
> 
> -#define cpu_has_aarch32   (boot_cpu_feature32(arm) == 1)
> +#define cpu_has_a32       ((boot_cpu_feature32(arm) == 1)
>  #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
> +#define cpu_has_aarch32   (cpu_has_a32 || cpu_has_thumb)

Yes, this looks like what is needed, thanks.

Ian.

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Julien Grall]

Hi Ian,

Sorry, I'm a bit late to answer to this mail.

On 09/25/2014 12:08 PM, Ian Campbell wrote:
> On Thu, 2014-09-25 at 14:01 +0530, Vijay Kilari wrote:
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index 2e80b5a..9fbd315 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>>>>      snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
>>>>      safe_strcat(*info, s);
>>>>  #endif
>>>> -    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
>>>> -    safe_strcat(*info, s);
>>>> +    if ( cpu_has_aarch32 )
>>>
>>> This is a bit subtle.
>>>
>>> On a v8 processor do we need to check that the cpu has 32-bit support
>>> before we look at this register (i.e. is it guaranteed to be
>>> available/sane on a v8 core which has no 32-bit stuff?)
>>
>> Here ID_PFR0_EL1 which is Aarch64 register is
>> mapped ID_PFR0 if Aarch32 is implemented. If Aarch32 is not implemented
>> this register read is RES0. So check if Aarch32 is not supported at all
>> then ID_PFR0_EL1 should be 0.
> 
> My concern was that the read would be UNDEFINED or UNPREFICTABLE or
> something else unhelpful, but I was looking at the ID_PFR0 section in
> the AArch32 part of the ARMv8 ARM, whereas the bit about it being RES0
> is under the ID_PFR0_EL1 section in the AArch64 section.
> 
>>
>>>
>>> Secondly, the way cpu_has_aarch32 is currently coded it is only actually
>>> checking for the A32 instruction set, not the T32 one.
>>>
>>> arm32 Xen runs in A32 so we could possibly ignore that distinction. But
>>> what about on v8, is T32 without A32 allowed? I suspect not, but the way
>>> the docs are worded makes it a bit unclear: they say "Not support in
>>> ARMv8", but I know you are allowed to build a v8 with no AArch32
>>> support, so I suspect they mean "if you do AArch32 you must do both T32
>>> and A32" (which takes me back to the first question...).
>>
>> From Spec
>>
> [...]
> Thanks, I understood this to be the case already.
> 
>> ID_PFR0 register does not show any dependency with A32 and T32 implementation.
>> But I think T32 instruction set being subset of A32 instruction set,
>> A32 is required for T32 support.
> 
> AFAIK T32 is not a subset of A32, it's essentially a completely separate
> encoding. For v7 (and earlier) I believe it is possible (and not
> unheardof) to implement only one and not the other (although they aren't
> called T32 and A32 in v7 I believe they are essentially the same thing
> modulo some minor v8 updates as the v7 Thumb and ARM instructions).

I just read the ARMv8 spec, AFAIU, if you implement aarch32 support you
have to support both ARM and Thumb instruction (see ID_PFR0).

The other value are marked as "Not supported in ARMv8".

For ARMv7, if the processor doesn't support Thumb we will already in
trouble because Xen is built for ARM instruction.

We have some assumption about it (see arch/arm/traps.c).

>>  But I could not find any statement
>> explicitly mentioning this in any doc.
> 
> WRT v8 AArch32, I couldn't either. Best to assume they are independent I
> think.
> 
>> I propose cpu_has_aarch32 to check for both A32 and T32. If any one
>> (A32/T32) is set we
>> can assume that aarch32 is supported. For ARM64 that supports only
>> Aarch64 ID_PFR0_EL1 is RES0. so this check for T32 | A32 should work.
>> Something like this
>>
>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>> index 7a6d3de..379e366 100644
>> --- a/xen/include/asm-arm/cpufeature.h
>> +++ b/xen/include/asm-arm/cpufeature.h
>> @@ -22,7 +22,9 @@
>>  #define boot_cpu_feature32(feat)       (boot_cpu_data.pfr32.feat)
>>
>> -#define cpu_has_aarch32   (boot_cpu_feature32(arm) == 1)
>> +#define cpu_has_a32       ((boot_cpu_feature32(arm) == 1)
>>  #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
>> +#define cpu_has_aarch32   (cpu_has_a32 || cpu_has_thumb)
> 
> Yes, this looks like what is needed, thanks.

Following my comment above, I don't think this change is necessary.

Regards,

-- 
Julien Grall

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Ian Campbell]

On Mon, 2014-09-29 at 13:18 +0100, Julien Grall wrote:
> Hi Ian,
> 
> Sorry, I'm a bit late to answer to this mail.
> 
> On 09/25/2014 12:08 PM, Ian Campbell wrote:
> > On Thu, 2014-09-25 at 14:01 +0530, Vijay Kilari wrote:
> >>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> >>>> index 2e80b5a..9fbd315 100644
> >>>> --- a/xen/arch/arm/setup.c
> >>>> +++ b/xen/arch/arm/setup.c
> >>>> @@ -883,8 +883,11 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
> >>>>      snprintf(s, sizeof(s), "xen-%d.%d-aarch64 ", major, minor);
> >>>>      safe_strcat(*info, s);
> >>>>  #endif
> >>>> -    snprintf(s, sizeof(s), "xen-%d.%d-armv7l ", major, minor);
> >>>> -    safe_strcat(*info, s);
> >>>> +    if ( cpu_has_aarch32 )
> >>>
> >>> This is a bit subtle.
> >>>
> >>> On a v8 processor do we need to check that the cpu has 32-bit support
> >>> before we look at this register (i.e. is it guaranteed to be
> >>> available/sane on a v8 core which has no 32-bit stuff?)
> >>
> >> Here ID_PFR0_EL1 which is Aarch64 register is
> >> mapped ID_PFR0 if Aarch32 is implemented. If Aarch32 is not implemented
> >> this register read is RES0. So check if Aarch32 is not supported at all
> >> then ID_PFR0_EL1 should be 0.
> > 
> > My concern was that the read would be UNDEFINED or UNPREFICTABLE or
> > something else unhelpful, but I was looking at the ID_PFR0 section in
> > the AArch32 part of the ARMv8 ARM, whereas the bit about it being RES0
> > is under the ID_PFR0_EL1 section in the AArch64 section.
> > 
> >>
> >>>
> >>> Secondly, the way cpu_has_aarch32 is currently coded it is only actually
> >>> checking for the A32 instruction set, not the T32 one.
> >>>
> >>> arm32 Xen runs in A32 so we could possibly ignore that distinction. But
> >>> what about on v8, is T32 without A32 allowed? I suspect not, but the way
> >>> the docs are worded makes it a bit unclear: they say "Not support in
> >>> ARMv8", but I know you are allowed to build a v8 with no AArch32
> >>> support, so I suspect they mean "if you do AArch32 you must do both T32
> >>> and A32" (which takes me back to the first question...).
> >>
> >> From Spec
> >>
> > [...]
> > Thanks, I understood this to be the case already.
> > 
> >> ID_PFR0 register does not show any dependency with A32 and T32 implementation.
> >> But I think T32 instruction set being subset of A32 instruction set,
> >> A32 is required for T32 support.
> > 
> > AFAIK T32 is not a subset of A32, it's essentially a completely separate
> > encoding. For v7 (and earlier) I believe it is possible (and not
> > unheardof) to implement only one and not the other (although they aren't
> > called T32 and A32 in v7 I believe they are essentially the same thing
> > modulo some minor v8 updates as the v7 Thumb and ARM instructions).
> 
> I just read the ARMv8 spec, AFAIU, if you implement aarch32 support you
> have to support both ARM and Thumb instruction (see ID_PFR0).
> 
> The other value are marked as "Not supported in ARMv8".
> 
> For ARMv7, if the processor doesn't support Thumb we will already in
> trouble because Xen is built for ARM instruction.
> 
> We have some assumption about it (see arch/arm/traps.c).
> 
> >>  But I could not find any statement
> >> explicitly mentioning this in any doc.
> > 
> > WRT v8 AArch32, I couldn't either. Best to assume they are independent I
> > think.
> > 
> >> I propose cpu_has_aarch32 to check for both A32 and T32. If any one
> >> (A32/T32) is set we
> >> can assume that aarch32 is supported. For ARM64 that supports only
> >> Aarch64 ID_PFR0_EL1 is RES0. so this check for T32 | A32 should work.
> >> Something like this
> >>
> >> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> >> index 7a6d3de..379e366 100644
> >> --- a/xen/include/asm-arm/cpufeature.h
> >> +++ b/xen/include/asm-arm/cpufeature.h
> >> @@ -22,7 +22,9 @@
> >>  #define boot_cpu_feature32(feat)       (boot_cpu_data.pfr32.feat)
> >>
> >> -#define cpu_has_aarch32   (boot_cpu_feature32(arm) == 1)
> >> +#define cpu_has_a32       ((boot_cpu_feature32(arm) == 1)
> >>  #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
> >> +#define cpu_has_aarch32   (cpu_has_a32 || cpu_has_thumb)
> > 
> > Yes, this looks like what is needed, thanks.
> 
> Following my comment above, I don't think this change is necessary.

Iff you are right about v8. You are probably right but it's not 100%
clear. On the other hand checking for what we actually mean has no
downside.

Ian.

Re: [RFC PATCH] xen/arm: check on domain type against hardware support

[Julien Grall]

On 09/29/2014 01:35 PM, Ian Campbell wrote:
>>>> I propose cpu_has_aarch32 to check for both A32 and T32. If any one
>>>> (A32/T32) is set we
>>>> can assume that aarch32 is supported. For ARM64 that supports only
>>>> Aarch64 ID_PFR0_EL1 is RES0. so this check for T32 | A32 should work.
>>>> Something like this
>>>>
>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>>> index 7a6d3de..379e366 100644
>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>> @@ -22,7 +22,9 @@
>>>>  #define boot_cpu_feature32(feat)       (boot_cpu_data.pfr32.feat)
>>>>
>>>> -#define cpu_has_aarch32   (boot_cpu_feature32(arm) == 1)
>>>> +#define cpu_has_a32       ((boot_cpu_feature32(arm) == 1)
>>>>  #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
>>>> +#define cpu_has_aarch32   (cpu_has_a32 || cpu_has_thumb)
>>>
>>> Yes, this looks like what is needed, thanks.
>>
>> Following my comment above, I don't think this change is necessary.
> 
> Iff you are right about v8. You are probably right but it's not 100%
> clear. On the other hand checking for what we actually mean has no
> downside.

The downside is name readability for the developer as the term A32 has
been introduced on ARMv8.

With only the ARMv7 spec the name is very confusing. I had to read the
ARMv8 spec to completely understand the name and the different between
aarch32 and a32.

I would at least add a comment on top of cpu_has_a32.

Regards,

Regards,

-- 
Julien Grall