Re: [Xen-devel] [PATCH] xen/arm: remove check for generic timer support for arm64

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Jun 2, 2014 at 7:09 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 06/02/2014 02:30 PM, Ian Campbell wrote:
>> On Mon, 2014-06-02 at 14:23 +0100, Julien Grall wrote:
>>> On 06/02/2014 02:03 PM, Ian Campbell wrote:
>>>> On Mon, 2014-06-02 at 12:13 +0100, Julien Grall wrote:
>>>>> On 06/02/2014 12:09 PM, Ian Campbell wrote:
>>>>>> On Mon, 2014-06-02 at 14:07 +0530, vijay.kilari@xxxxxxxxx wrote:
>>>>>>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>>>>>>
>>>>>>> arm64 always supports generic timer. So check is not required
>>>>>>> for arm64. For platforms which supports only aarch64 mode this
>>>>>>> check always passes and panics
>>>>>>
>>>>>> Ah, because the relevant feature flag/register is 32-bit only.
>>>>>>
>>>>>> I'd prefer to see this done in the cpufeature header as:
>>>>>>         #ifdef CONFIG_ARM_32
>>>>>>         #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
>>>>>>         #else
>>>>>>         #define cpu_has_gentimer  (1)
>>>>>>         #endif
>>>>>> rather than adding #ifdef to the common code. Likewise for any similar
>>>>>> "always on for aarch64" features.
>>>>>
>>>>> AFAIU, the feature flag exists on ARMv8 platform with aarch32 support.
>>>>> So an ifdef may not be the correct solution here.
>>>>
>>>> The flag might exist in the AArch32 feature registers (for compat with
>>>> v7) but AIUI the feature is not actually optional on v8.
>>>
>>> The manual says the ARM Generic Timer is an optional extension to an
>>> ARMv8 implementation.
>>
>> So it does. In that case cpu_has_gentimer should turn into a check of
>> ID_PFR1_EL1.GenTimer for arm64 builds.
>
> This is already the case (without Vijay's patch). But on AArch64-only
> implementation, this register is RAZ.
>
> It looks like ID_PFR{0,1}_EL1 are only used for ARMv7 and ARMv8 which
> support 32-bit.

AFAIK, for arm64, irrespective of whether it supports aarch64 or
aarch32 or both,
generic timer is required. just because arm64 supports aarch32 it is
not required
to check for ID_PFR{0,1}_EL1 register

In foundation model it is working without my patch because it support
aarch32 mode
and ID_PFR{0,1}_EL1 has generic timer information. However  generic timer
is required for aarch64 mode. Hence check is not required for platform that
supports aarch32 mode

>
> Regards,
>
> --
> Julien Grall

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