Re: [PATCH v2 2/2] xen/arm: Throw messages for unknown FP/SIMD implement ID

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

On 24/08/2020 04:28, Wei Chen wrote:
> Arm ID_AA64PFR0_EL1 register provides two fields to describe CPU
> FP/SIMD implementations. Currently, we exactly know the meaning of
> 0x0, 0x1 and 0xf of these fields. Xen treats value < 8 as FP/SIMD
> features presented. If there is a value 0x2 bumped in the future,
> Xen behaviors for value <= 0x1 can also take effect. But what Xen
> done for value <= 0x1 may not always cover new value 0x2 required.
Right, but this will also happen with all the other features. This may 
actually confuse the users as they may think the rest of the features 
are fully supported which is not correct. For instance, dom0 will crash 
if you boot Xen on a SVE-capable hardware.
> We throw these messages to break the silence when Xen detected
> unknown FP/SIMD IDs to notice user to check.
It feels a bit odd to me to print unknown for the FP/SIMD feature but 
not for all the rest.
IMHO, the right approach is to sanitize ID registers exposed to domains 
and only expose features we know are correctly handled.
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   xen/arch/arm/setup.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7968cee47d..c7802d0e49 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -99,6 +99,28 @@ static const char * __initdata processor_implementers[] = {
>       ['i'] = "Intel Corporation",
>   };
>   
> +static const char * __initdata fp_implements[] = {
> +    "Floating-point",
> +    "Floating-point + half-precision floating-point arithmetic",
> +    "Floating-point Unknown ID 0x2",
> +    "Floating-point Unknown ID 0x3",
> +    "Floating-point Unknown ID 0x4",
> +    "Floating-point Unknown ID 0x5",
> +    "Floating-point Unknown ID 0x6",
> +    "Floating-point Unknown ID 0x7",
> +};
> +
> +static const char * __initdata advsimd_implements[] = {
> +    "AdvancedSIMD",
> +    "AdvancedSIMD + half-precision floating-point arithmetic",
> +    "AdvancedSIMD Unknown ID 0x2",
> +    "AdvancedSIMD Unknown ID 0x3",
> +    "AdvancedSIMD Unknown ID 0x4",
> +    "AdvancedSIMD Unknown ID 0x5",
> +    "AdvancedSIMD Unknown ID 0x6",
> +    "AdvancedSIMD Unknown ID 0x7",
> +};
> +
>   static void __init processor_id(void)
>   {
>       const char *implementer = "Unknown";
> @@ -129,8 +151,8 @@ static void __init processor_id(void)
>              cpu_has_el1_32 ? "64+32" : cpu_has_el1_64 ? "64" : "No",
>              cpu_has_el0_32 ? "64+32" : cpu_has_el0_64 ? "64" : "No");
>       printk("    Extensions:%s%s%s\n",
> -           cpu_has_fp ? " FloatingPoint" : "",
> -           cpu_has_simd ? " AdvancedSIMD" : "",
> +           cpu_has_fp ? fp_implements[boot_cpu_feature64(fp)] : "",
> +           cpu_has_simd ? advsimd_implements[boot_cpu_feature64(simd)] : "",
So far, each extension name are just a word and they are all separated 
with spaces. With this change, there will be multiple words per 
extension which is quite confusion.
If we decide to go ahead printing the "unknown", then we want to provide 
a full description of the extension on a separate line. Maybe:
"AdvancedSIMD: With half-precision floating-point arithmetic".


>              cpu_has_gicv3 ? " GICv3-SysReg" : "");
>   
>       printk("  Debug Features: %016"PRIx64" %016"PRIx64"\n",
Cheers,

--
Julien Grall