Messages in this thread Patch in this message | | | Date | Thu, 21 Dec 2023 18:14:20 +0000 | From | Oliver Upton <> | Subject | Re: [PATCH v4 02/10] KVM: arm64: Add ptdump registration with debugfs for the stage-2 pagetables |
| |
On Mon, Dec 18, 2023 at 01:58:52PM +0000, Sebastian Ene wrote: > +config PTDUMP_STAGE2_DEBUGFS > + bool "Present the stage-2 pagetables to debugfs" > + depends on PTDUMP_DEBUGFS && KVM > + default n > + help > + Say Y here if you want to show the stage-2 kernel pagetables > + layout in a debugfs file. This information is only useful for kernel developers > + who are working in architecture specific areas of the kernel. > + It is probably not a good idea to enable this feature in a production > + kernel.
It isn't really a good idea to mount debugfs at all in a production system. There are already plenty worse interfaces lurking in that filesystem. The pKVM portions already depend on CONFIG_NVHE_EL2_DEBUG, so I don't see a need for this Kconfig option.
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index e5f75f1f1..ee8d7cb67 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -40,6 +40,7 @@ > #include <asm/kvm_pkvm.h> > #include <asm/kvm_emulate.h> > #include <asm/sections.h> > +#include <kvm_ptdump.h> > > #include <kvm/arm_hypercalls.h> > #include <kvm/arm_pmu.h> > @@ -2592,6 +2593,7 @@ static __init int kvm_arm_init(void) > if (err) > goto out_subs; > > + kvm_ptdump_register_host(); > kvm_arm_initialised = true; > > return 0; > diff --git a/arch/arm64/kvm/kvm_ptdump.h b/arch/arm64/kvm/kvm_ptdump.h > new file mode 100644 > index 000000000..98b595ce8 > --- /dev/null > +++ b/arch/arm64/kvm/kvm_ptdump.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +// > +// Copyright (C) Google, 2023 > +// Author: Sebastian Ene <sebastianene@google.com> You've got the comment styles backwards for these. The SPDX license uses the 'C++' style comment (//), whereas your multiline comment should always use a 'C' style comment (/* */). > +struct kvm_ptdump_register { > + void *(*get_ptdump_info)(struct kvm_ptdump_register *reg); > + void (*put_ptdump_info)(void *priv); > + int (*show_ptdump_info)(struct seq_file *m, void *v); > + void *priv; > +};
Please thoroughly consider the necessity of this. You're wrapping a callback structure with yet another callback structure. IMO, it would make a lot more sense to implement the file ops structure for every walker variant you need and avoid the indirection, it's hard to understand.
> +void kvm_ptdump_register_host(void) > +{ > + if (!is_protected_kvm_enabled()) > + return; > + > + kvm_ptdump_debugfs_register(&host_reg, "host_page_tables", > + kvm_debugfs_dir); > +} > + > +static int __init kvm_host_ptdump_init(void) > +{ > + host_reg.priv = (void *)host_s2_pgtable_pages(); > + return 0; > +} > + > +device_initcall(kvm_host_ptdump_init);
Why can't all of this be called from finalize_pkvm()?
> -- > 2.43.0.472.g3155946c3a-goog >
-- Thanks, Oliver
| |