| Message ID | 1461343688-8552-7-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 22, 2016 at 06:48:08PM +0200, Ard Biesheuvel wrote: > From: Mark Rutland <mark.rutland@arm.com> > > Occasionally it's useful to inspect the page tables used for EFI runtime > services, similarly to those cases where it's useful to be able to > inspect the kernel page tables. > > Now that the kernel page table dump code has been made generic, use it > to export the EFI runtime services page tables under debugfs (as > "efi_page_tables"). > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > [ ardb: mildly refactored for compatibility with ARM ] > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/firmware/efi/arm-runtime.c | 21 ++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 6ae21e41a429..c48fc2f5413b 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -27,6 +27,7 @@ > #include <asm/mmu.h> > #include <asm/pgalloc.h> > #include <asm/pgtable.h> > +#include <asm/ptdump.h> > > extern u64 efi_system_table; > > @@ -133,3 +134,23 @@ void efi_virtmap_unload(void) > efi_set_pgd(current->active_mm); > preempt_enable(); > } > + > +static const struct addr_marker efi_addr_markers[] = { > + { 0, "EFI runtime services" }, > + { -1 }, > +}; > + > +static struct ptdump_info efi_ptdump_info = { > + .mm = &efi_mm, > + .markers = efi_addr_markers, > + .base_addr = 0, > + .max_addr = SZ_1G, > +}; I see that max_addr isn't used for arm64, and for ARM it's only used in one place. It doesn't seem great to have that on arm64 given it's unused. Do we actually need max_addr? Is there any reason not to always dump whole tables? I guess you're trying to avoid dumping the kernel VA range on 32-bit? Thanks, Mark. > + > +static int efi_ptdump_init(void) > +{ > + if (!efi_enabled(EFI_RUNTIME_SERVICES)) > + return 0; > + return ptdump_register(&efi_ptdump_info, "efi_page_tables"); > +} > +device_initcall(efi_ptdump_init); > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 22 April 2016 at 19:01, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Apr 22, 2016 at 06:48:08PM +0200, Ard Biesheuvel wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> >> Occasionally it's useful to inspect the page tables used for EFI runtime >> services, similarly to those cases where it's useful to be able to >> inspect the kernel page tables. >> >> Now that the kernel page table dump code has been made generic, use it >> to export the EFI runtime services page tables under debugfs (as >> "efi_page_tables"). >> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> [ ardb: mildly refactored for compatibility with ARM ] >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/firmware/efi/arm-runtime.c | 21 ++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c >> index 6ae21e41a429..c48fc2f5413b 100644 >> --- a/drivers/firmware/efi/arm-runtime.c >> +++ b/drivers/firmware/efi/arm-runtime.c >> @@ -27,6 +27,7 @@ >> #include <asm/mmu.h> >> #include <asm/pgalloc.h> >> #include <asm/pgtable.h> >> +#include <asm/ptdump.h> >> >> extern u64 efi_system_table; >> >> @@ -133,3 +134,23 @@ void efi_virtmap_unload(void) >> efi_set_pgd(current->active_mm); >> preempt_enable(); >> } >> + >> +static const struct addr_marker efi_addr_markers[] = { >> + { 0, "EFI runtime services" }, >> + { -1 }, >> +}; >> + >> +static struct ptdump_info efi_ptdump_info = { >> + .mm = &efi_mm, >> + .markers = efi_addr_markers, >> + .base_addr = 0, >> + .max_addr = SZ_1G, >> +}; > > I see that max_addr isn't used for arm64, and for ARM it's only used in > one place. It doesn't seem great to have that on arm64 given it's > unused. > > Do we actually need max_addr? Is there any reason not to always dump > whole tables? > > I guess you're trying to avoid dumping the kernel VA range on 32-bit? > Indeed. On ARM, the efi_page_tables dumps its copy of kernel_page_tables. If that is OK (the information could potentially be useful, I suppose) we can drop the max _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Apr 22, 2016 at 07:20:38PM +0200, Ard Biesheuvel wrote: > On 22 April 2016 at 19:01, Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Apr 22, 2016 at 06:48:08PM +0200, Ard Biesheuvel wrote: > >> +static struct ptdump_info efi_ptdump_info = { > >> + .mm = &efi_mm, > >> + .markers = efi_addr_markers, > >> + .base_addr = 0, > >> + .max_addr = SZ_1G, > >> +}; > > > > I see that max_addr isn't used for arm64, and for ARM it's only used in > > one place. It doesn't seem great to have that on arm64 given it's > > unused. > > > > Do we actually need max_addr? Is there any reason not to always dump > > whole tables? > > > > I guess you're trying to avoid dumping the kernel VA range on 32-bit? > > Indeed. On ARM, the efi_page_tables dumps its copy of > kernel_page_tables. If that is OK (the information could potentially > be useful, I suppose) we can drop the max I'd argue it's always worth dumping the full tables (dropipng the max). For instance, we could have some bug that leaves the kernel VA range in the EFI tables inconsistent with what we expect/require, and having that exposed would make that clear. Regardless, arbitrarily limiting the VA range of the arm64 dump doesn't seem great. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 22 April 2016 at 19:25, Mark Rutland <mark.rutland@arm.com> wrote: > On Fri, Apr 22, 2016 at 07:20:38PM +0200, Ard Biesheuvel wrote: >> On 22 April 2016 at 19:01, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Fri, Apr 22, 2016 at 06:48:08PM +0200, Ard Biesheuvel wrote: > >> >> +static struct ptdump_info efi_ptdump_info = { >> >> + .mm = &efi_mm, >> >> + .markers = efi_addr_markers, >> >> + .base_addr = 0, >> >> + .max_addr = SZ_1G, >> >> +}; >> > >> > I see that max_addr isn't used for arm64, and for ARM it's only used in >> > one place. It doesn't seem great to have that on arm64 given it's >> > unused. >> > >> > Do we actually need max_addr? Is there any reason not to always dump >> > whole tables? >> > >> > I guess you're trying to avoid dumping the kernel VA range on 32-bit? >> >> Indeed. On ARM, the efi_page_tables dumps its copy of >> kernel_page_tables. If that is OK (the information could potentially >> be useful, I suppose) we can drop the max > > I'd argue it's always worth dumping the full tables (dropipng the max). > > For instance, we could have some bug that leaves the kernel VA range in > the EFI tables inconsistent with what we expect/require, and having that > exposed would make that clear. > Well, the problem here is that we only sync the EFI page tables with the kernel page tables in switch_mm() (if they turn out to be out of sync). This means we should either sync them in this code path as well, or remove the kernel VA range from the output, since we'll be dumping known stale data otherwise. > Regardless, arbitrarily limiting the VA range of the arm64 dump doesn't > seem great. > True. [0 .. TASK_SIZE) looks like a useful and not arbitrary range to limit the EFI page table output to. Unfortunately, TASK_SIZE resolves to a value that depends on TIF_32BIT, so will still need some preprocessor conditional to select between TASK_SIZE_64 (on arm64) and TASK_SIZE (on ARM) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c index 6ae21e41a429..c48fc2f5413b 100644 --- a/drivers/firmware/efi/arm-runtime.c +++ b/drivers/firmware/efi/arm-runtime.c @@ -27,6 +27,7 @@ #include <asm/mmu.h> #include <asm/pgalloc.h> #include <asm/pgtable.h> +#include <asm/ptdump.h> extern u64 efi_system_table; @@ -133,3 +134,23 @@ void efi_virtmap_unload(void) efi_set_pgd(current->active_mm); preempt_enable(); } + +static const struct addr_marker efi_addr_markers[] = { + { 0, "EFI runtime services" }, + { -1 }, +}; + +static struct ptdump_info efi_ptdump_info = { + .mm = &efi_mm, + .markers = efi_addr_markers, + .base_addr = 0, + .max_addr = SZ_1G, +}; + +static int efi_ptdump_init(void) +{ + if (!efi_enabled(EFI_RUNTIME_SERVICES)) + return 0; + return ptdump_register(&efi_ptdump_info, "efi_page_tables"); +} +device_initcall(efi_ptdump_init);