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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2014-09-29 at 13:18 +0100, Julien Grall wrote:
> 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.

Iff you are right about v8. You are probably right but it's not 100%
clear. On the other hand checking for what we actually mean has no
downside.

Ian.