Hi Mingwei,
On Mon, Sep 26, 2022 at 5:27 PM Mingwei Zhang <mizhang@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
