| Message ID | 1576145663-9909-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Introduce ISAR6 CPU ID register | expand |
On 2019-12-12 10:14, Anshuman Khandual wrote: > This adds basic building blocks required for ISAR6 CPU ID register > which > identifies support for various instruction implementation on AArch32 > state. nit: the register name is ID_ISAR6. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > Cc: James Morse <james.morse@arm.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-kernel@vger.kernel.org > Cc: kvmarm@lists.cs.columbia.edu > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arm64/include/asm/cpu.h | 1 + > arch/arm64/include/asm/sysreg.h | 9 +++++++++ > arch/arm64/kernel/cpufeature.c | 7 ++++++- > arch/arm64/kernel/cpuinfo.c | 1 + > arch/arm64/kvm/sys_regs.c | 2 +- > 5 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/cpu.h > b/arch/arm64/include/asm/cpu.h > index d72d995..b4a4053 100644 > --- a/arch/arm64/include/asm/cpu.h > +++ b/arch/arm64/include/asm/cpu.h > @@ -39,6 +39,7 @@ struct cpuinfo_arm64 { > u32 reg_id_isar3; > u32 reg_id_isar4; > u32 reg_id_isar5; > + u32 reg_id_isar6; > u32 reg_id_mmfr0; > u32 reg_id_mmfr1; > u32 reg_id_mmfr2; > diff --git a/arch/arm64/include/asm/sysreg.h > b/arch/arm64/include/asm/sysreg.h > index 6db3a9b..4fd3327 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -146,6 +146,7 @@ > #define SYS_ID_ISAR4_EL1 sys_reg(3, 0, 0, 2, 4) > #define SYS_ID_ISAR5_EL1 sys_reg(3, 0, 0, 2, 5) > #define SYS_ID_MMFR4_EL1 sys_reg(3, 0, 0, 2, 6) > +#define SYS_ID_ISAR6_EL1 sys_reg(3, 0, 0, 2, 7) > > #define SYS_MVFR0_EL1 sys_reg(3, 0, 0, 3, 0) > #define SYS_MVFR1_EL1 sys_reg(3, 0, 0, 3, 1) > @@ -683,6 +684,14 @@ > #define ID_ISAR5_AES_SHIFT 4 > #define ID_ISAR5_SEVL_SHIFT 0 > > +#define ID_ISAR6_JSCVT_SHIFT 0 > +#define ID_ISAR6_DP_SHIFT 4 > +#define ID_ISAR6_FHM_SHIFT 8 > +#define ID_ISAR6_SB_SHIFT 12 > +#define ID_ISAR6_SPECRES_SHIFT 16 > +#define ID_ISAR6_BF16_SHIFT 20 > +#define ID_ISAR6_I8MM_SHIFT 24 I couldn't find the last two items in the E.a revision of the ARMv8 ARM. I guess they are for post 8.5 revisions of the architecture? > + > #define MVFR0_FPROUND_SHIFT 28 > #define MVFR0_FPSHVEC_SHIFT 24 > #define MVFR0_FPSQRT_SHIFT 20 > diff --git a/arch/arm64/kernel/cpufeature.c > b/arch/arm64/kernel/cpufeature.c > index f344cea..3b9ac8b 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -346,7 +346,7 @@ static const struct arm64_ftr_bits ftr_zcr[] = { > * Common ftr bits for a 32bit register with all hidden, strict > * attributes, with 4bit feature fields and a default safe value of > * 0. Covers the following 32bit registers: > - * id_isar[0-4], id_mmfr[1-3], id_pfr1, mvfr[0-1] > + * id_isar[0-4, 6], id_mmfr[1-3], id_pfr1, mvfr[0-1] > */ > static const struct arm64_ftr_bits ftr_generic_32bits[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 28, 4, 0), > @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { > ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), > ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), > ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), > + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), > > /* Op1 = 0, CRn = 0, CRm = 3 */ > ARM64_FTR_REG(SYS_MVFR0_EL1, ftr_generic_32bits), > @@ -603,6 +604,7 @@ void __init init_cpu_features(struct > cpuinfo_arm64 *info) > init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3); > init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4); > init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5); > + init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6); > init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0); > init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1); > init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2); > @@ -756,6 +758,8 @@ void update_cpu_features(int cpu, > info->reg_id_isar4, boot->reg_id_isar4); > taint |= check_update_ftr_reg(SYS_ID_ISAR5_EL1, cpu, > info->reg_id_isar5, boot->reg_id_isar5); > + taint |= check_update_ftr_reg(SYS_ID_ISAR6_EL1, cpu, > + info->reg_id_isar6, boot->reg_id_isar6); > > /* > * Regardless of the value of the AuxReg field, the AIFSR, ADFSR, > and > @@ -834,6 +838,7 @@ static u64 __read_sysreg_by_encoding(u32 sys_id) > read_sysreg_case(SYS_ID_ISAR3_EL1); > read_sysreg_case(SYS_ID_ISAR4_EL1); > read_sysreg_case(SYS_ID_ISAR5_EL1); > + read_sysreg_case(SYS_ID_ISAR6_EL1); > read_sysreg_case(SYS_MVFR0_EL1); > read_sysreg_case(SYS_MVFR1_EL1); > read_sysreg_case(SYS_MVFR2_EL1); > diff --git a/arch/arm64/kernel/cpuinfo.c > b/arch/arm64/kernel/cpuinfo.c > index 10121f5..6005d38 100644 > --- a/arch/arm64/kernel/cpuinfo.c > +++ b/arch/arm64/kernel/cpuinfo.c > @@ -362,6 +362,7 @@ static void __cpuinfo_store_cpu(struct > cpuinfo_arm64 *info) > info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1); > info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1); > info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1); > + info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1); > info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1); > info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1); > info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 7dadd24..a6b8ca1 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1424,7 +1424,7 @@ static const struct sys_reg_desc > sys_reg_descs[] = { > ID_SANITISED(ID_ISAR4_EL1), > ID_SANITISED(ID_ISAR5_EL1), > ID_SANITISED(ID_MMFR4_EL1), > - ID_UNALLOCATED(2,7), > + ID_SANITISED(ID_ISAR6_EL1), > > /* CRm=3 */ > ID_SANITISED(MVFR0_EL1), Otherwise, Acked-by: Marc Zyngier <maz@kernel.org> M.
On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote: > +#define ID_ISAR6_JSCVT_SHIFT 0 > +#define ID_ISAR6_DP_SHIFT 4 > +#define ID_ISAR6_FHM_SHIFT 8 > +#define ID_ISAR6_SB_SHIFT 12 > +#define ID_ISAR6_SPECRES_SHIFT 16 > +#define ID_ISAR6_BF16_SHIFT 20 > +#define ID_ISAR6_I8MM_SHIFT 24 > @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { > ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), > ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), > ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), > + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), Using ftr_generic_32bits exposes the lowest-common-denominator for all 4-bit fields in the register, and I don't think that's the right thing to do here, because: * We have no idea what ID_ISAR6 bits [31:28] may mean in future. * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the ARMv8.0-PredInv extension) operate on the local PE and are not broadcast. To make those work as a guest expects, the host will need to do additional things (e.g. to preserve that illusion when a vCPU is migrated from one pCPU to another and back). Given that, think we should add an explicit ftr_id_isar6 which only exposes the fields that we're certain are safe to expose to a guest (i.e. without SPECRES). Thanks, Mark.
On 12/12/2019 14:46, Mark Rutland wrote: > On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote: >> +#define ID_ISAR6_JSCVT_SHIFT 0 >> +#define ID_ISAR6_DP_SHIFT 4 >> +#define ID_ISAR6_FHM_SHIFT 8 >> +#define ID_ISAR6_SB_SHIFT 12 >> +#define ID_ISAR6_SPECRES_SHIFT 16 >> +#define ID_ISAR6_BF16_SHIFT 20 >> +#define ID_ISAR6_I8MM_SHIFT 24 > >> @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { >> ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), >> ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), >> ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), > >> + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), > > Using ftr_generic_32bits exposes the lowest-common-denominator for all > 4-bit fields in the register, and I don't think that's the right thing > to do here, because: > > * We have no idea what ID_ISAR6 bits [31:28] may mean in future. > > * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the > ARMv8.0-PredInv extension) operate on the local PE and are not > broadcast. To make those work as a guest expects, the host will need > to do additional things (e.g. to preserve that illusion when a vCPU is > migrated from one pCPU to another and back). > > Given that, think we should add an explicit ftr_id_isar6 which only > exposes the fields that we're certain are safe to expose to a guest > (i.e. without SPECRES). Agree. Thanks for pointing this out. I recommended the usage of generic_32bits table without actually looking at the feature definitions. Anshuman, Sorry about this. Cheers Suzuki
On Thu, Dec 12, 2019 at 03:22:13PM +0000, Suzuki Kuruppassery Poulose wrote: > On 12/12/2019 14:46, Mark Rutland wrote: > > On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote: > > > +#define ID_ISAR6_JSCVT_SHIFT 0 > > > +#define ID_ISAR6_DP_SHIFT 4 > > > +#define ID_ISAR6_FHM_SHIFT 8 > > > +#define ID_ISAR6_SB_SHIFT 12 > > > +#define ID_ISAR6_SPECRES_SHIFT 16 > > > +#define ID_ISAR6_BF16_SHIFT 20 > > > +#define ID_ISAR6_I8MM_SHIFT 24 > > > > > @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { > > > ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), > > > ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), > > > ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), > > > > > + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), > > > > Using ftr_generic_32bits exposes the lowest-common-denominator for all > > 4-bit fields in the register, and I don't think that's the right thing > > to do here, because: > > > > * We have no idea what ID_ISAR6 bits [31:28] may mean in future. > > > > * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the > > ARMv8.0-PredInv extension) operate on the local PE and are not > > broadcast. To make those work as a guest expects, the host will need > > to do additional things (e.g. to preserve that illusion when a vCPU is > > migrated from one pCPU to another and back). > > > > Given that, think we should add an explicit ftr_id_isar6 which only > > exposes the fields that we're certain are safe to expose to a guest > > (i.e. without SPECRES). > > Agree. Thanks for pointing this out. I recommended the usage of > generic_32bits table without actually looking at the feature > definitions. No worries; this is /really/ easy to miss! Looking again, comparing to ARM DDI 0487E.a, there are a few other things we should probably sort out: * ID_DFR0 fields need more thought; we should limit what we expose here. I don't think it's valid for us to expose TraceFilt, and I suspect we need to add capping for debug features we currently emulate. * ID_ISAR0[31:28] are RES0 in ARMv8, Reserved/UNK in ARMv7. We should probably ftr_id_isar0 so we can hide those bits. * ID_ISAR5[23:10] are RES0 We handle this already! :) * ID_MMFR4.SpecSEI should be trated as higher safe. We should update ftr_id_mmfr4 to handle this and other fields. * ID_PFR0 is missing DIT and CSV2 We should probably add these (but neither RAS not AMU). * ID_PFR2 is missing We should probably add this for SSBS and CSV3. Thanks, Mark.
On 12/12/2019 05:13 PM, Marc Zyngier wrote: > On 2019-12-12 10:14, Anshuman Khandual wrote: >> This adds basic building blocks required for ISAR6 CPU ID register which >> identifies support for various instruction implementation on AArch32 state. > > nit: the register name is ID_ISAR6. Sure, will change. > >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: James Morse <james.morse@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: kvmarm@lists.cs.columbia.edu >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/include/asm/cpu.h | 1 + >> arch/arm64/include/asm/sysreg.h | 9 +++++++++ >> arch/arm64/kernel/cpufeature.c | 7 ++++++- >> arch/arm64/kernel/cpuinfo.c | 1 + >> arch/arm64/kvm/sys_regs.c | 2 +- >> 5 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h >> index d72d995..b4a4053 100644 >> --- a/arch/arm64/include/asm/cpu.h >> +++ b/arch/arm64/include/asm/cpu.h >> @@ -39,6 +39,7 @@ struct cpuinfo_arm64 { >> u32 reg_id_isar3; >> u32 reg_id_isar4; >> u32 reg_id_isar5; >> + u32 reg_id_isar6; >> u32 reg_id_mmfr0; >> u32 reg_id_mmfr1; >> u32 reg_id_mmfr2; >> diff --git a/arch/arm64/include/asm/sysreg.h >> b/arch/arm64/include/asm/sysreg.h >> index 6db3a9b..4fd3327 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -146,6 +146,7 @@ >> #define SYS_ID_ISAR4_EL1 sys_reg(3, 0, 0, 2, 4) >> #define SYS_ID_ISAR5_EL1 sys_reg(3, 0, 0, 2, 5) >> #define SYS_ID_MMFR4_EL1 sys_reg(3, 0, 0, 2, 6) >> +#define SYS_ID_ISAR6_EL1 sys_reg(3, 0, 0, 2, 7) >> >> #define SYS_MVFR0_EL1 sys_reg(3, 0, 0, 3, 0) >> #define SYS_MVFR1_EL1 sys_reg(3, 0, 0, 3, 1) >> @@ -683,6 +684,14 @@ >> #define ID_ISAR5_AES_SHIFT 4 >> #define ID_ISAR5_SEVL_SHIFT 0 >> >> +#define ID_ISAR6_JSCVT_SHIFT 0 >> +#define ID_ISAR6_DP_SHIFT 4 >> +#define ID_ISAR6_FHM_SHIFT 8 >> +#define ID_ISAR6_SB_SHIFT 12 >> +#define ID_ISAR6_SPECRES_SHIFT 16 >> +#define ID_ISAR6_BF16_SHIFT 20 >> +#define ID_ISAR6_I8MM_SHIFT 24 > > I couldn't find the last two items in the E.a revision of the ARMv8 ARM. > I guess they are for post 8.5 revisions of the architecture? Yes. > >> + >> #define MVFR0_FPROUND_SHIFT 28 >> #define MVFR0_FPSHVEC_SHIFT 24 >> #define MVFR0_FPSQRT_SHIFT 20 >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index f344cea..3b9ac8b 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -346,7 +346,7 @@ static const struct arm64_ftr_bits ftr_zcr[] = { >> * Common ftr bits for a 32bit register with all hidden, strict >> * attributes, with 4bit feature fields and a default safe value of >> * 0. Covers the following 32bit registers: >> - * id_isar[0-4], id_mmfr[1-3], id_pfr1, mvfr[0-1] >> + * id_isar[0-4, 6], id_mmfr[1-3], id_pfr1, mvfr[0-1] >> */ >> static const struct arm64_ftr_bits ftr_generic_32bits[] = { >> ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 28, 4, 0), >> @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { >> ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), >> ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), >> ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), >> + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), >> >> /* Op1 = 0, CRn = 0, CRm = 3 */ >> ARM64_FTR_REG(SYS_MVFR0_EL1, ftr_generic_32bits), >> @@ -603,6 +604,7 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info) >> init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3); >> init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4); >> init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5); >> + init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6); >> init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0); >> init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1); >> init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2); >> @@ -756,6 +758,8 @@ void update_cpu_features(int cpu, >> info->reg_id_isar4, boot->reg_id_isar4); >> taint |= check_update_ftr_reg(SYS_ID_ISAR5_EL1, cpu, >> info->reg_id_isar5, boot->reg_id_isar5); >> + taint |= check_update_ftr_reg(SYS_ID_ISAR6_EL1, cpu, >> + info->reg_id_isar6, boot->reg_id_isar6); >> >> /* >> * Regardless of the value of the AuxReg field, the AIFSR, ADFSR, and >> @@ -834,6 +838,7 @@ static u64 __read_sysreg_by_encoding(u32 sys_id) >> read_sysreg_case(SYS_ID_ISAR3_EL1); >> read_sysreg_case(SYS_ID_ISAR4_EL1); >> read_sysreg_case(SYS_ID_ISAR5_EL1); >> + read_sysreg_case(SYS_ID_ISAR6_EL1); >> read_sysreg_case(SYS_MVFR0_EL1); >> read_sysreg_case(SYS_MVFR1_EL1); >> read_sysreg_case(SYS_MVFR2_EL1); >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c >> index 10121f5..6005d38 100644 >> --- a/arch/arm64/kernel/cpuinfo.c >> +++ b/arch/arm64/kernel/cpuinfo.c >> @@ -362,6 +362,7 @@ static void __cpuinfo_store_cpu(struct >> cpuinfo_arm64 *info) >> info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1); >> info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1); >> info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1); >> + info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1); >> info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1); >> info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1); >> info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1); >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 7dadd24..a6b8ca1 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -1424,7 +1424,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { >> ID_SANITISED(ID_ISAR4_EL1), >> ID_SANITISED(ID_ISAR5_EL1), >> ID_SANITISED(ID_MMFR4_EL1), >> - ID_UNALLOCATED(2,7), >> + ID_SANITISED(ID_ISAR6_EL1), >> >> /* CRm=3 */ >> ID_SANITISED(MVFR0_EL1), > > Otherwise, > > Acked-by: Marc Zyngier <maz@kernel.org> > > M.
On 12/12/2019 08:52 PM, Suzuki Kuruppassery Poulose wrote: > On 12/12/2019 14:46, Mark Rutland wrote: >> On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote: >>> +#define ID_ISAR6_JSCVT_SHIFT 0 >>> +#define ID_ISAR6_DP_SHIFT 4 >>> +#define ID_ISAR6_FHM_SHIFT 8 >>> +#define ID_ISAR6_SB_SHIFT 12 >>> +#define ID_ISAR6_SPECRES_SHIFT 16 >>> +#define ID_ISAR6_BF16_SHIFT 20 >>> +#define ID_ISAR6_I8MM_SHIFT 24 >> >>> @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { >>> ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), >>> ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), >>> ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), >> >>> + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), >> >> Using ftr_generic_32bits exposes the lowest-common-denominator for all >> 4-bit fields in the register, and I don't think that's the right thing >> to do here, because: >> >> * We have no idea what ID_ISAR6 bits [31:28] may mean in future. >> >> * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the >> ARMv8.0-PredInv extension) operate on the local PE and are not >> broadcast. To make those work as a guest expects, the host will need >> to do additional things (e.g. to preserve that illusion when a vCPU is >> migrated from one pCPU to another and back). >> >> Given that, think we should add an explicit ftr_id_isar6 which only >> exposes the fields that we're certain are safe to expose to a guest >> (i.e. without SPECRES). > > Agree. Thanks for pointing this out. I recommended the usage of > generic_32bits table without actually looking at the feature > definitions. > > Anshuman, > > Sorry about this. Not a problem, will add an explicit definition for ftr_id_isar6 with all features except SPECRES as Mark had pointed put. > > Cheers > Suzuki
On 12/12/2019 10:01 PM, Mark Rutland wrote: > On Thu, Dec 12, 2019 at 03:22:13PM +0000, Suzuki Kuruppassery Poulose wrote: >> On 12/12/2019 14:46, Mark Rutland wrote: >>> On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote: >>>> +#define ID_ISAR6_JSCVT_SHIFT 0 >>>> +#define ID_ISAR6_DP_SHIFT 4 >>>> +#define ID_ISAR6_FHM_SHIFT 8 >>>> +#define ID_ISAR6_SB_SHIFT 12 >>>> +#define ID_ISAR6_SPECRES_SHIFT 16 >>>> +#define ID_ISAR6_BF16_SHIFT 20 >>>> +#define ID_ISAR6_I8MM_SHIFT 24 >>> >>>> @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { >>>> ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), >>>> ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), >>>> ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), >>> >>>> + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), >>> >>> Using ftr_generic_32bits exposes the lowest-common-denominator for all >>> 4-bit fields in the register, and I don't think that's the right thing >>> to do here, because: >>> >>> * We have no idea what ID_ISAR6 bits [31:28] may mean in future. >>> >>> * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the >>> ARMv8.0-PredInv extension) operate on the local PE and are not >>> broadcast. To make those work as a guest expects, the host will need >>> to do additional things (e.g. to preserve that illusion when a vCPU is >>> migrated from one pCPU to another and back). >>> >>> Given that, think we should add an explicit ftr_id_isar6 which only >>> exposes the fields that we're certain are safe to expose to a guest >>> (i.e. without SPECRES). >> >> Agree. Thanks for pointing this out. I recommended the usage of >> generic_32bits table without actually looking at the feature >> definitions. > > No worries; this is /really/ easy to miss! > > Looking again, comparing to ARM DDI 0487E.a, there are a few other > things we should probably sort out: > > * ID_DFR0 fields need more thought; we should limit what we expose here. > I don't think it's valid for us to expose TraceFilt, and I suspect we Sure, will go ahead and drop TraceFilt [28..31] from ID_DFR0 register. > need to add capping for debug features we currently emulate. Could you please elaborate ? > > * ID_ISAR0[31:28] are RES0 in ARMv8, Reserved/UNK in ARMv7. > We should probably ftr_id_isar0 so we can hide those bits. Sure, will do. > > * ID_ISAR5[23:10] are RES0 > We handle this already! :) I may be missing something here but some of these fields are already there. #define ID_ISAR5_RDM_SHIFT 24 #define ID_ISAR5_CRC32_SHIFT 16 #define ID_ISAR5_SHA2_SHIFT 12 #define ID_ISAR5_SHA1_SHIFT 8 #define ID_ISAR5_AES_SHIFT 4 #define ID_ISAR5_SEVL_SHIFT 0 static const struct arm64_ftr_bits ftr_id_isar5[] = { ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_RDM_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_CRC32_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SHA2_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SHA1_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_AES_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SEVL_SHIFT, 4, 0), ARM64_FTR_END, }; > > * ID_MMFR4.SpecSEI should be trated as higher safe. > We should update ftr_id_mmfr4 to handle this and other fields. Sure but should we also export other fields as higher safe in there ? > > * ID_PFR0 is missing DIT and CSV2 > We should probably add these (but neither RAS not AMU). Sure, will do. > > * ID_PFR2 is missing > We should probably add this for SSBS and CSV3. Sure but should we add corresponding ID_AA64PFR2_EL1 register as well ? > > Thanks, > Mark. >
diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h index d72d995..b4a4053 100644 --- a/arch/arm64/include/asm/cpu.h +++ b/arch/arm64/include/asm/cpu.h @@ -39,6 +39,7 @@ struct cpuinfo_arm64 { u32 reg_id_isar3; u32 reg_id_isar4; u32 reg_id_isar5; + u32 reg_id_isar6; u32 reg_id_mmfr0; u32 reg_id_mmfr1; u32 reg_id_mmfr2; diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 6db3a9b..4fd3327 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -146,6 +146,7 @@ #define SYS_ID_ISAR4_EL1 sys_reg(3, 0, 0, 2, 4) #define SYS_ID_ISAR5_EL1 sys_reg(3, 0, 0, 2, 5) #define SYS_ID_MMFR4_EL1 sys_reg(3, 0, 0, 2, 6) +#define SYS_ID_ISAR6_EL1 sys_reg(3, 0, 0, 2, 7) #define SYS_MVFR0_EL1 sys_reg(3, 0, 0, 3, 0) #define SYS_MVFR1_EL1 sys_reg(3, 0, 0, 3, 1) @@ -683,6 +684,14 @@ #define ID_ISAR5_AES_SHIFT 4 #define ID_ISAR5_SEVL_SHIFT 0 +#define ID_ISAR6_JSCVT_SHIFT 0 +#define ID_ISAR6_DP_SHIFT 4 +#define ID_ISAR6_FHM_SHIFT 8 +#define ID_ISAR6_SB_SHIFT 12 +#define ID_ISAR6_SPECRES_SHIFT 16 +#define ID_ISAR6_BF16_SHIFT 20 +#define ID_ISAR6_I8MM_SHIFT 24 + #define MVFR0_FPROUND_SHIFT 28 #define MVFR0_FPSHVEC_SHIFT 24 #define MVFR0_FPSQRT_SHIFT 20 diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index f344cea..3b9ac8b 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -346,7 +346,7 @@ static const struct arm64_ftr_bits ftr_zcr[] = { * Common ftr bits for a 32bit register with all hidden, strict * attributes, with 4bit feature fields and a default safe value of * 0. Covers the following 32bit registers: - * id_isar[0-4], id_mmfr[1-3], id_pfr1, mvfr[0-1] + * id_isar[0-4, 6], id_mmfr[1-3], id_pfr1, mvfr[0-1] */ static const struct arm64_ftr_bits ftr_generic_32bits[] = { ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 28, 4, 0), @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry { ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits), ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5), ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4), + ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits), /* Op1 = 0, CRn = 0, CRm = 3 */ ARM64_FTR_REG(SYS_MVFR0_EL1, ftr_generic_32bits), @@ -603,6 +604,7 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info) init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3); init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4); init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5); + init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6); init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0); init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1); init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2); @@ -756,6 +758,8 @@ void update_cpu_features(int cpu, info->reg_id_isar4, boot->reg_id_isar4); taint |= check_update_ftr_reg(SYS_ID_ISAR5_EL1, cpu, info->reg_id_isar5, boot->reg_id_isar5); + taint |= check_update_ftr_reg(SYS_ID_ISAR6_EL1, cpu, + info->reg_id_isar6, boot->reg_id_isar6); /* * Regardless of the value of the AuxReg field, the AIFSR, ADFSR, and @@ -834,6 +838,7 @@ static u64 __read_sysreg_by_encoding(u32 sys_id) read_sysreg_case(SYS_ID_ISAR3_EL1); read_sysreg_case(SYS_ID_ISAR4_EL1); read_sysreg_case(SYS_ID_ISAR5_EL1); + read_sysreg_case(SYS_ID_ISAR6_EL1); read_sysreg_case(SYS_MVFR0_EL1); read_sysreg_case(SYS_MVFR1_EL1); read_sysreg_case(SYS_MVFR2_EL1); diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index 10121f5..6005d38 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -362,6 +362,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1); info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1); info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1); + info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1); info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1); info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1); info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1); diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 7dadd24..a6b8ca1 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1424,7 +1424,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_SANITISED(ID_ISAR4_EL1), ID_SANITISED(ID_ISAR5_EL1), ID_SANITISED(ID_MMFR4_EL1), - ID_UNALLOCATED(2,7), + ID_SANITISED(ID_ISAR6_EL1), /* CRm=3 */ ID_SANITISED(MVFR0_EL1),
This adds basic building blocks required for ISAR6 CPU ID register which identifies support for various instruction implementation on AArch32 state. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Marc Zyngier <maz@kernel.org> Cc: James Morse <james.morse@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: linux-kernel@vger.kernel.org Cc: kvmarm@lists.cs.columbia.edu Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/include/asm/cpu.h | 1 + arch/arm64/include/asm/sysreg.h | 9 +++++++++ arch/arm64/kernel/cpufeature.c | 7 ++++++- arch/arm64/kernel/cpuinfo.c | 1 + arch/arm64/kvm/sys_regs.c | 2 +- 5 files changed, 18 insertions(+), 2 deletions(-)