One of the oddities of the architecture is that the AArch64 views of the
AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL.
Nonetheless, KVM exposes these registers to userspace for the sake of
save/restore. It is possible that the UNKNOWN value could differ between
systems, leading to a rejected write from userspace.
Avoid the issue altogether by handling the AArch32 ID registers as
RAZ/WI when on an AArch64-only system.
Signed-off-by: Oliver Upton <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++-------------
1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6d0511247df4..9569772cf09a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1144,6 +1144,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
return 0;
}
+static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ /*
+ * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
+ * EL. Promote to RAZ/WI in order to guarantee consistency between
+ * systems.
+ */
+ if (!kvm_supports_32bit_el0())
+ return REG_RAZ | REG_USER_WI;
+
+ return id_visibility(vcpu, r);
+}
+
static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
const struct sys_reg_desc *r)
{
@@ -1331,6 +1345,15 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
.visibility = id_visibility, \
}
+/* sys_reg_desc initialiser for known cpufeature ID registers */
+#define AA32_ID_SANITISED(name) { \
+ SYS_DESC(SYS_##name), \
+ .access = access_id_reg, \
+ .get_user = get_id_reg, \
+ .set_user = set_id_reg, \
+ .visibility = aa32_id_visibility, \
+}
+
/*
* sys_reg_desc initialiser for architecturally unallocated cpufeature ID
* register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
@@ -1418,33 +1441,33 @@ static const struct sys_reg_desc sys_reg_descs[] = {
/* AArch64 mappings of the AArch32 ID registers */
/* CRm=1 */
- ID_SANITISED(ID_PFR0_EL1),
- ID_SANITISED(ID_PFR1_EL1),
- ID_SANITISED(ID_DFR0_EL1),
+ AA32_ID_SANITISED(ID_PFR0_EL1),
+ AA32_ID_SANITISED(ID_PFR1_EL1),
+ AA32_ID_SANITISED(ID_DFR0_EL1),
ID_HIDDEN(ID_AFR0_EL1),
- ID_SANITISED(ID_MMFR0_EL1),
- ID_SANITISED(ID_MMFR1_EL1),
- ID_SANITISED(ID_MMFR2_EL1),
- ID_SANITISED(ID_MMFR3_EL1),
+ AA32_ID_SANITISED(ID_MMFR0_EL1),
+ AA32_ID_SANITISED(ID_MMFR1_EL1),
+ AA32_ID_SANITISED(ID_MMFR2_EL1),
+ AA32_ID_SANITISED(ID_MMFR3_EL1),
/* CRm=2 */
- ID_SANITISED(ID_ISAR0_EL1),
- ID_SANITISED(ID_ISAR1_EL1),
- ID_SANITISED(ID_ISAR2_EL1),
- ID_SANITISED(ID_ISAR3_EL1),
- ID_SANITISED(ID_ISAR4_EL1),
- ID_SANITISED(ID_ISAR5_EL1),
- ID_SANITISED(ID_MMFR4_EL1),
- ID_SANITISED(ID_ISAR6_EL1),
+ AA32_ID_SANITISED(ID_ISAR0_EL1),
+ AA32_ID_SANITISED(ID_ISAR1_EL1),
+ AA32_ID_SANITISED(ID_ISAR2_EL1),
+ AA32_ID_SANITISED(ID_ISAR3_EL1),
+ AA32_ID_SANITISED(ID_ISAR4_EL1),
+ AA32_ID_SANITISED(ID_ISAR5_EL1),
+ AA32_ID_SANITISED(ID_MMFR4_EL1),
+ AA32_ID_SANITISED(ID_ISAR6_EL1),
/* CRm=3 */
- ID_SANITISED(MVFR0_EL1),
- ID_SANITISED(MVFR1_EL1),
- ID_SANITISED(MVFR2_EL1),
+ AA32_ID_SANITISED(MVFR0_EL1),
+ AA32_ID_SANITISED(MVFR1_EL1),
+ AA32_ID_SANITISED(MVFR2_EL1),
ID_UNALLOCATED(3,3),
- ID_SANITISED(ID_PFR2_EL1),
+ AA32_ID_SANITISED(ID_PFR2_EL1),
ID_HIDDEN(ID_DFR1_EL1),
- ID_SANITISED(ID_MMFR5_EL1),
+ AA32_ID_SANITISED(ID_MMFR5_EL1),
ID_UNALLOCATED(3,7),
/* AArch64 ID registers */
--
2.37.2.789.g6183377224-goog
Hi Oliver,
On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <[email protected]> wrote:
>
> One of the oddities of the architecture is that the AArch64 views of the
> AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL.
> Nonetheless, KVM exposes these registers to userspace for the sake of
> save/restore. It is possible that the UNKNOWN value could differ between
> systems, leading to a rejected write from userspace.
>
> Avoid the issue altogether by handling the AArch32 ID registers as
> RAZ/WI when on an AArch64-only system.
>
> Signed-off-by: Oliver Upton <[email protected]>
> ---
> arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++-------------
> 1 file changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 6d0511247df4..9569772cf09a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1144,6 +1144,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *r)
> +{
> + /*
> + * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
> + * EL. Promote to RAZ/WI in order to guarantee consistency between
> + * systems.
> + */
> + if (!kvm_supports_32bit_el0())
> + return REG_RAZ | REG_USER_WI;
> +
> + return id_visibility(vcpu, r);
> +}
> +
> static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
> {
> @@ -1331,6 +1345,15 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
> .visibility = id_visibility, \
> }
>
> +/* sys_reg_desc initialiser for known cpufeature ID registers */
> +#define AA32_ID_SANITISED(name) { \
> + SYS_DESC(SYS_##name), \
> + .access = access_id_reg, \
> + .get_user = get_id_reg, \
> + .set_user = set_id_reg, \
> + .visibility = aa32_id_visibility, \
> +}
> +
> /*
> * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
> * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
> @@ -1418,33 +1441,33 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>
> /* AArch64 mappings of the AArch32 ID registers */
> /* CRm=1 */
> - ID_SANITISED(ID_PFR0_EL1),
> - ID_SANITISED(ID_PFR1_EL1),
> - ID_SANITISED(ID_DFR0_EL1),
> + AA32_ID_SANITISED(ID_PFR0_EL1),
> + AA32_ID_SANITISED(ID_PFR1_EL1),
> + AA32_ID_SANITISED(ID_DFR0_EL1),
> ID_HIDDEN(ID_AFR0_EL1),
> - ID_SANITISED(ID_MMFR0_EL1),
> - ID_SANITISED(ID_MMFR1_EL1),
> - ID_SANITISED(ID_MMFR2_EL1),
> - ID_SANITISED(ID_MMFR3_EL1),
> + AA32_ID_SANITISED(ID_MMFR0_EL1),
> + AA32_ID_SANITISED(ID_MMFR1_EL1),
> + AA32_ID_SANITISED(ID_MMFR2_EL1),
> + AA32_ID_SANITISED(ID_MMFR3_EL1),
>
> /* CRm=2 */
> - ID_SANITISED(ID_ISAR0_EL1),
> - ID_SANITISED(ID_ISAR1_EL1),
> - ID_SANITISED(ID_ISAR2_EL1),
> - ID_SANITISED(ID_ISAR3_EL1),
> - ID_SANITISED(ID_ISAR4_EL1),
> - ID_SANITISED(ID_ISAR5_EL1),
> - ID_SANITISED(ID_MMFR4_EL1),
> - ID_SANITISED(ID_ISAR6_EL1),
> + AA32_ID_SANITISED(ID_ISAR0_EL1),
> + AA32_ID_SANITISED(ID_ISAR1_EL1),
> + AA32_ID_SANITISED(ID_ISAR2_EL1),
> + AA32_ID_SANITISED(ID_ISAR3_EL1),
> + AA32_ID_SANITISED(ID_ISAR4_EL1),
> + AA32_ID_SANITISED(ID_ISAR5_EL1),
> + AA32_ID_SANITISED(ID_MMFR4_EL1),
> + AA32_ID_SANITISED(ID_ISAR6_EL1),
>
> /* CRm=3 */
> - ID_SANITISED(MVFR0_EL1),
> - ID_SANITISED(MVFR1_EL1),
> - ID_SANITISED(MVFR2_EL1),
> + AA32_ID_SANITISED(MVFR0_EL1),
> + AA32_ID_SANITISED(MVFR1_EL1),
> + AA32_ID_SANITISED(MVFR2_EL1),
> ID_UNALLOCATED(3,3),
> - ID_SANITISED(ID_PFR2_EL1),
> + AA32_ID_SANITISED(ID_PFR2_EL1),
> ID_HIDDEN(ID_DFR1_EL1),
Perhaps it might be better to handle ID_AFR0_EL1 and ID_DFR1_EL1
in the same way as the other AArch32 ID registers for consistency ?
(i.e. treat them RAZ/USER_WI instead of RAZ if kvm_supports_32bit_el0()
is false instead of RAZ)
Thank you,
Reiji
> - ID_SANITISED(ID_MMFR5_EL1),
> + AA32_ID_SANITISED(ID_MMFR5_EL1),
> ID_UNALLOCATED(3,7),
>
> /* AArch64 ID registers */
> --
> 2.37.2.789.g6183377224-goog
>
Hi Reiji,
On Tue, Sep 06, 2022 at 09:52:53PM -0700, Reiji Watanabe wrote:
[...]
> > /* CRm=3 */
> > - ID_SANITISED(MVFR0_EL1),
> > - ID_SANITISED(MVFR1_EL1),
> > - ID_SANITISED(MVFR2_EL1),
> > + AA32_ID_SANITISED(MVFR0_EL1),
> > + AA32_ID_SANITISED(MVFR1_EL1),
> > + AA32_ID_SANITISED(MVFR2_EL1),
> > ID_UNALLOCATED(3,3),
> > - ID_SANITISED(ID_PFR2_EL1),
> > + AA32_ID_SANITISED(ID_PFR2_EL1),
> > ID_HIDDEN(ID_DFR1_EL1),
>
> Perhaps it might be better to handle ID_AFR0_EL1 and ID_DFR1_EL1
> in the same way as the other AArch32 ID registers for consistency ?
> (i.e. treat them RAZ/USER_WI instead of RAZ if kvm_supports_32bit_el0()
> is false instead of RAZ)
Thanks for having a look. I stopped short of treating these registers as
RAZ/USER_WI since an attempted nonzero write to either of these
registers is a userspace bug (KVM always advertised 0).
As the ABI isn't busted for these registers I'd prefer to leave it in
place. Having said that, I'm not too strongly motivated in either
direction.
--
Thanks,
Oliver