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.
We throw these messages to break the silence when Xen detected
unknown FP/SIMD IDs to notice user to check.

Signed-off-by: Wei Chen <wei.chen@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/setup.c             | 12 ++++++++++++
 xen/include/asm-arm/cpufeature.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7968cee47d..ef39ce1ec6 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -133,6 +133,18 @@ static void __init processor_id(void)
            cpu_has_simd ? " AdvancedSIMD" : "",
            cpu_has_gicv3 ? " GICv3-SysReg" : "");
 
+    /* Warn user if we find unknown floating-point features */
+    if ( cpu_has_unknown_fp )
+        printk(XENLOG_WARNING "WARNING: Unknown Floating-point ID:%d, "
+               "this may result to corruption on the platform\n",
+               boot_cpu_feature64(fp));
+
+    /* Warn user if we find unknown AdvancedSIMD features */
+    if ( cpu_has_unknown_simd )
+        printk(XENLOG_WARNING "WARNING: Unknown AdvancedSIMD ID:%d, "
+               "this may result to corruption on the platform\n",
+               boot_cpu_feature64(simd));
+
     printk("  Debug Features: %016"PRIx64" %016"PRIx64"\n",
            boot_cpu_data.dbg64.bits[0], boot_cpu_data.dbg64.bits[1]);
     printk("  Auxiliary Features: %016"PRIx64" %016"PRIx64"\n",
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 10878ead8a..a32309986e 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -16,6 +16,8 @@
 #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
 #define cpu_has_simd      (boot_cpu_feature64(simd) < 8)
 #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
+#define cpu_has_unknown_fp   (cpu_has_fp && (boot_cpu_feature64(fp) >= 2))
+#define cpu_has_unknown_simd (cpu_has_simd && (boot_cpu_feature64(simd) >= 2))
 #endif
 
 #define cpu_feature32(c, feat)         ((c)->pfr32.feat)
-- 
2.17.1

Hi,

On 25/08/2020 11:08, 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.
> We throw these messages to break the silence when Xen detected
> unknown FP/SIMD IDs to notice user to check.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

OOI, is this reviewed-by coming from internal review?

> ---
>   xen/arch/arm/setup.c             | 12 ++++++++++++
>   xen/include/asm-arm/cpufeature.h |  2 ++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7968cee47d..ef39ce1ec6 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -133,6 +133,18 @@ static void __init processor_id(void)
>              cpu_has_simd ? " AdvancedSIMD" : "",
>              cpu_has_gicv3 ? " GICv3-SysReg" : "");
>   
> +    /* Warn user if we find unknown floating-point features */
> +    if ( cpu_has_unknown_fp )
> +        printk(XENLOG_WARNING "WARNING: Unknown Floating-point ID:%d, "
> +               "this may result to corruption on the platform\n",
> +               boot_cpu_feature64(fp));
> +
> +    /* Warn user if we find unknown AdvancedSIMD features */
> +    if ( cpu_has_unknown_simd )
> +        printk(XENLOG_WARNING "WARNING: Unknown AdvancedSIMD ID:%d, "
> +               "this may result to corruption on the platform\n",
> +               boot_cpu_feature64(simd));
> +
>       printk("  Debug Features: %016"PRIx64" %016"PRIx64"\n",
>              boot_cpu_data.dbg64.bits[0], boot_cpu_data.dbg64.bits[1]);
>       printk("  Auxiliary Features: %016"PRIx64" %016"PRIx64"\n",
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 10878ead8a..a32309986e 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -16,6 +16,8 @@
>   #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>   #define cpu_has_simd      (boot_cpu_feature64(simd) < 8)
>   #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
> +#define cpu_has_unknown_fp   (cpu_has_fp && (boot_cpu_feature64(fp) >= 2))
> +#define cpu_has_unknown_simd (cpu_has_simd && (boot_cpu_feature64(simd) >= 2))

I would rather prefer if we don't introduce cpu_has_unknown_{fp, simd} 
but open-code directly in the 'if'.

Other than that the code looks ok to me.

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2020年8月25日 19:18
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Andre Przywara <Andre.Przywara@arm.com>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Penny Zheng <Penny.Zheng@arm.com>; Kaly
> Xin <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v3 2/2] xen/arm: Throw messages for unknown FP/SIMD
> implement ID
> 
> Hi,
> 
> On 25/08/2020 11:08, 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.
> > We throw these messages to break the silence when Xen detected
> > unknown FP/SIMD IDs to notice user to check.
> >
> > Signed-off-by: Wei Chen <wei.chen@arm.com>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> OOI, is this reviewed-by coming from internal review?

