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: <20220927002715.2142353-1-mizhang@google.com>
In-Reply-To: <20220927002715.2142353-1-mizhang@google.com>
From:   Reiji Watanabe <reijiw@google.com>
Date:   Tue, 27 Sep 2022 00:01:22 -0700
Message-ID: <CAAeT=FwqWkbhFLHvr12A2rYdzZuDJWBti=jKQRargvAb6bKtaA@mail.gmail.com>
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out
 the code that validates HPFAR
To:     Mingwei Zhang <mizhang@google.com>
Cc:     Marc Zyngier <maz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        kvmarm@lists.cs.columbia.edu,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mingwei,

On Mon, Sep 26, 2022 at 5:27 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> Cleanup __get_fault_info() to take out the code that checks HPFAR. The
> conditions in __get_fault_info() that checks if HPFAR contains a valid IPA
> is slightly messy in that several conditions are written within one IF
> statement acrossing multiple lines and are connected with different logical
> operators. Among them, some conditions come from ARM Spec, while others
> come from CPU erratum. This makes the code hard to read and difficult to
> extend.
>
> So, cleanup the function to improve the readability. In particular,
> explicitly specify each condition separately within a newly created inline
> function.
>
> No functional changes are intended.
>
> Suggested-by: Oliver Upton <oupton@google.com>
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  arch/arm64/kvm/hyp/include/hyp/fault.h | 36 ++++++++++++++++----------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
> index 1b8a2dcd712f..4575500d26ff 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/fault.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
> @@ -41,12 +41,6 @@ static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
>         return true;
>  }
>
> -static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
> -{
> -       u64 hpfar, far;
> -
> -       far = read_sysreg_el2(SYS_FAR);
> -
>         /*
>          * The HPFAR can be invalid if the stage 2 fault did not
>          * happen during a stage 1 page table walk (the ESR_EL2.S1PTW
> @@ -58,14 +52,30 @@ static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
>          * permission fault or the errata workaround is enabled, we
>          * resolve the IPA using the AT instruction.
>          */
> -       if (!(esr & ESR_ELx_S1PTW) &&
> -           (cpus_have_final_cap(ARM64_WORKAROUND_834220) ||
> -            (esr & ESR_ELx_FSC_TYPE) == FSC_PERM)) {
> -               if (!__translate_far_to_hpfar(far, &hpfar))
> -                       return false;
> -       } else {
> +static inline bool __hpfar_is_valid(u64 esr)

Unlike what the name implies, this function returns true for some
cases that HPFAR is not valid (i.e. SEA).  I think the function
returns true when KVM doesn't need HPFAR, or when HPFAR is valid.
IMHO the name might be a bit misleading, although I don't have
a good name for this.  It would be nice to state that in the
comment at least.

Thank you,
Reiji


> +{
> +       if (esr & ESR_ELx_S1PTW)
> +               return true;
> +
> +       if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
> +               return false;
> +
> +       if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
> +               return false;
> +
> +       return true;
> +}
> +
> +static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
> +{
> +       u64 hpfar, far;
> +
> +       far = read_sysreg_el2(SYS_FAR);
> +
> +       if (!__hpfar_is_valid(esr) && !__translate_far_to_hpfar(far, &hpfar))
> +               return false;
> +       else
>                 hpfar = read_sysreg(hpfar_el2);
> -       }
>
>         fault->far_el2 = far;
>         fault->hpfar_el2 = hpfar;
>
> base-commit: c59fb127583869350256656b7ed848c398bef879
> --
> 2.37.3.998.g577e59143f-goog
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm