| Message ID | 20220927002715.2142353-1-mizhang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Cleanup the __get_fault_info() to take out the code that validates HPFAR | expand |
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
Hi Mingwei, On Mon, Sep 26, 2022 at 5:27 PM Mingwei Zhang <mizhang@google.com> 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@google.com> > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > 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@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Tue, 27 Sep 2022 01:14:16 -0400, Oliver Upton <oliver.upton@linux.dev> 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.
> > +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 <mizhang@google.com> 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.
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;
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@google.com> Signed-off-by: Mingwei Zhang <mizhang@google.com> --- arch/arm64/kvm/hyp/include/hyp/fault.h | 36 ++++++++++++++++---------- 1 file changed, 23 insertions(+), 13 deletions(-) base-commit: c59fb127583869350256656b7ed848c398bef879