Received-SPF: pass (google.com: domain of linux-kernel+bounces-48004-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 1 Feb 2024 11:20:53 +0000
From: Sebastian Ene <sebastianene@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: will@kernel.org, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>, catalin.marinas@arm.com,
	mark.rutland@arm.com, akpm@linux-foundation.org, maz@kernel.org,
	kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kernel-team@android.com,
	vdonnefort@google.com, qperret@google.com, smostafa@google.com
Subject: Re: [PATCH v4 02/10] KVM: arm64: Add ptdump registration with
 debugfs for the stage-2 pagetables
Message-ID: <Zbt-leieTD64ZefR@google.com>
References: <20231218135859.2513568-2-sebastianene@google.com>
 <20231218135859.2513568-4-sebastianene@google.com>
 <ZYSAfORj2-cXo5t_@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZYSAfORj2-cXo5t_@linux.dev>

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 <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 (/* */).
>

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