lkml.org 
[lkml]   [2023]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-12-21 19:15    [W:4.382 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site