| Message ID | 20200825100847.27988-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 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,
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,
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)