lkml.org 
[lkml]   [2022]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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@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

\
 
 \ /
  Last update: 2022-09-27 07:15    [W:0.399 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site