Re: [PATCH v2 6/8] arm64: stacktrace: remove stack type from fp translator

Will Deacon
2022-08-08T12:05:55Z

On Fri, Aug 05, 2022 at 01:45:20PM +0100, Mark Rutland wrote:
> In subsequent patches we'll remove the stack_type enum, and move the FP
> translation logic out of the raw FP unwind code.
>
> In preparation for doing so, this patch removes the type parameter from
> the FP translation callback, and modifies kvm_nvhe_stack_kern_va() to
> determine the relevant stack directly.
>
> So that kvm_nvhe_stack_kern_va() can use the stackinfo_*() helpers,
> these are moved earlier in the file, but are not modified in any way.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/include/asm/stacktrace/common.h | 6 +-
> arch/arm64/kvm/stacktrace.c | 78 ++++++++++++----------
> 2 files changed, 43 insertions(+), 41 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0071f2459c703..0bd9d7ad295e0 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -114,13 +114,11 @@ static inline void unwind_init_common(struct unwind_state *state,
> * a kernel address.
> *
> * @fp: the frame pointer to be updated to its kernel address.
> - * @type: the stack type associated with frame pointer @fp
> *
> * Returns true and success and @fp is updated to the corresponding
> * kernel virtual address; otherwise returns false.
> */
> -typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp,
> - enum stack_type type);
> +typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
>
> /*
> * on_accessible_stack_fn() - Check whether a stack range is on any
> @@ -165,7 +163,7 @@ unwind_next_frame_record(struct unwind_state *state,
> * If fp is not from the current address space perform the necessary
> * translation before dereferencing it to get the next fp.
> */
> - if (translate_fp && !translate_fp(&kern_fp, info.type))
> + if (translate_fp && !translate_fp(&kern_fp))
> return -EINVAL;
>
> /*
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index 26927344a2632..abdc231ae70fd 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -21,6 +21,34 @@
>
> #include <asm/stacktrace/nvhe.h>
>
> +static struct stack_info stackinfo_get_overflow(void)
> +{
> + struct kvm_nvhe_stacktrace_info *stacktrace_info
> + = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> + unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
> + unsigned long high = low + OVERFLOW_STACK_SIZE;
> +
> + return (struct stack_info) {
> + .low = low,
> + .high = high,
> + .type = STACK_TYPE_OVERFLOW,
> + };
> +}
> +
> +static struct stack_info stackinfo_get_hyp(void)
> +{
> + struct kvm_nvhe_stacktrace_info *stacktrace_info
> + = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> + unsigned long low = (unsigned long)stacktrace_info->stack_base;
> + unsigned long high = low + PAGE_SIZE;
> +
> + return (struct stack_info) {
> + .low = low,
> + .high = high,
> + .type = STACK_TYPE_HYP,
> + };
> +}
> +
> /*
> * kvm_nvhe_stack_kern_va - Convert KVM nVHE HYP stack addresses to a kernel VAs
> *
> @@ -34,27 +62,31 @@
> * Returns true on success and updates @addr to its corresponding kernel VA;
> * otherwise returns false.
> */
> -static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
> - enum stack_type type)
> +static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
> {
> struct kvm_nvhe_stacktrace_info *stacktrace_info;
> unsigned long hyp_base, kern_base, hyp_offset;
> + struct stack_info stack;
>
> stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
>
> - switch (type) {
> - case STACK_TYPE_HYP:
> + stack = stackinfo_get_hyp();
> + if (stackinfo_on_stack(&stack, *addr, 1)) {
> kern_base = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
> hyp_base = (unsigned long)stacktrace_info->stack_base;
> - break;
> - case STACK_TYPE_OVERFLOW:
> + goto found;
> + }
> +
> + stack = stackinfo_get_overflow();
> + if (stackinfo_on_stack(&stack, *addr, 1)) {

Why is '1' the appropriate size for these two checks, given that addr points
at an unsigned long?

Will