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 <[email protected]>
Signed-off-by: Mingwei Zhang <[email protected]>
---
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)
+{
+ 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
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 <[email protected]>
> Signed-off-by: Mingwei Zhang <[email protected]>
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
Hi Mingwei,
On Mon, Sep 26, 2022 at 5:27 PM Mingwei Zhang <[email protected]> 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 <[email protected]>
> Signed-off-by: Mingwei Zhang <[email protected]>
> ---
> 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
> [email protected]
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Tue, 27 Sep 2022 01:14:16 -0400,
Oliver Upton <[email protected]> wrote:
>
> 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.
Honestly, I'd refrain from such changes *unless* they enable something
else. The current code is well understood by people hacking on it, and
although I don't mind revamping it, it has to be for a good reason.
I'd be much more receptive to such a change if it was a prefix to
something that actually made a significant change.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
> > +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
>
Yeah, I agree with you Reiji that the name does not reflect the
meaning of the function. So I was thinking about other names like
__translate_hpfar_to_far_needed().
>
> Honestly, I'd refrain from such changes *unless* they enable something
> else. The current code is well understood by people hacking on it, and
> although I don't mind revamping it, it has to be for a good reason.
>
> I'd be much more receptive to such a change if it was a prefix to
> something that actually made a significant change.
>
> Thanks,
>
> M.
>
Hi Marc,
Thanks for the feedback. I am not sure about the style of the KVM ARM
side. But in general I think mixing the generic code for ARM and
specific CPU errata handling is misleading. For instance, in this
case:
+ if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
+ return false;
+
+ if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
+ return false;
As shown it would be much cleaner to separate the two cases as the
former case is suggested in ARMv8 Spec D13.2.55. The latter case would
definitely come from a different source.
But I also don't have a strong opinion pushing this one. So, let me
pull it back then :)
Mingwei,
On Tue, 27 Sep 2022 13:48:52 -0400,
Mingwei Zhang <[email protected]> wrote:
>
> >
> > Honestly, I'd refrain from such changes *unless* they enable something
> > else. The current code is well understood by people hacking on it, and
> > although I don't mind revamping it, it has to be for a good reason.
> >
> > I'd be much more receptive to such a change if it was a prefix to
> > something that actually made a significant change.
> >
> > Thanks,
> >
> > M.
> >
> Hi Marc,
>
> Thanks for the feedback. I am not sure about the style of the KVM ARM
> side. But in general I think mixing the generic code for ARM and
> specific CPU errata handling is misleading. For instance, in this
> case:
>
> + if ((esr & ESR_ELx_FSC_TYPE) == FSC_PERM)
> + return false;
> +
> + if (cpus_have_final_cap(ARM64_WORKAROUND_834220))
> + return false;
>
> As shown it would be much cleaner to separate the two cases as the
> former case is suggested in ARMv8 Spec D13.2.55. The latter case would
> definitely come from a different source.
I think we're talking at cross purposes. I don't object to the change
per se. I simply question its value *in isolation*. One of the many
things that makes the kernel hard to maintain is churn. Refactoring
just for the sake of it *is* churn. In this case, cosmetic churn.
But if you make this is part of something touching this area and
improving things from a functional perspective, then I'll happily
merge it.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.