MVFR2 is not available on arm32. Don't try to read it. Make MVFR2 arm64
only.

Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 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;
 };
 
-- 
2.17.1

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@xilinx.com>
> ---
>   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