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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-06-02 at 14:39 +0100, Julien Grall 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).

cpu_has_gentimer uses boot_cpu_feature32 and is therefore only testing
for AArch32 features. It should use boot_cpu_feature64 to check for the
AArch64 feature instead.

>  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.

Yes, these registers contains AArch32 features. AArch64 features are in
ID_AA64PFR0_EL1 et al.

Ian.


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