Re: [PATCH] xen/arm: do not read MVFR2 on arm32

[Julien Grall <julien@xxxxxxx>]

On 05/01/2021 19:05, Stefano Stabellini wrote:
> MVFR2 is not available on arm32. Don't try to read it. Make MVFR2 arm64
> only.
Not really, MVFR2 is allocated when running in AArch32 mode on Armv8. It 
just doesn't exist on Armv7. See my answer your previous e-mail for more 
details.
> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/cpufeature.c        | 2 ++
>   xen/include/asm-arm/cpufeature.h | 2 +-
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 1f6a85aafe..9e3377eae3 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -150,7 +150,9 @@ void identify_cpu(struct cpuinfo_arm *c)
>   
>           c->mvfr.bits[0] = READ_SYSREG(MVFR0_EL1);
>           c->mvfr.bits[1] = READ_SYSREG(MVFR1_EL1);
> +#ifdef CONFIG_ARM_64
>           c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> +#endif
>   }
>   
>   /*
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 6058744c18..fa20cb493a 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -272,7 +272,7 @@ struct cpuinfo_arm {
>       } isa32;
>   
>       struct {
> -        register_t bits[3];
> +        register_t bits[2 + IS_ENABLED(CONFIG_ARM_64)];
mvfr.bits[2] will be accessed from arch/arm/vcpreg.c, so you will 
provide garbagge to guest if the user happen to run Xen on Arm32 on Armv8.
Given that MVFR2 exists, I think it would be best to keep the definition 
in cpuinfo_arm around and only hardcode the value in cpufeature.c.
Please see my previous e-mail for more rationale on this approach.


Cheers,

--
Julien Grall