Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220902154804.1939819-1-oliver.upton@linux.dev> <20220902154804.1939819-7-oliver.upton@linux.dev>
In-Reply-To: <20220902154804.1939819-7-oliver.upton@linux.dev>
From:   Reiji Watanabe <reijiw@google.com>
Date:   Tue, 6 Sep 2022 21:52:53 -0700
Message-ID: <CAAeT=FxARdyXJyDgh_E4L-w0azuCY+47WgoM9MheBwyS8SdX1Q@mail.gmail.com>
Subject: Re: [PATCH v2 6/7] KVM: arm64: Treat 32bit ID registers as RAZ/WI on
 64bit-only system
To:     Oliver Upton <oliver.upton@linux.dev>
Cc:     Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Oliver,

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