From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 804EDC47DDF for ; Thu, 1 Feb 2024 11:21:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=5EyMCU9a1R5OlHNYFgVdlQKldKFPpJsXoAvEN0Px8eI=; b=kL/fH16AEE1sXC i2qxlo3iV8Qe9vXI8JcT2dla4ML3g1e6TY5KqvQjaQTTQ9cSi/5lRjZsXJ1p1uPUo49ZGaXJOxiek znLfKnz+BSoN51BLcdPrHKvlUOziDItok2xZwD1ZZu2XIW+6uMd6sRenfqR/gHNCIswKeOG4maYdQ lYi8wmqn3OTbUPzXSYkMNrKIvl4mzsrhrrEMz2+3h1gqmZOzPhdoUQ/zjgUscoyFfJ/+2mtxZnsmI Ar0oRoqA4hn+YMXTumeK/HhzKLsQVH9aUMFJiiCqE8XpbZBuCHBom+AN9LKTjjAt3JD+p8ERvKiY6 hc/zDtR44zfA48KcykZA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVV7u-00000007Z7R-2DUU; Thu, 01 Feb 2024 11:21:02 +0000 Received: from mail-qt1-x836.google.com ([2607:f8b0:4864:20::836]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVV7s-00000007Z73-0gjM for linux-arm-kernel@lists.infradead.org; Thu, 01 Feb 2024 11:21:01 +0000 Received: by mail-qt1-x836.google.com with SMTP id d75a77b69052e-428405a0205so251661cf.1 for ; Thu, 01 Feb 2024 03:20:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1706786458; x=1707391258; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=fqzrZh/phrQ2srQm2BnmFIw+lB3QB2LYN941NxOH46g=; b=ttNp5fWcAdqRLPP2X/2g8pcT+zooG4wv5xS9dj3kd1sEDnqlUDIDtE/Awpm1BlQzJB k/5lAIbFvtgcKVj7A1Z85Y2rRCe4/+y9JZPpBOdZBJRTAZD50vSR1Isd0qjTutxXH7k3 0mHAcnlfU/mh62QNwi83aljXnuIR6nRm7EJGTUP7MdS+WVEBr2C0LIn+4UGAd4ARTfHk 5IQ8Qnz0UEzYQSirkfU4jdqxsYdBlHn+T2aufBtKh5HIF0OU5UgfOuvcGkz2D9rhqTfo yTblCijYNEE3fkk1PHk8HD9J/J07BBsQ+mhcD//CpolyeHcBaKddjBSN8+Wb26cF3j4M Hmiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706786458; x=1707391258; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=fqzrZh/phrQ2srQm2BnmFIw+lB3QB2LYN941NxOH46g=; b=WTd8jPcHLrLNXIO/oo5x3pQtjsJOe32WNwpYFavbJ7pH7bU4SWpIOkTZMW9jft+TUk KfNjhyvgvKNQHIzaEbrHd6wpyFiv7yf0onLLJ233PJyNHJ3fBh+GA0L304ODuAQ6YbLi NdP/o/xaKvGjw47Zmi1GroYEgkPlrT/mNrip8XJSaFOnx08aTCy2kDRoabHKoIDK7V6d BCybTyceuW8+Y/mNxnZUkjQOV7dWjZBFqYZFtvbY35QCkHaZdJgG+WzOjC/kAwRLEQXe qaUlxoMg8VXarzgfZrOuX1Q586bPgXXjvXhzSQwGakw6VnnHkIrHlmuNLz9YATA7ICHM uWPg== X-Gm-Message-State: AOJu0YzRbayYIMItApyEF3aGvkqAfFTKSY5cABCy5pNwT/eAK64o6kNe aKkmFZ4ON2soqggsjrZAly8OlNHfoYdc9DHhhzTTa553uunSZiNPlNenjerKnw== X-Google-Smtp-Source: AGHT+IFvcoacZcqmKjVlcfvGKTUBeXwJNujhqrCXdRQ1KCKeKplLNH4V2pWPkKaOWPM/NS3weOopUQ== X-Received: by 2002:a05:622a:a092:b0:42b:e315:7ccc with SMTP id jv18-20020a05622aa09200b0042be3157cccmr179396qtb.3.1706786457784; Thu, 01 Feb 2024 03:20:57 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCU0wMEaqr74OBV3IHDIugd73kg2KetkRS38VGng+OIGRFCMvNEaekrFtURewaeEdG1F+u9/Dgblys1sQbiYMbf0aQ721uWy38FP0H3tS52Lll7Nnd6MzgPnoTkvCvwSFjyMZfDB+tqeouwk4ZabKh3wxFG2TdnVOgVN36+uHAfY2lrDw86DWsq0szNB2c0Z88GfYVP3UvNpfImSbsrnQk4ZtHneNxIVlTFIVZjuazK2tPnU/mUzMyY2bDGXC7cCmbnq9z5QyHZCmR4tct3UW5Ob1HmPjsdefDb9lHvU1u7X47hqi903Ur/iknc83B+y0sRAb+Nk9xBK/7wZ+CjoUCiFO8fFqK+HoCqF7qh7f7NpajjOvppdHZVfdaocn45UkvEqLwIIPKuykdNqtIfGKrTU6L3sUDmSC+K6z69OwNWqiGezCWTITsE9hiskswGIKfHkyMD98Y1frCYc+9ScqnwGMgqzbigCvpCJthk9nFW+zUdzQ5xjlitPeTAq2rmJUuw= Received: from google.com (161.126.77.34.bc.googleusercontent.com. [34.77.126.161]) by smtp.gmail.com with ESMTPSA id d1-20020a814f01000000b0060406f89705sm1031527ywb.144.2024.02.01.03.20.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Feb 2024 03:20:57 -0800 (PST) Date: Thu, 1 Feb 2024 11:20:53 +0000 From: Sebastian Ene To: Oliver Upton Cc: will@kernel.org, James Morse , Suzuki K Poulose , Zenghui Yu , 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: References: <20231218135859.2513568-2-sebastianene@google.com> <20231218135859.2513568-4-sebastianene@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240201_032100_233614_436F9354 X-CRM114-Status: GOOD ( 39.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > > #include > > #include > > +#include > > > > #include > > #include > > @@ -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 > > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel