| Message ID | 20200824032825.18368-3-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix Guest random crash on Cortex-N1/A76/A75 cores | expand |
Hi, > On 24 Aug 2020, at 04:28, Wei Chen <Wei.Chen@arm.com> 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> > --- > 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)] : "", > cpu_has_gicv3 ? " GICv3-SysReg" : ""); > > printk(" Debug Features: %016"PRIx64" %016"PRIx64"\n", > -- > 2.17.1 >
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@arm.com> > --- > 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,
Hi Julien, > On 24 Aug 2020, at 14:34, Julien Grall <julien@xen.org> wrote: > > 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. I would see this as an improvement already. More could be done for SVE (and other bits) but this should be in another patch set. > >> 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. I actually started to look into this last week because I came to an issue comparable to SVE with pointer authentication. Maybe we should discuss this subject separately as clearing TID3 bit in HCR and emulating all ID registers is possible but I want to check first if this could have big impacts on performances and see discuss how to design this as there could be a huge amount of cases for example if we want to allow different parameters for different domains. Cheers Bertrand > >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> --- >> 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
On 24/08/2020 17:57, Bertrand Marquis wrote: > Hi Julien, Hi, > >> On 24 Aug 2020, at 14:34, Julien Grall <julien@xen.org> wrote: >> >> 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. > > I would see this as an improvement already. TBH, I only view this patch as a band-aid. I am OK with them but they need to be useful. When I read "unknown value...", I have no clue whether the message is good or bad. It would be better to extend the message with what could go wrong. E.g "Unknown value X, this may result to corruption on the platform". I would also consider to taint Xen as it may become unstable with this option set. > More could be done for SVE (and other bits) but this should be in another patch set. > >> >>> 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. > > I actually started to look into this last week because I came to an issue comparable to SVE with pointer authentication. > Maybe we should discuss this subject separately as clearing TID3 bit in HCR and emulating all ID registers is possible > but I want to check first if this could have big impacts on performances I don't expect any performance drop. In Linux, they will cache all the values and use alternative to avoid runtime check. I also don't expect any OS to use them at runtime in hotpath. > and see discuss how to design this as there > could be a huge amount of cases for example if we want to allow different parameters for different domains. I would suggest to first consider a simple approach where we expose the same view of the ID registers to all the guests but it is sanitized. We can then discuss whether we want to have a per-guest view of the ID registers and how to configure it. Regarding the implementation, I personally quite like the Linux cpufeature framework. It is quite powerful and I believe would suit quite well Xen. Cheers,
Hi Julien, Bertrand, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2020年8月25日 1:23 > To: Bertrand Marquis <Bertrand.Marquis@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Xen-devel <xen- > devel@lists.xenproject.org>; sstabellini@kernel.org; Andre Przywara > <Andre.Przywara@arm.com>; Penny Zheng <Penny.Zheng@arm.com>; Kaly > Xin <Kaly.Xin@arm.com>; nd <nd@arm.com> > Subject: Re: [PATCH v2 2/2] xen/arm: Throw messages for unknown > FP/SIMD implement ID > > > > On 24/08/2020 17:57, Bertrand Marquis wrote: > > Hi Julien, > > Hi, > > > > >> On 24 Aug 2020, at 14:34, Julien Grall <julien@xen.org> wrote: > >> > >> 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. > > > > I would see this as an improvement already. > > TBH, I only view this patch as a band-aid. I am OK with them but they > need to be useful. > > When I read "unknown value...", I have no clue whether the message is > good or bad. It would be better to extend the message with what could go > wrong. E.g "Unknown value X, this may result to corruption on the platform". > > I would also consider to taint Xen as it may become unstable with this > option set. > Hmm, yes, it seems current messages are pale. The user could not understand what we want the message expressed when he got one such message. I will improve them to give user a clear meaning. And I think a XENLOG_WARNING would be better to notice user. I will do them in v3. > > More could be done for SVE (and other bits) but this should be in another > patch set. > > > >> > >>> 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. > > > > I actually started to look into this last week because I came to an issue > comparable to SVE with pointer authentication. > > Maybe we should discuss this subject separately as clearing TID3 bit in HCR > and emulating all ID registers is possible > > but I want to check first if this could have big impacts on performances > > I don't expect any performance drop. In Linux, they will cache all the > values and use alternative to avoid runtime check. > > I also don't expect any OS to use them at runtime in hotpath. > I think this would not be a problem. Most ID check operations would be done in system/application initialization stage. > > and see discuss how to design this as there > > could be a huge amount of cases for example if we want to allow different > parameters for different domains. > > I would suggest to first consider a simple approach where we expose the > same view of the ID registers to all the guests but it is sanitized. > Can I ask the reason why we don't expose native ID registers to guest? Is it because we want to avoid guest using some unaware features of Xen? > We can then discuss whether we want to have a per-guest view of the ID > registers and how to configure it. > > Regarding the implementation, I personally quite like the Linux > cpufeature framework. It is quite powerful and I believe would suit > quite well Xen. > I agree with you. But I think is quite huge, and out of this patch set's scope. We need a separate thread to discuss the design and implement. > Cheers, > > -- > Julien Grall
> On 25 Aug 2020, at 04:02, Wei Chen <Wei.Chen@arm.com> wrote: > > Hi Julien, Bertrand, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 2020年8月25日 1:23 >> To: Bertrand Marquis <Bertrand.Marquis@arm.com> >> Cc: Wei Chen <Wei.Chen@arm.com>; Xen-devel <xen- >> devel@lists.xenproject.org>; sstabellini@kernel.org; Andre Przywara >> <Andre.Przywara@arm.com>; Penny Zheng <Penny.Zheng@arm.com>; Kaly >> Xin <Kaly.Xin@arm.com>; nd <nd@arm.com> >> Subject: Re: [PATCH v2 2/2] xen/arm: Throw messages for unknown >> FP/SIMD implement ID >> >> >> >> On 24/08/2020 17:57, Bertrand Marquis wrote: >>> Hi Julien, >> >> Hi, >> >>> >>>> On 24 Aug 2020, at 14:34, Julien Grall <julien@xen.org> wrote: >>>> >>>> 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. >>> >>> I would see this as an improvement already. >> >> TBH, I only view this patch as a band-aid. I am OK with them but they >> need to be useful. >> >> When I read "unknown value...", I have no clue whether the message is >> good or bad. It would be better to extend the message with what could go >> wrong. E.g "Unknown value X, this may result to corruption on the platform". >> > >> I would also consider to taint Xen as it may become unstable with this >> option set. >> > > Hmm, yes, it seems current messages are pale. The user could not understand > what we want the message expressed when he got one such message. I will improve > them to give user a clear meaning. And I think a XENLOG_WARNING would be better > to notice user. I will do them in v3. > >>> More could be done for SVE (and other bits) but this should be in another >> patch set. >>> >>>> >>>>> 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. >>> >>> I actually started to look into this last week because I came to an issue >> comparable to SVE with pointer authentication. >>> Maybe we should discuss this subject separately as clearing TID3 bit in HCR >> and emulating all ID registers is possible >>> but I want to check first if this could have big impacts on performances >> >> I don't expect any performance drop. In Linux, they will cache all the >> values and use alternative to avoid runtime check. >> >> I also don't expect any OS to use them at runtime in hotpath. >> > > I think this would not be a problem. Most ID check operations would be done > in system/application initialization stage. > >>> and see discuss how to design this as there >>> could be a huge amount of cases for example if we want to allow different >> parameters for different domains. >> >> I would suggest to first consider a simple approach where we expose the >> same view of the ID registers to all the guests but it is sanitized. >> > > Can I ask the reason why we don't expose native ID registers to guest? Is it because > we want to avoid guest using some unaware features of Xen? We should avoid guest using features that we do not support or that we do not even enable. If you take SVE or Pointer Authentication, we are not activating the bits in HCR or CPTR to let a guest use those so the fact that we still let those bit set in the processor features registers is making guests try to use them and crash. > >> We can then discuss whether we want to have a per-guest view of the ID >> registers and how to configure it. >> >> Regarding the implementation, I personally quite like the Linux >> cpufeature framework. It is quite powerful and I believe would suit >> quite well Xen. >> > > I agree with you. But I think is quite huge, and out of this patch set's scope. > We need a separate thread to discuss the design and implement. Definitely this is out of scope of this patch I agree. Regards Bertrand
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)] : "", cpu_has_gicv3 ? " GICv3-SysReg" : ""); printk(" Debug Features: %016"PRIx64" %016"PRIx64"\n",
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> --- xen/arch/arm/setup.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)