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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Tue, Sep 23, 2014 at 10:01 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2014-09-18 at 17:43 +0530, vijay.kilari@xxxxxxxxx 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel