Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
FP/SIMD or not. But currently, this two MACROs only consider value 0
of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
that support FP/SIMD and half-precision floating-point features, the
ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
no FP/SIMD support. In this case, the vfp_save/restore_state will not
take effect.

Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
(see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
platforms, Xen will always miss the float pointer registers save/restore.
If different vCPUs are running on the same pCPU, the float pointer
registers will be corrupted randomly.

This patch fixes Xen on these new cores.

Signed-off-by: Wei Chen <wei.chen@arm.com>
---
 xen/include/asm-arm/cpufeature.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 674beb0353..588089e5ae 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -13,8 +13,8 @@
 #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
 #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
 #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
-#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
-#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
+#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
+#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
 #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
 #endif

--
2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

> On 18 Aug 2020, at 04:11, Wei Chen <Wei.Chen@arm.com> wrote:
> 
> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
> FP/SIMD or not. But currently, this two MACROs only consider value 0
> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
> that support FP/SIMD and half-precision floating-point features, the
> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
> no FP/SIMD support. In this case, the vfp_save/restore_state will not
> take effect.
> 
> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> platforms, Xen will always miss the float pointer registers save/restore.
> If different vCPUs are running on the same pCPU, the float pointer
> registers will be corrupted randomly.
> 
> This patch fixes Xen on these new cores.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I tested this patch on an N1SDP and it solving random crashes issues.

Could we consider applying this to 4.14 ?

> ---
> xen/include/asm-arm/cpufeature.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 674beb0353..588089e5ae 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -13,8 +13,8 @@
> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
> #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
> #endif
> 
> -- 
> 2.17.1
> 

Hi Wei,

On 18/08/2020 04:11, Wei Chen wrote:
> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
> FP/SIMD or not. But currently, this two MACROs only consider value 0

s/this/these/

> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
> that support FP/SIMD and half-precision floating-point features, the
> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
> no FP/SIMD support. In this case, the vfp_save/restore_state will not
> take effect.
> 
> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
> half-precision floatiing-point. 

I am not sure to understand this sentence. Could you clarify?

> Their ID_AA64PFR0_EL1.FP/SMID are 1
> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> platforms, Xen will always miss the float pointer registers save/restore.

s/float pointer/floating point/?

> If different vCPUs are running on the same pCPU, the float pointer

Likewise?

> registers will be corrupted randomly.
> 
> This patch fixes Xen on these new cores.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
>   xen/include/asm-arm/cpufeature.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 674beb0353..588089e5ae 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -13,8 +13,8 @@
>   #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>   #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>   #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>   #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
>   #endif
> 
> --
> 2.17.1
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Please configure your e-mail client to remove the disclaimer.

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2020年8月18日 17:51
> 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>; Steve Capper <Steve.Capper@arm.com>;
> Kaly Xin <Kaly.Xin@arm.com>
> Subject: Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU
> context switch
> 
> Hi Wei,
> 
> On 18/08/2020 04:11, Wei Chen wrote:
> > Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
> > FP/SIMD or not. But currently, this two MACROs only consider value 0
> 
> s/this/these/


Got it

> 
> > of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
> > that support FP/SIMD and half-precision floating-point features, the
> > ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
> > no FP/SIMD support. In this case, the vfp_save/restore_state will not
> > take effect.
> >
> > Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
> > half-precision floatiing-point.
> 
> I am not sure to understand this sentence. Could you clarify?
> 

Sorry about my  unclear express. From the TRM documents (Cortex-A75/A76/N1), I found
their ID_AA64PFR0_EL1.FP and ID_AA64PFR0_EL1.SIMD fields are 0x1.  It's because except
basic Advanced SIMD/FP supports, these CPUs also support half-precision floating-point
arithmetic.

> > Their ID_AA64PFR0_EL1.FP/SMID are 1
> > (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> > platforms, Xen will always miss the float pointer registers save/restore.
> 
> s/float pointer/floating point/?
> 

Yes

> > If different vCPUs are running on the same pCPU, the float pointer
> 
> Likewise?
>

Yes, I will fix these typos in next version.
 
> > registers will be corrupted randomly.
> >
> > This patch fixes Xen on these new cores.
> >
> > Signed-off-by: Wei Chen <wei.chen@arm.com>
> > ---
> >   xen/include/asm-arm/cpufeature.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-
> arm/cpufeature.h
> > index 674beb0353..588089e5ae 100644
> > --- a/xen/include/asm-arm/cpufeature.h
> > +++ b/xen/include/asm-arm/cpufeature.h
> > @@ -13,8 +13,8 @@
> >   #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
> >   #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
> >   #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> > -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> > -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> > +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> > +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
> >   #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
> >   #endif
> >
> > --
> > 2.17.1
> >
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> 
> Please configure your e-mail client to remove the disclaimer.
> 

Thanks for this reminder. I have done.

> Cheers,
> 
> --
> Julien Grall

On 18/08/2020 04:11, Wei Chen wrote:

Hi Wei,

> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
> FP/SIMD or not. But currently, this two MACROs only consider value 0
> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
> that support FP/SIMD and half-precision floating-point features, the
> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
> no FP/SIMD support. In this case, the vfp_save/restore_state will not
> take effect.
> 
> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> platforms, Xen will always miss the float pointer registers save/restore.
> If different vCPUs are running on the same pCPU, the float pointer
> registers will be corrupted randomly.

That's a good catch, thanks for working this out!

One thing below...

> This patch fixes Xen on these new cores.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
>  xen/include/asm-arm/cpufeature.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 674beb0353..588089e5ae 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -13,8 +13,8 @@
>  #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>  #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>  #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)

But this is only good until the next feature bump. I think we should be
more future-proof here. The architecture describes those two fields as
"signed"[1], and guarantees that "if value >= 0" is a valid test for the
feature. Which means we are good as long as the sign bit (bit 3) is
clear, which translates into:
#define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
Same for simd.

Cheers,
Andre.

[1] ARMv8 ARM (ARM DDI 0487F.b), section D13.1.3

>  #define cpu_has_gicv3     (boot_cpu_feature64(gic) == 1)
>  #endif
>  
> 

> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com> wrote:
> 
> On 18/08/2020 04:11, Wei Chen wrote:
> 
> Hi Wei,
> 
>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>> that support FP/SIMD and half-precision floating-point features, the
>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>> take effect.
>> 
>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>> platforms, Xen will always miss the float pointer registers save/restore.
>> If different vCPUs are running on the same pCPU, the float pointer
>> registers will be corrupted randomly.
> 
> That's a good catch, thanks for working this out!
> 
> One thing below...
> 
>> This patch fixes Xen on these new cores.
>> 
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> xen/include/asm-arm/cpufeature.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>> index 674beb0353..588089e5ae 100644
>> --- a/xen/include/asm-arm/cpufeature.h
>> +++ b/xen/include/asm-arm/cpufeature.h
>> @@ -13,8 +13,8 @@
>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
> 
> But this is only good until the next feature bump. I think we should be
> more future-proof here. The architecture describes those two fields as
> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
> feature. Which means we are good as long as the sign bit (bit 3) is
> clear, which translates into:
> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
> Same for simd.
> 

We cannot really be sure that a new version introduced will require the
same context save/restore so it might dangerous to claim we support
something we have no idea about.
I agree though about the analysis on the fact that values under 8 should
be valid but only 0 and 1 currently exist [1], other values are reserved.

So I would vote to keep the 1 for now there.

Cheers
Bertrand

[1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1

Hi Bertrand,

On 18/08/2020 10:25, Bertrand Marquis wrote:
>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>> index 674beb0353..588089e5ae 100644
>>> --- a/xen/include/asm-arm/cpufeature.h
>>> +++ b/xen/include/asm-arm/cpufeature.h
>>> @@ -13,8 +13,8 @@
>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>>
>> But this is only good until the next feature bump. I think we should be
>> more future-proof here. The architecture describes those two fields as
>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>> feature. Which means we are good as long as the sign bit (bit 3) is
>> clear, which translates into:
>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>> Same for simd.
>>
> 
> We cannot really be sure that a new version introduced will require the
> same context save/restore so it might dangerous to claim we support
> something we have no idea about.

Right. However, if we don't do anything for those values, it may be 
possible to see corruption again when it gets bumped.

If we are concerned about incompatibility, then we should start checking 
the features and only allow boot with what we know.

Cheers,

-- 
Julien Grall

On 18/08/2020 10:25, Bertrand Marquis wrote:

Hi,

>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com> wrote:
>>
>> On 18/08/2020 04:11, Wei Chen wrote:
>>
>> Hi Wei,
>>
>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>> that support FP/SIMD and half-precision floating-point features, the
>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>> take effect.
>>>
>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>> platforms, Xen will always miss the float pointer registers save/restore.
>>> If different vCPUs are running on the same pCPU, the float pointer
>>> registers will be corrupted randomly.
>>
>> That's a good catch, thanks for working this out!
>>
>> One thing below...
>>
>>> This patch fixes Xen on these new cores.
>>>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>> ---
>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>> index 674beb0353..588089e5ae 100644
>>> --- a/xen/include/asm-arm/cpufeature.h
>>> +++ b/xen/include/asm-arm/cpufeature.h
>>> @@ -13,8 +13,8 @@
>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>>
>> But this is only good until the next feature bump. I think we should be
>> more future-proof here. The architecture describes those two fields as
>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>> feature. Which means we are good as long as the sign bit (bit 3) is
>> clear, which translates into:
>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>> Same for simd.
>>
> 
> We cannot really be sure that a new version introduced will require the
> same context save/restore so it might dangerous to claim we support
> something we have no idea about.

I am pretty sure we can, because this is what the FP feature describes.
If a feature bump would introduce a larger state to be saved and
restored, that would be covered by a new field, look at AdvSIMD and SVE
for examples.
The feature number would only be bumped if it's compatible:
====================
· The field holds a signed value.
· The field value 0xF indicates that the feature is not implemented.
· The field value 0x0 indicates that the feature is implemented.
· Software that depends on the feature can use the test:
      if value >= 0 {  // Software features that depend on the presence
of the hardware feature }
====================
(ARMv8 ARM D13.1.3)

And this is how Linux handles this.

Cheers,
Andre

> I agree though about the analysis on the fact that values under 8 should
> be valid but only 0 and 1 currently exist [1], other values are reserved.
> 
> So I would vote to keep the 1 for now there.
> 
> Cheers
> Bertrand
> 
> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1
> 

> On 18 Aug 2020, at 10:42, André Przywara <andre.przywara@arm.com> wrote:
> 
> On 18/08/2020 10:25, Bertrand Marquis wrote:
> 
> Hi,
> 
>>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com> wrote:
>>> 
>>> On 18/08/2020 04:11, Wei Chen wrote:
>>> 
>>> Hi Wei,
>>> 
>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>>> that support FP/SIMD and half-precision floating-point features, the
>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>>> take effect.
>>>> 
>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>>> platforms, Xen will always miss the float pointer registers save/restore.
>>>> If different vCPUs are running on the same pCPU, the float pointer
>>>> registers will be corrupted randomly.
>>> 
>>> That's a good catch, thanks for working this out!
>>> 
>>> One thing below...
>>> 
>>>> This patch fixes Xen on these new cores.
>>>> 
>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>>> ---
>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>>> index 674beb0353..588089e5ae 100644
>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>> @@ -13,8 +13,8 @@
>>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>>> 
>>> But this is only good until the next feature bump. I think we should be
>>> more future-proof here. The architecture describes those two fields as
>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>>> feature. Which means we are good as long as the sign bit (bit 3) is
>>> clear, which translates into:
>>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>>> Same for simd.
>>> 
>> 
>> We cannot really be sure that a new version introduced will require the
>> same context save/restore so it might dangerous to claim we support
>> something we have no idea about.
> 
> I am pretty sure we can, because this is what the FP feature describes.
> If a feature bump would introduce a larger state to be saved and
> restored, that would be covered by a new field, look at AdvSIMD and SVE
> for examples.
> The feature number would only be bumped if it's compatible:
> ====================
> · The field holds a signed value.
> · The field value 0xF indicates that the feature is not implemented.
> · The field value 0x0 indicates that the feature is implemented.
> · Software that depends on the feature can use the test:
>      if value >= 0 {  // Software features that depend on the presence
> of the hardware feature }
> ====================
> (ARMv8 ARM D13.1.3)
> 
> And this is how Linux handles this.

Then changing the code to use <8 should be ok.

Cheers
Bertrand

> 
> Cheers,
> Andre
> 
>> I agree though about the analysis on the fact that values under 8 should
>> be valid but only 0 and 1 currently exist [1], other values are reserved.
>> 
>> So I would vote to keep the 1 for now there.
>> 
>> Cheers
>> Bertrand
>> 
>> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1

On 18/08/2020 11:13, Bertrand Marquis wrote:

Hi,

>> On 18 Aug 2020, at 10:42, André Przywara <andre.przywara@arm.com> wrote:
>>
>> On 18/08/2020 10:25, Bertrand Marquis wrote:
>>
>> Hi,
>>
>>>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com> wrote:
>>>>
>>>> On 18/08/2020 04:11, Wei Chen wrote:
>>>>
>>>> Hi Wei,
>>>>
>>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>>>> that support FP/SIMD and half-precision floating-point features, the
>>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>>>> take effect.
>>>>>
>>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>>>> platforms, Xen will always miss the float pointer registers save/restore.
>>>>> If different vCPUs are running on the same pCPU, the float pointer
>>>>> registers will be corrupted randomly.
>>>>
>>>> That's a good catch, thanks for working this out!
>>>>
>>>> One thing below...
>>>>
>>>>> This patch fixes Xen on these new cores.
>>>>>
>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>>>> ---
>>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>>>> index 674beb0353..588089e5ae 100644
>>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>>> @@ -13,8 +13,8 @@
>>>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>>>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>>>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>>>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>>>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>>>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>>>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>>>>
>>>> But this is only good until the next feature bump. I think we should be
>>>> more future-proof here. The architecture describes those two fields as
>>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>>>> feature. Which means we are good as long as the sign bit (bit 3) is
>>>> clear, which translates into:
>>>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>>>> Same for simd.
>>>>
>>>
>>> We cannot really be sure that a new version introduced will require the
>>> same context save/restore so it might dangerous to claim we support
>>> something we have no idea about.
>>
>> I am pretty sure we can, because this is what the FP feature describes.
>> If a feature bump would introduce a larger state to be saved and
>> restored, that would be covered by a new field, look at AdvSIMD and SVE
>> for examples.
>> The feature number would only be bumped if it's compatible:
>> ====================
>> · The field holds a signed value.
>> · The field value 0xF indicates that the feature is not implemented.
>> · The field value 0x0 indicates that the feature is implemented.
>> · Software that depends on the feature can use the test:
>>      if value >= 0 {  // Software features that depend on the presence
>> of the hardware feature }
>> ====================
>> (ARMv8 ARM D13.1.3)
>>
>> And this is how Linux handles this.
> 
> Then changing the code to use <8 should be ok.

Thanks. Another angle to look at this:
Using "< 8" will never be worse than "<= 1", since we only derive the
existence of the floating point registers from it. The moment we see a 2
in this register field, the "<= 1" would definitely fail to save/restore
the FP registers again. But the ARM ARM guarantees that those registers
are still around (since "value >= 0" hits, so the feature is present, as
shown above).
The theoretical worst case with "< 8" would be that it would not cover
*enough* state, but as described above this will never happen, with this
particular FP/SIMD field.

Cheers,
Andre

>>> I agree though about the analysis on the fact that values under 8 should
>>> be valid but only 0 and 1 currently exist [1], other values are reserved.
>>>
>>> So I would vote to keep the 1 for now there.
>>>
>>> Cheers
>>> Bertrand
>>>
>>> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1
> 

> On 18 Aug 2020, at 12:22, André Przywara <andre.przywara@arm.com> wrote:
> 
> On 18/08/2020 11:13, Bertrand Marquis wrote:
> 
> Hi,
> 
>>> On 18 Aug 2020, at 10:42, André Przywara <andre.przywara@arm.com> wrote:
>>> 
>>> On 18/08/2020 10:25, Bertrand Marquis wrote:
>>> 
>>> Hi,
>>> 
>>>>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com> wrote:
>>>>> 
>>>>> On 18/08/2020 04:11, Wei Chen wrote:
>>>>> 
>>>>> Hi Wei,
>>>>> 
>>>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>>>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>>>>> that support FP/SIMD and half-precision floating-point features, the
>>>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>>>>> take effect.
>>>>>> 
>>>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>>>>> platforms, Xen will always miss the float pointer registers save/restore.
>>>>>> If different vCPUs are running on the same pCPU, the float pointer
>>>>>> registers will be corrupted randomly.
>>>>> 
>>>>> That's a good catch, thanks for working this out!
>>>>> 
>>>>> One thing below...
>>>>> 
>>>>>> This patch fixes Xen on these new cores.
>>>>>> 
>>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>>>>> ---
>>>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>>>>>> index 674beb0353..588089e5ae 100644
>>>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>>>> @@ -13,8 +13,8 @@
>>>>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
>>>>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
>>>>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
>>>>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
>>>>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
>>>>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
>>>>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
>>>>> 
>>>>> But this is only good until the next feature bump. I think we should be
>>>>> more future-proof here. The architecture describes those two fields as
>>>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>>>>> feature. Which means we are good as long as the sign bit (bit 3) is
>>>>> clear, which translates into:
>>>>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
>>>>> Same for simd.
>>>>> 
>>>> 
>>>> We cannot really be sure that a new version introduced will require the
>>>> same context save/restore so it might dangerous to claim we support
>>>> something we have no idea about.
>>> 
>>> I am pretty sure we can, because this is what the FP feature describes.
>>> If a feature bump would introduce a larger state to be saved and
>>> restored, that would be covered by a new field, look at AdvSIMD and SVE
>>> for examples.
>>> The feature number would only be bumped if it's compatible:
>>> ====================
>>> · The field holds a signed value.
>>> · The field value 0xF indicates that the feature is not implemented.
>>> · The field value 0x0 indicates that the feature is implemented.
>>> · Software that depends on the feature can use the test:
>>>     if value >= 0 {  // Software features that depend on the presence
>>> of the hardware feature }
>>> ====================
>>> (ARMv8 ARM D13.1.3)
>>> 
>>> And this is how Linux handles this.
>> 
>> Then changing the code to use <8 should be ok.
> 
> Thanks. Another angle to look at this:
> Using "< 8" will never be worse than "<= 1", since we only derive the
> existence of the floating point registers from it. The moment we see a 2
> in this register field, the "<= 1" would definitely fail to save/restore
> the FP registers again. But the ARM ARM guarantees that those registers
> are still around (since "value >= 0" hits, so the feature is present, as
> shown above).
> The theoretical worst case with "< 8" would be that it would not cover
> *enough* state, but as described above this will never happen, with this
> particular FP/SIMD field.

We could also issue a warning for a “non supported FP/SIMD” if something
else then 0 or 1 shows up so that at least it does not passthrough without
being noticed.

Cheers
Bertrand

> 
> Cheers,
> Andre
> 
>>>> I agree though about the analysis on the fact that values under 8 should
>>>> be valid but only 0 and 1 currently exist [1], other values are reserved.
>>>> 
>>>> So I would vote to keep the 1 for now there.
>>>> 
>>>> Cheers
>>>> Bertrand
>>>> 
>>>> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1

Hi Julien, Andre, Bertrand,

Thanks for your comments. It took me some time to remove the disclaimer message. 
And please see my comments below : )

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Sent: 2020年8月18日 21:42
> To: Andre Przywara <Andre.Przywara@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Xen-devel <xen-
> devel@lists.xenproject.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Julien Grall <julien@xen.org>; Steve Capper <Steve.Capper@arm.com>; Kaly
> Xin <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU
> context switch
> 
> 
> 
> > On 18 Aug 2020, at 12:22, André Przywara <andre.przywara@arm.com>
> wrote:
> >
> > On 18/08/2020 11:13, Bertrand Marquis wrote:
> >
> > Hi,
> >
> >>> On 18 Aug 2020, at 10:42, André Przywara <andre.przywara@arm.com>
> wrote:
> >>>
> >>> On 18/08/2020 10:25, Bertrand Marquis wrote:
> >>>
> >>> Hi,
> >>>
> >>>>> On 18 Aug 2020, at 10:14, André Przywara <andre.przywara@arm.com>
> wrote:
> >>>>>
> >>>>> On 18/08/2020 04:11, Wei Chen wrote:
> >>>>>
> >>>>> Hi Wei,
> >>>>>
> >>>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU
> supports
> >>>>>> FP/SIMD or not. But currently, this two MACROs only consider value
> 0
> >>>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for
> CPUs
> >>>>>> that support FP/SIMD and half-precision floating-point features, the
> >>>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat
> them as
> >>>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will
> not
> >>>>>> take effect.
> >>>>>>
> >>>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD
> and
> >>>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
> >>>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> >>>>>> platforms, Xen will always miss the float pointer registers
> save/restore.
> >>>>>> If different vCPUs are running on the same pCPU, the float pointer
> >>>>>> registers will be corrupted randomly.
> >>>>>
> >>>>> That's a good catch, thanks for working this out!
> >>>>>
> >>>>> One thing below...
> >>>>>
> >>>>>> This patch fixes Xen on these new cores.
> >>>>>>
> >>>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
> >>>>>> ---
> >>>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
> >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-
> arm/cpufeature.h
> >>>>>> index 674beb0353..588089e5ae 100644
> >>>>>> --- a/xen/include/asm-arm/cpufeature.h
> >>>>>> +++ b/xen/include/asm-arm/cpufeature.h
> >>>>>> @@ -13,8 +13,8 @@
> >>>>>> #define cpu_has_el2_64    (boot_cpu_feature64(el2) >= 1)
> >>>>>> #define cpu_has_el3_32    (boot_cpu_feature64(el3) == 2)
> >>>>>> #define cpu_has_el3_64    (boot_cpu_feature64(el3) >= 1)
> >>>>>> -#define cpu_has_fp        (boot_cpu_feature64(fp) == 0)
> >>>>>> -#define cpu_has_simd      (boot_cpu_feature64(simd) == 0)
> >>>>>> +#define cpu_has_fp        (boot_cpu_feature64(fp) <= 1)
> >>>>>> +#define cpu_has_simd      (boot_cpu_feature64(simd) <= 1)
> >>>>>
> >>>>> But this is only good until the next feature bump. I think we should be
> >>>>> more future-proof here. The architecture describes those two fields
> as
> >>>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
> >>>>> feature. Which means we are good as long as the sign bit (bit 3) is
> >>>>> clear, which translates into:
> >>>>> #define cpu_has_fp        (boot_cpu_feature64(fp) < 8)
> >>>>> Same for simd.
> >>>>>
> >>>>
> >>>> We cannot really be sure that a new version introduced will require the
> >>>> same context save/restore so it might dangerous to claim we support
> >>>> something we have no idea about.
> >>>
> >>> I am pretty sure we can, because this is what the FP feature describes.
> >>> If a feature bump would introduce a larger state to be saved and
> >>> restored, that would be covered by a new field, look at AdvSIMD and
> SVE
> >>> for examples.
> >>> The feature number would only be bumped if it's compatible:
> >>> ====================
> >>> · The field holds a signed value.
> >>> · The field value 0xF indicates that the feature is not implemented.
> >>> · The field value 0x0 indicates that the feature is implemented.
> >>> · Software that depends on the feature can use the test:
> >>>     if value >= 0 {  // Software features that depend on the presence
> >>> of the hardware feature }
> >>> ====================
> >>> (ARMv8 ARM D13.1.3)
> >>>
> >>> And this is how Linux handles this.
> >>
> >> Then changing the code to use <8 should be ok.
> >
> > Thanks. Another angle to look at this:
> > Using "< 8" will never be worse than "<= 1", since we only derive the
> > existence of the floating point registers from it. The moment we see a 2
> > in this register field, the "<= 1" would definitely fail to save/restore
> > the FP registers again. But the ARM ARM guarantees that those registers
> > are still around (since "value >= 0" hits, so the feature is present, as
> > shown above).
> > The theoretical worst case with "< 8" would be that it would not cover
> > *enough* state, but as described above this will never happen, with this
> > particular FP/SIMD field.
> 
> We could also issue a warning for a “non supported FP/SIMD” if something
> else then 0 or 1 shows up so that at least it does not passthrough without
> being noticed.
> 

I think we have made up the agreement to use "< 8" in these MACROs : )
The reset is that shall we need to throw any information to notice/warn
user that we detected a feature we haven't met before. In my opinion,
I agree with Bertrand, we shall give some message.

Quote from Arm ARM:
For a field describing some class of functionality:
• The value 0x1 was defined as indicating that item A is present.
• The value 0x2 was defined as indicating that items B and C are present, in addition to item A

If there is a value 0x3 bumped in the future, indicates D is present. Because of "< 8", what xen
Has done for A/B/C can also take effect for 0x3. But what Xen done for A/B/C may not always
cover D required. In this case, I think it valuable to pop some warning message for Xen known
FP/SIMD values. 

> Cheers
> Bertrand
> 
> >
> > Cheers,
> > Andre
> >
> >>>> I agree though about the analysis on the fact that values under 8 should
> >>>> be valid but only 0 and 1 currently exist [1], other values are reserved.
> >>>>
> >>>> So I would vote to keep the 1 for now there.
> >>>>
> >>>> Cheers
> >>>> Bertrand
> >>>>
> >>>> [1] https://developer.arm.com/docs/ddi0595/h/aarch64-system-
> registers/id_aa64pfr0_el1
>