While arch/*/mem/ptdump handles the kernel pagetable dumping code,
introduce KVM/ptdump which deals with the stage-2 pagetables. The
separation is necessary because most of the definitions from the
stage-2 pagetable reside in the KVM path and we will be invoking
functionality **specific** to KVM.
This registers a wrapper on top of debugfs_create_file which allows us
to hook callbacks on the debugfs open/show/close. The callbacks are used
to prepare the display portion of the pagetable dumping code.
Guard this functionality under the newly introduced PTDUMP_STAGE2_DEBUGFS.
Signed-off-by: Sebastian Ene <[email protected]>
---
arch/arm64/kvm/Kconfig | 13 +++++
arch/arm64/kvm/Makefile | 1 +
arch/arm64/kvm/arm.c | 2 +
arch/arm64/kvm/kvm_ptdump.h | 18 +++++++
arch/arm64/kvm/ptdump.c | 96 +++++++++++++++++++++++++++++++++++++
5 files changed, 130 insertions(+)
create mode 100644 arch/arm64/kvm/kvm_ptdump.h
create mode 100644 arch/arm64/kvm/ptdump.c
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 83c1e09be..0014e55e2 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -71,4 +71,17 @@ config PROTECTED_NVHE_STACKTRACE
If unsure, or not using protected nVHE (pKVM), say N.
+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.
+
+ If in doubt, say N.
+
endif # VIRTUALIZATION
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index c0c050e53..190eac175 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
vgic/vgic-its.o vgic/vgic-debug.o
kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o
+kvm-$(CONFIG_PTDUMP_STAGE2_DEBUGFS) += ptdump.o
always-y := hyp_constants.h hyp-constants.s
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 <[email protected]>
+
+#ifndef __KVM_PTDUMP_H
+#define __KVM_PTDUMP_H
+
+#include <asm/ptdump.h>
+
+
+#ifdef CONFIG_PTDUMP_STAGE2_DEBUGFS
+void kvm_ptdump_register_host(void);
+#else
+static inline void kvm_ptdump_register_host(void) { }
+#endif /* CONFIG_PTDUMP_STAGE2_DEBUGFS */
+
+#endif /* __KVM_PTDUMP_H */
diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
new file mode 100644
index 000000000..5816fc632
--- /dev/null
+++ b/arch/arm64/kvm/ptdump.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Debug helper used to dump the stage-2 pagetables of the system and their
+// associated permissions.
+//
+// Copyright (C) Google, 2023
+// Author: Sebastian Ene <[email protected]>
+
+#include <linux/debugfs.h>
+#include <linux/kvm_host.h>
+#include <linux/seq_file.h>
+
+#include <asm/kvm_pkvm.h>
+#include <kvm_ptdump.h>
+
+
+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;
+};
+
+static int kvm_ptdump_open(struct inode *inode, struct file *file);
+static int kvm_ptdump_release(struct inode *inode, struct file *file);
+static int kvm_ptdump_show(struct seq_file *m, void *);
+
+static const struct file_operations kvm_ptdump_fops = {
+ .open = kvm_ptdump_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = kvm_ptdump_release,
+};
+
+static int kvm_ptdump_open(struct inode *inode, struct file *file)
+{
+ struct kvm_ptdump_register *reg = inode->i_private;
+ void *info = NULL;
+ int ret;
+
+ if (reg->get_ptdump_info) {
+ info = reg->get_ptdump_info(reg);
+ if (!info)
+ return -ENOMEM;
+ }
+
+ if (!reg->show_ptdump_info)
+ reg->show_ptdump_info = kvm_ptdump_show;
+
+ ret = single_open(file, reg->show_ptdump_info, info);
+ if (ret && reg->put_ptdump_info)
+ reg->put_ptdump_info(info);
+
+ return ret;
+}
+
+static int kvm_ptdump_release(struct inode *inode, struct file *file)
+{
+ struct kvm_ptdump_register *reg = inode->i_private;
+ struct seq_file *seq_file = file->private_data;
+
+ if (reg->put_ptdump_info)
+ reg->put_ptdump_info(seq_file->private);
+
+ return 0;
+}
+
+static int kvm_ptdump_show(struct seq_file *m, void *)
+{
+ return -EINVAL;
+}
+
+static void kvm_ptdump_debugfs_register(struct kvm_ptdump_register *reg,
+ const char *name, struct dentry *parent)
+{
+ debugfs_create_file(name, 0400, parent, reg, &kvm_ptdump_fops);
+}
+
+static struct kvm_ptdump_register host_reg;
+
+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);
--
2.43.0.472.g3155946c3a-goog
On Mon, Dec 18, 2023 at 01:58:52PM +0000, Sebastian Ene wrote:
> While arch/*/mem/ptdump handles the kernel pagetable dumping code,
> introduce KVM/ptdump which deals with the stage-2 pagetables. The
> separation is necessary because most of the definitions from the
> stage-2 pagetable reside in the KVM path and we will be invoking
> functionality **specific** to KVM.
>
> This registers a wrapper on top of debugfs_create_file which allows us
> to hook callbacks on the debugfs open/show/close. The callbacks are used
> to prepare the display portion of the pagetable dumping code.
> Guard this functionality under the newly introduced PTDUMP_STAGE2_DEBUGFS.
>
> Signed-off-by: Sebastian Ene <[email protected]>
> ---
> arch/arm64/kvm/Kconfig | 13 +++++
> arch/arm64/kvm/Makefile | 1 +
> arch/arm64/kvm/arm.c | 2 +
> arch/arm64/kvm/kvm_ptdump.h | 18 +++++++
> arch/arm64/kvm/ptdump.c | 96 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 130 insertions(+)
> create mode 100644 arch/arm64/kvm/kvm_ptdump.h
> create mode 100644 arch/arm64/kvm/ptdump.c
>
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 83c1e09be..0014e55e2 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -71,4 +71,17 @@ config PROTECTED_NVHE_STACKTRACE
>
> If unsure, or not using protected nVHE (pKVM), say N.
>
> +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.
> +
> + If in doubt, say N.
> +
> endif # VIRTUALIZATION
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index c0c050e53..190eac175 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> vgic/vgic-its.o vgic/vgic-debug.o
>
> kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o
> +kvm-$(CONFIG_PTDUMP_STAGE2_DEBUGFS) += ptdump.o
>
> always-y := hyp_constants.h hyp-constants.s
>
> 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 <[email protected]>
> +
> +#ifndef __KVM_PTDUMP_H
> +#define __KVM_PTDUMP_H
> +
> +#include <asm/ptdump.h>
> +
> +
> +#ifdef CONFIG_PTDUMP_STAGE2_DEBUGFS
> +void kvm_ptdump_register_host(void);
> +#else
> +static inline void kvm_ptdump_register_host(void) { }
> +#endif /* CONFIG_PTDUMP_STAGE2_DEBUGFS */
> +
> +#endif /* __KVM_PTDUMP_H */
> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> new file mode 100644
> index 000000000..5816fc632
> --- /dev/null
> +++ b/arch/arm64/kvm/ptdump.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Debug helper used to dump the stage-2 pagetables of the system and their
> +// associated permissions.
> +//
> +// Copyright (C) Google, 2023
> +// Author: Sebastian Ene <[email protected]>
> +
> +#include <linux/debugfs.h>
> +#include <linux/kvm_host.h>
> +#include <linux/seq_file.h>
> +
> +#include <asm/kvm_pkvm.h>
> +#include <kvm_ptdump.h>
> +
> +
> +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;
> +};
> +
> +static int kvm_ptdump_open(struct inode *inode, struct file *file);
> +static int kvm_ptdump_release(struct inode *inode, struct file *file);
> +static int kvm_ptdump_show(struct seq_file *m, void *);
> +
> +static const struct file_operations kvm_ptdump_fops = {
> + .open = kvm_ptdump_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = kvm_ptdump_release,
> +};
> +
> +static int kvm_ptdump_open(struct inode *inode, struct file *file)
> +{
> + struct kvm_ptdump_register *reg = inode->i_private;
> + void *info = NULL;
> + int ret;
> +
> + if (reg->get_ptdump_info) {
> + info = reg->get_ptdump_info(reg);
> + if (!info)
> + return -ENOMEM;
> + }
> +
> + if (!reg->show_ptdump_info)
> + reg->show_ptdump_info = kvm_ptdump_show;
> +
> + ret = single_open(file, reg->show_ptdump_info, info);
> + if (ret && reg->put_ptdump_info)
> + reg->put_ptdump_info(info);
> +
> + return ret;
> +}
> +
> +static int kvm_ptdump_release(struct inode *inode, struct file *file)
> +{
> + struct kvm_ptdump_register *reg = inode->i_private;
> + struct seq_file *seq_file = file->private_data;
> +
> + if (reg->put_ptdump_info)
> + reg->put_ptdump_info(seq_file->private);
> +
Call single_release here.
> + return 0;
> +}
> +
> +static int kvm_ptdump_show(struct seq_file *m, void *)
> +{
> + return -EINVAL;
> +}
> +
> +static void kvm_ptdump_debugfs_register(struct kvm_ptdump_register *reg,
> + const char *name, struct dentry *parent)
> +{
> + debugfs_create_file(name, 0400, parent, reg, &kvm_ptdump_fops);
> +}
> +
> +static struct kvm_ptdump_register host_reg;
> +
> +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);
> --
> 2.43.0.472.g3155946c3a-goog
>
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 <[email protected]>
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
On Thu, Dec 21, 2023 at 06:14:20PM +0000, Oliver Upton wrote:
Hi Oliver,
I am planning to split the series based on your suggestion and I
wanted to make sure that I understand your feedback.
> 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.
>
I created a separate option because I wanted to re-use the parsing
functionality from the already existing ptdump code for EL1. This option
is turned off in production and only enabled for debug.
I was thinking to make use of the `CONFIG_NVHE_EL2_DEBUG` but then I abandoned
this ideea as one can use ptdump for vHE as well.
> > 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 <[email protected]>
>
> 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 (/* */).
>
Let me fix this, thanks.
> > +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.
>
I think we can drop this and have different file_ops.
> > +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()?
>
I guess it can be called from finalize_pkvm before the is_protected_kvm_enabled
check. This should work for nvhe & vhe as well.
Thanks,
Seb
> > --
> > 2.43.0.472.g3155946c3a-goog
> >
>
> --
> Thanks,
> Oliver
On Thu, Feb 01, 2024 at 11:20:53AM +0000, Sebastian Ene wrote:
> On Thu, Dec 21, 2023 at 06:14:20PM +0000, Oliver Upton wrote:
>
> Hi Oliver,
>
> I am planning to split the series based on your suggestion and I
> wanted to make sure that I understand your feedback.
>
> > 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.
> >
>
> I created a separate option because I wanted to re-use the parsing
> functionality from the already existing ptdump code for EL1. This option
> is turned off in production and only enabled for debug.
>
> I was thinking to make use of the `CONFIG_NVHE_EL2_DEBUG` but then I abandoned
> this ideea as one can use ptdump for vHE as well.
Fair enough. I was going to say we could just have KVM follow
CONFIG_PTDUMP_DEBUGFS, but it doesn't matter either way.
> > > +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()?
> >
>
> I guess it can be called from finalize_pkvm before the is_protected_kvm_enabled
> check. This should work for nvhe & vhe as well.
What does nvhe and vhe modes have to do with it? I thought this was for
hooking up the host's S2, which does not exist outside protected mode.
--
Thanks,
Oliver
On Mon, Feb 05, 2024 at 01:14:27PM +0000, Oliver Upton wrote:
> On Thu, Feb 01, 2024 at 11:20:53AM +0000, Sebastian Ene wrote:
> > On Thu, Dec 21, 2023 at 06:14:20PM +0000, Oliver Upton wrote:
> >
> > Hi Oliver,
> >
> > I am planning to split the series based on your suggestion and I
> > wanted to make sure that I understand your feedback.
> >
> > > 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.
> > >
> >
> > I created a separate option because I wanted to re-use the parsing
> > functionality from the already existing ptdump code for EL1. This option
> > is turned off in production and only enabled for debug.
> >
> > I was thinking to make use of the `CONFIG_NVHE_EL2_DEBUG` but then I abandoned
> > this ideea as one can use ptdump for vHE as well.
>
> Fair enough. I was going to say we could just have KVM follow
> CONFIG_PTDUMP_DEBUGFS, but it doesn't matter either way.
>
> > > > +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()?
> > >
> >
> > I guess it can be called from finalize_pkvm before the is_protected_kvm_enabled
> > check. This should work for nvhe & vhe as well.
>
> What does nvhe and vhe modes have to do with it? I thought this was for
> hooking up the host's S2, which does not exist outside protected mode.
>
True I guess there is no other need for the initialization portion in
this function. I will split the series to address the non-protected
support first.
Thanks,
Seb
> --
> Thanks,
> Oliver