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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


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