Ahh. No, I remember Bertrand gave me a reviewed-by in v2, so I picked it.
I had left OSS for a while, and forgot something. If I can't pick it directly, could
you please tell me how can I handle such reviewed-by?

> 
> > ---
> >   xen/arch/arm/setup.c             | 12 ++++++++++++
> >   xen/include/asm-arm/cpufeature.h |  2 ++
> >   2 files changed, 14 insertions(+)
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 7968cee47d..ef39ce1ec6 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -133,6 +133,18 @@ static void __init processor_id(void)
> >              cpu_has_simd ? " AdvancedSIMD" : "",
> >              cpu_has_gicv3 ? " GICv3-SysReg" : "");
> >
> > +    /* Warn user if we find unknown floating-point features */
> > +    if ( cpu_has_unknown_fp )
> > +        printk(XENLOG_WARNING "WARNING: Unknown Floating-point ID:%d, "
> > +               "this may result to corruption on the platform\n",
> > +               boot_cpu_feature64(fp));
> > +
> > +    /* Warn user if we find unknown AdvancedSIMD features */
> > +    if ( cpu_has_unknown_simd )
> > +        printk(XENLOG_WARNING "WARNING: Unknown AdvancedSIMD ID:%d,
> "
> > +               "this may result to corruption on the platform\n",
> > +               boot_cpu_feature64(simd));
> > +
> >       printk("  Debug Features: %016"PRIx64" %016"PRIx64"\n",
> >              boot_cpu_data.dbg64.bits[0], boot_cpu_data.dbg64.bits[1]);
> >       printk("  Auxiliary Features: %016"PRIx64" %016"PRIx64"\n",
> > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-
> arm/cpufeature.h
> > index 10878ead8a..a32309986e 100644
> > --- a/xen/include/asm-arm/cpufeature.h
> > +++ b/xen/include/asm-arm/cpufeature.h
> > @@ -16,6 +16,8 @@
> >   #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
> >   #define cpu_has_simd      (boot_cpu_feature64(simd) < 8)
> >   #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
> > +#define cpu_has_unknown_fp   (cpu_has_fp && (boot_cpu_feature64(fp) >=
> 2))
> > +#define cpu_has_unknown_simd (cpu_has_simd &&
> (boot_cpu_feature64(simd) >= 2))
> 
> I would rather prefer if we don't introduce cpu_has_unknown_{fp, simd}
> but open-code directly in the 'if'.
> 
> Other than that the code looks ok to me.

Thanks, I could address it in v4.

> 
> Cheers,
> 
> --
> Julien Grall

On 25/08/2020 15:29, Wei Chen wrote:
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2020年8月25日 19:18
>> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Andre Przywara <Andre.Przywara@arm.com>; Bertrand Marquis
>> <Bertrand.Marquis@arm.com>; Penny Zheng <Penny.Zheng@arm.com>; Kaly
>> Xin <Kaly.Xin@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH v3 2/2] xen/arm: Throw messages for unknown FP/SIMD
>> implement ID
>>
>> Hi,
>>
>> On 25/08/2020 11:08, 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.
>>> We throw these messages to break the silence when Xen detected
>>> unknown FP/SIMD IDs to notice user to check.
>>>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> OOI, is this reviewed-by coming from internal review?
> 
> Ahh. No, I remember Bertrand gave me a reviewed-by in v2, so I picked it.
> I had left OSS for a while, and forgot something. If I can't pick it directly, could
> you please tell me how can I handle such reviewed-by?

In general reviewed-by implies the code was reviewed carefully. They 
should only be carried to a new version if they changes are very trivial.

You can also ask the reviewer if he/she is happy with the changes you 
will make so you can carry the tag.

Cheers,

-- 
Julien Grall