Hi Oliver,
On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <oliver.upton@xxxxxxxxx> 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 <oliver.upton@xxxxxxxxx>
> ---
> 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
>
