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;
Date:   Mon, 26 Sep 2022 22:14:16 -0700
From:   Oliver Upton <oliver.upton@linux.dev>
To:     Mingwei Zhang <mizhang@google.com>
Cc:     Marc Zyngier <maz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        James Morse <james.morse@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out
 the code that validates HPFAR
Message-ID: <YzKGqLzjv0I/gkMi@google.com>
References: <20220927002715.2142353-1-mizhang@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220927002715.2142353-1-mizhang@google.com>
Precedence: bulk

Hi Mingwei,

On Tue, Sep 27, 2022 at 12:27:15AM +0000, Mingwei Zhang 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
						   ^~~~~~~~

Call it the ARM ARM or Arm ARM, depending on what stylization you
subscribe to :)

> come from CPU erratum. This makes the code hard to read and
> difficult to extend.

I'd recommend you avoid alluding to future changes unless they're posted
on the mailing list.

> 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>

Sorry to nitpick, but maybe reword the changelog like so:

  KVM: arm64: Extract conditions for HPFAR_EL2 validity into helper

  __get_fault_info() open-codes checks for several conditions for the
  validity of HPFAR_EL2 based on the architecture as well as CPU errata
  workarounds. As these conditions are concatenated into a single if
  statement the result is somewhat difficult for the reader to parse.

  Improve the readability by extracting the conditional logic into a
  helper function. While at it, expand the predicates for the validity
  of HPFAR_EL2 into individual conditions.

  No functional change intended.

> ---
>  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.
>  	 */

This leaves the comment at a very odd indentation. Perhaps it'd be best
to interleave the comment with the below conditions? IMO it would do a
better job of documenting the code that way.

> +static inline bool __hpfar_is_valid(u64 esr)
> +{
> +	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

nit: rewrite to make the logic a bit more direct:

	if (__hpfar_is_valid(esr))
		hpfar = read_sysreg(hpfar_el2);
	else if (!__translate_far_to_hpfar(far, &hpfar))
		return false;

--
Thanks,
Oliver