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

[Julien Grall <julien.grall@linaro.org>]

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