linux-kernel - Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YzKGqLzjv0I/gkMi@google.com>
Date:   Mon, 26 Sep 2022 22:14:16 -0700
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Mingwei Zhang <mizhang@...gle.com>
Cc:     Marc Zyngier <maz@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: arm64: Cleanup the __get_fault_info() to take out
 the code that validates HPFAR

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@...gle.com>
> Signed-off-by: Mingwei Zhang <mizhang@...gle.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