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@xxxxxxxxxx>
> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
