diff mbox series

[v7,6/6] KVM: arm64: Symbolize the nVHE HYP addresses

Message ID 20220408200349.1529080-7-kaleshsingh@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Hypervisor stack enhancements | expand

Commit Message

Kalesh Singh April 8, 2022, 8:03 p.m. UTC 
Reintroduce the __kvm_nvhe_ symbols in kallsyms, ignoring the local
symbols in this namespace. The local symbols are not informative and
can cause aliasing issues when symbolizing the addresses.

With the necessary symbols now in kallsyms we can symbolize nVHE
addresses using the %p print format specifier:

[   98.916444][  T426] kvm [426]: nVHE hyp panic at: [<ffffffc0096156fc>] __kvm_nvhe_overflow_stack+0x8/0x34!

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
Tested-by: Fuad Tabba <tabba@google.com>
Reviewed-by: Fuad Tabba <tabba@google.com>
---

Changes in v6:
  - Add Fuad's Reviewed-by and Tested-by tags.
  
Changes in v2:
  - Fix printk warnings - %p expects (void *)


 arch/arm64/kvm/handle_exit.c | 13 +++++--------
 scripts/kallsyms.c           |  2 +-
 2 files changed, 6 insertions(+), 9 deletions(-)

Comments

Marc Zyngier April 18, 2022, 10:16 a.m. UTC | #1 
On Fri, 08 Apr 2022 21:03:29 +0100,
Kalesh Singh <kaleshsingh@google.com> wrote:
> 
> Reintroduce the __kvm_nvhe_ symbols in kallsyms, ignoring the local
> symbols in this namespace. The local symbols are not informative and
> can cause aliasing issues when symbolizing the addresses.
> 
> With the necessary symbols now in kallsyms we can symbolize nVHE
> addresses using the %p print format specifier:
> 
> [   98.916444][  T426] kvm [426]: nVHE hyp panic at: [<ffffffc0096156fc>] __kvm_nvhe_overflow_stack+0x8/0x34!
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> Tested-by: Fuad Tabba <tabba@google.com>
> Reviewed-by: Fuad Tabba <tabba@google.com>
> ---
> 
> Changes in v6:
>   - Add Fuad's Reviewed-by and Tested-by tags.
>   
> Changes in v2:
>   - Fix printk warnings - %p expects (void *)
> 
> 
>  arch/arm64/kvm/handle_exit.c | 13 +++++--------
>  scripts/kallsyms.c           |  2 +-
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 97fe14aab1a3..a377b871bf58 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -295,13 +295,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
>  	u64 elr_in_kimg = __phys_to_kimg(elr_phys);
>  	u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr_virt;
>  	u64 mode = spsr & PSR_MODE_MASK;
> +	u64 panic_addr = elr_virt + hyp_offset;
>  
> -	/*
> -	 * The nVHE hyp symbols are not included by kallsyms to avoid issues
> -	 * with aliasing. That means that the symbols cannot be printed with the
> -	 * "%pS" format specifier, so fall back to the vmlinux address if
> -	 * there's no better option.
> -	 */
>  	if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
>  		kvm_err("Invalid host exception to nVHE hyp!\n");
>  	} else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> @@ -321,9 +316,11 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
>  		if (file)
>  			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
>  		else
> -			kvm_err("nVHE hyp BUG at: %016llx!\n", elr_virt + hyp_offset);
> +			kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
> +					(void *)panic_addr);
>  	} else {
> -		kvm_err("nVHE hyp panic at: %016llx!\n", elr_virt + hyp_offset);
> +		kvm_err("nVHE hyp panic at: [<%016llx>] %pB!\n", panic_addr,
> +				(void *)panic_addr);
>  	}
>  
>  	/*
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 8caabddf817c..ad2c93640a92 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -111,7 +111,7 @@ static bool is_ignored_symbol(const char *name, char type)
>  		".L",			/* local labels, .LBB,.Ltmpxxx,.L__unnamed_xx,.LASANPC, etc. */
>  		"__crc_",		/* modversions */
>  		"__efistub_",		/* arm64 EFI stub namespace */
> -		"__kvm_nvhe_",		/* arm64 non-VHE KVM namespace */
> +		"__kvm_nvhe_$",		/* arm64 local symbols in non-VHE KVM namespace */
>  		"__AArch64ADRPThunk_",	/* arm64 lld */
>  		"__ARMV5PILongThunk_",	/* arm lld */
>  		"__ARMV7PILongThunk_",

