Re: [PATCH v2 6/7] KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system

[Reiji Watanabe]

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
>