If you are sanitising this, shouldn'tt you also apply the same rules
as the rest of the kernel for non-'__-kvm_nvhe_' symbols? For example,
I see a long list of .L* symbols:

0000000000000000 r __kvm_nvhe_.L144721
0000000000000090 r __kvm_nvhe_.L144721
00000000000000b4 r __kvm_nvhe_.L144721
00000000000004b0 r __kvm_nvhe_.L144721
000000000000051c r __kvm_nvhe_.L144721
0000000000000650 r __kvm_nvhe_.L144721
0000000000000694 r __kvm_nvhe_.L144721
0000000000000761 r __kvm_nvhe_.L144721
00000000000007a7 r __kvm_nvhe_.L144721
00000000000007c7 r __kvm_nvhe_.L144721
0000000000000814 r __kvm_nvhe_.L144721
0000000000000b08 r __kvm_nvhe_.L144721
0000000000003200 r __kvm_nvhe_.L144721

(83 of them in total on a local build) that I think should also be
trimmed.

Thanks,

	M.
Kalesh Singh April 19, 2022, 2:42 a.m. UTC | #2 
On Mon, Apr 18, 2022 at 3:16 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 08 Apr 2022 21:03:29 +0100,
> Kalesh Singh <kaleshsingh@google.com> wrote:
> >
> > Reintroduce the __kvm_nvhe_ symbols in kallsyms, ignoring the local
> > symbols in this namespace. The local symbols are not informative and
> > can cause aliasing issues when symbolizing the addresses.
> >
> > With the necessary symbols now in kallsyms we can symbolize nVHE
> > addresses using the %p print format specifier:
> >
> > [   98.916444][  T426] kvm [426]: nVHE hyp panic at: [<ffffffc0096156fc>] __kvm_nvhe_overflow_stack+0x8/0x34!
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > Tested-by: Fuad Tabba <tabba@google.com>
> > Reviewed-by: Fuad Tabba <tabba@google.com>
> > ---
> >
> > Changes in v6:
> >   - Add Fuad's Reviewed-by and Tested-by tags.
> >
> > Changes in v2:
> >   - Fix printk warnings - %p expects (void *)
> >
> >
> >  arch/arm64/kvm/handle_exit.c | 13 +++++--------
> >  scripts/kallsyms.c           |  2 +-
> >  2 files changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index 97fe14aab1a3..a377b871bf58 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -295,13 +295,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> >       u64 elr_in_kimg = __phys_to_kimg(elr_phys);
> >       u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr_virt;
> >       u64 mode = spsr & PSR_MODE_MASK;
> > +     u64 panic_addr = elr_virt + hyp_offset;
> >
> > -     /*
> > -      * The nVHE hyp symbols are not included by kallsyms to avoid issues
> > -      * with aliasing. That means that the symbols cannot be printed with the
> > -      * "%pS" format specifier, so fall back to the vmlinux address if
> > -      * there's no better option.
> > -      */
> >       if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
> >               kvm_err("Invalid host exception to nVHE hyp!\n");
> >       } else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> > @@ -321,9 +316,11 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> >               if (file)
> >                       kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
> >               else
> > -                     kvm_err("nVHE hyp BUG at: %016llx!\n", elr_virt + hyp_offset);
> > +                     kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
> > +                                     (void *)panic_addr);
> >       } else {
> > -             kvm_err("nVHE hyp panic at: %016llx!\n", elr_virt + hyp_offset);
> > +             kvm_err("nVHE hyp panic at: [<%016llx>] %pB!\n", panic_addr,
> > +                             (void *)panic_addr);
> >       }
> >
> >       /*
> > diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> > index 8caabddf817c..ad2c93640a92 100644
> > --- a/scripts/kallsyms.c
> > +++ b/scripts/kallsyms.c
> > @@ -111,7 +111,7 @@ static bool is_ignored_symbol(const char *name, char type)
> >               ".L",                   /* local labels, .LBB,.Ltmpxxx,.L__unnamed_xx,.LASANPC, etc. */
> >               "__crc_",               /* modversions */
> >               "__efistub_",           /* arm64 EFI stub namespace */
> > -             "__kvm_nvhe_",          /* arm64 non-VHE KVM namespace */
> > +             "__kvm_nvhe_$",         /* arm64 local symbols in non-VHE KVM namespace */
> >               "__AArch64ADRPThunk_",  /* arm64 lld */
> >               "__ARMV5PILongThunk_",  /* arm lld */
> >               "__ARMV7PILongThunk_",
>
> If you are sanitising this, shouldn'tt you also apply the same rules
> as the rest of the kernel for non-'__-kvm_nvhe_' symbols? For example,
> I see a long list of .L* symbols:
>
> 0000000000000000 r __kvm_nvhe_.L144721
> 0000000000000090 r __kvm_nvhe_.L144721
> 00000000000000b4 r __kvm_nvhe_.L144721
> 00000000000004b0 r __kvm_nvhe_.L144721
> 000000000000051c r __kvm_nvhe_.L144721
> 0000000000000650 r __kvm_nvhe_.L144721
> 0000000000000694 r __kvm_nvhe_.L144721
> 0000000000000761 r __kvm_nvhe_.L144721
> 00000000000007a7 r __kvm_nvhe_.L144721
> 00000000000007c7 r __kvm_nvhe_.L144721
> 0000000000000814 r __kvm_nvhe_.L144721
> 0000000000000b08 r __kvm_nvhe_.L144721
> 0000000000003200 r __kvm_nvhe_.L144721
>
> (83 of them in total on a local build) that I think should also be
> trimmed.

Good catch. I’ll fix it in the next version along with your other
comments. Thanks for the reviews Mark.

-Kalesh

>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..a377b871bf58 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -295,13 +295,8 @@  void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 	u64 elr_in_kimg = __phys_to_kimg(elr_phys);
 	u64 hyp_offset = elr_in_kimg - kaslr_offset() - elr_virt;
 	u64 mode = spsr & PSR_MODE_MASK;
+	u64 panic_addr = elr_virt + hyp_offset;
 
-	/*
-	 * The nVHE hyp symbols are not included by kallsyms to avoid issues
-	 * with aliasing. That means that the symbols cannot be printed with the
-	 * "%pS" format specifier, so fall back to the vmlinux address if
-	 * there's no better option.
-	 */
 	if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
 		kvm_err("Invalid host exception to nVHE hyp!\n");
 	} else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
@@ -321,9 +316,11 @@  void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 		if (file)
 			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
 		else
-			kvm_err("nVHE hyp BUG at: %016llx!\n", elr_virt + hyp_offset);
+			kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
+					(void *)panic_addr);
 	} else {
-		kvm_err("nVHE hyp panic at: %016llx!\n", elr_virt + hyp_offset);
+		kvm_err("nVHE hyp panic at: [<%016llx>] %pB!\n", panic_addr,
+				(void *)panic_addr);
 	}
 
 	/*
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 8caabddf817c..ad2c93640a92 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -111,7 +111,7 @@  static bool is_ignored_symbol(const char *name, char type)
 		".L",			/* local labels, .LBB,.Ltmpxxx,.L__unnamed_xx,.LASANPC, etc. */
 		"__crc_",		/* modversions */
 		"__efistub_",		/* arm64 EFI stub namespace */
-		"__kvm_nvhe_",		/* arm64 non-VHE KVM namespace */
+		"__kvm_nvhe_$",		/* arm64 local symbols in non-VHE KVM namespace */
 		"__AArch64ADRPThunk_",	/* arm64 lld */
 		"__ARMV5PILongThunk_",	/* arm lld */
 		"__ARMV7PILongThunk_",